You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2022/11/29 14:37:27 UTC
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #6924: [Draft] User two factor authentication
weizhouapache commented on code in PR #6924:
URL: https://github.com/apache/cloudstack/pull/6924#discussion_r1034820810
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -139,8 +142,8 @@
import com.cloud.projects.dao.ProjectAccountDao;
import com.cloud.projects.dao.ProjectDao;
import com.cloud.region.ha.GlobalLoadBalancingRulesService;
-import com.cloud.server.auth.UserAuthenticator;
-import com.cloud.server.auth.UserAuthenticator.ActionOnFailedAuthentication;
+import org.apache.cloudstack.auth.UserAuthenticator;
Review Comment:
@harikrishna-patnala
the imports in many classes are not ordered after moving com.cloud.server.auth.UserAuthenticator to org.apache.cloudstack.auth.UserAuthenticator
can you fix them ?
##########
ui/src/views/dashboard/Dashboard.vue:
##########
@@ -56,6 +60,7 @@ export default {
}
},
created () {
+ console.log(this.$store.getters.twoFaEnabled)
Review Comment:
this seems unnecesary
##########
setup/db/22beta4to22GA.sql:
##########
@@ -61,7 +61,7 @@ ALTER TABLE `cloud`.`user_ip_address` DROP COLUMN public_ip_address;
ALTER TABLE `cloud`.`user_ip_address` CHANGE public_ip_address1 public_ip_address char(40) NOT NULL COMMENT 'the public ip address';
DROP VIEW if exists port_forwarding_rules_view;
-ALTER TABLE `cloud`.`port_forwarding_rules` ADD COLUMN `dest_ip_address1` char(40) NOT NULL COMMENT 'the destination ip address';
+ ALTER TABLE `cloud`.`port_forwarding_rules` ADD COLUMN `dest_ip_address1` char(40) NOT NULL COMMENT 'the destination ip address';
Review Comment:
this seems unnecessary.
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -315,6 +322,31 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
private int _cleanupInterval;
private List<String> apiNameList;
+ protected static Map<String, UserTwoFactorAuthenticator> userTwoFactorAuthenticationProvidersMap = new HashMap<>();
+
+ private List<UserTwoFactorAuthenticator> userTwoFactorAuthenticationProviders;
+
+ static ConfigKey<Boolean> enableUserTwoFactorAuthentication = new ConfigKey<Boolean>("Advanced",
+ Boolean.class,
+ "enable.user.two.factor.authentication",
+ "false",
+ "Determines whether two factor authentication is enabled or not. This can also be configured at domain level.",
+ false,
Review Comment:
@harikrishna-patnala
these settings are dynamic , right ?
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -315,6 +322,31 @@ public class AccountManagerImpl extends ManagerBase implements AccountManager, M
private int _cleanupInterval;
private List<String> apiNameList;
+ protected static Map<String, UserTwoFactorAuthenticator> userTwoFactorAuthenticationProvidersMap = new HashMap<>();
+
+ private List<UserTwoFactorAuthenticator> userTwoFactorAuthenticationProviders;
+
+ static ConfigKey<Boolean> enableUserTwoFactorAuthentication = new ConfigKey<Boolean>("Advanced",
+ Boolean.class,
+ "enable.user.two.factor.authentication",
+ "false",
+ "Determines whether two factor authentication is enabled or not. This can also be configured at domain level.",
+ false,
+ ConfigKey.Scope.Domain);
+
+ ConfigKey<Boolean> mandateUserTwoFactorAuthentication = new ConfigKey<Boolean>("Advanced",
+ Boolean.class,
+ "mandate.user.two.factor.authentication",
+ "false",
+ "Determines whether to make the two factor authentication mandatory or not. This can also be configured at domain level.",
+ false,
+ ConfigKey.Scope.Domain);
+
+ ConfigKey<String> userTwoFactorAuthenticationDefaultProvider = new ConfigKey<>("Advanced", String.class,
+ "user.two.factor.authentication.default.provider",
+ "GOOGLE",
+ "The default user two factor authentication provider plugin. Eg. google, staticpin", true, ConfigKey.Scope.Domain);
Review Comment:
is there check on the value of config `userTwoFactorAuthenticationDefaultProvider` ?
##########
plugins/user-two-factor-authenticators/static-pin/src/main/java/org/apache/cloudstack/auth/StaticPinUserTwoFactorAuthenticator.java:
##########
@@ -0,0 +1,74 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package org.apache.cloudstack.auth;
+
+import javax.inject.Inject;
+
+import com.cloud.exception.CloudAuthenticationException;
+import com.cloud.user.UserAccount;
+import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
+
+import com.cloud.user.dao.UserAccountDao;
+import com.cloud.utils.component.AdapterBase;
+
+import java.util.Random;
+
+public class StaticPinUserTwoFactorAuthenticator extends AdapterBase implements UserTwoFactorAuthenticator {
+ public static final Logger s_logger = Logger.getLogger(StaticPinUserTwoFactorAuthenticator.class);
+
+ @Inject
+ private UserAccountDao _userAccountDao;
+
+ @Override
+ public String getName() {
+ return "staticpin";
+ }
+
+ @Override
+ public String getDescription() {
+ return "Static Pin user two factor authentication provider Plugin";
+ }
+
+ @Override
+ public void check2FA(String code, UserAccount userAccount) throws CloudAuthenticationException {
+ String expectedCode = getStaticPin(userAccount);
+ if (expectedCode.equals(code)) {
+ s_logger.info("2FA matches user's input");
+ return;
+ }
+ throw new CloudAuthenticationException("two-factor authentication code provided is invalid");
+ }
+
+ private String getStaticPin(UserAccount userAccount) {
+ return userAccount.getKeyFor2fa();
+ }
+
+ @Override
+ public String setup2FAKey(UserAccount userAccount) {
+ if (StringUtils.isNotEmpty(userAccount.getKeyFor2fa())) {
+ throw new CloudRuntimeException(String.format("2FA key is already setup for the user account %s", userAccount.getAccountName()));
+ }
+ long seed = System.currentTimeMillis();
+ Random rng = new Random(seed);
Review Comment:
@harikrishna-patnala
would be good to use SecureRandom ?
##########
server/src/main/java/com/cloud/user/AccountManagerImpl.java:
##########
@@ -3100,6 +3171,151 @@ public String getConfigComponentName() {
@Override
public ConfigKey<?>[] getConfigKeys() {
- return new ConfigKey<?>[] {UseSecretKeyInResponse};
+ return new ConfigKey<?>[] {UseSecretKeyInResponse, enableUserTwoFactorAuthentication, userTwoFactorAuthenticationDefaultProvider, mandateUserTwoFactorAuthentication};
+ }
+
+ public List<UserTwoFactorAuthenticator> getUserTwoFactorAuthenticationProviders() {
+ return userTwoFactorAuthenticationProviders;
+ }
+
+ public void setUserTwoFactorAuthenticationProviders(final List<UserTwoFactorAuthenticator> userTwoFactorAuthenticationProviders) {
+ this.userTwoFactorAuthenticationProviders = userTwoFactorAuthenticationProviders;
+ }
+
+ private void initializeUserTwoFactorAuthenticationProvidersMap() {
+ if (userTwoFactorAuthenticationProviders != null) {
+ for (final UserTwoFactorAuthenticator userTwoFactorAuthenticator : userTwoFactorAuthenticationProviders) {
+ userTwoFactorAuthenticationProvidersMap.put(userTwoFactorAuthenticator.getName().toLowerCase(), userTwoFactorAuthenticator);
+ }
+ }
+ }
+
+ @Override
+ public void verifyUsingTwoFactorAuthenticationCode(final String code, final Long domainId, final Long userAccountId) {
+
+ Account caller = CallContext.current().getCallingAccount();
+ Account owner = _accountService.getActiveAccountById(caller.getId());
+
+ checkAccess(caller, null, true, owner);
+
+ UserAccount userAccount = _accountService.getUserAccountById(userAccountId);
+ if (!userAccount.isUser2faEnabled()) {
+ throw new CloudRuntimeException(String.format("Two factor authentication is not enabled on the user: %s", userAccount.getUsername()));
+ }
+ UserTwoFactorAuthenticator userTwoFactorAuthenticator = getUserTwoFactorAuthenticator(domainId, userAccountId);
+ userTwoFactorAuthenticator.check2FA(code, userAccount);
+ }
+
+ @Override
+ public UserTwoFactorAuthenticator getUserTwoFactorAuthenticator(Long domainId, Long userAccountId) {
+ if (userAccountId != null) {
+ UserAccount userAccount = _accountService.getUserAccountById(userAccountId);
+ if (userAccount.getUser2faProvider() != null) {
+ return getUserTwoFactorAuthenticator(userAccount.getUser2faProvider());
+ }
+ }
+ final String name = userTwoFactorAuthenticationDefaultProvider.valueIn(domainId);
+ return getUserTwoFactorAuthenticator(name);
+ }
+
+ @Override
+ public UserTwoFactorAuthenticationSetupResponse setupUserTwoFactorAuthentication(SetupUserTwoFactorAuthenticationCmd cmd) {
+ String providerName = cmd.getProvider();
+
+ Account caller = CallContext.current().getCallingAccount();
+ Account owner = _accountService.getActiveAccountById(caller.getId());
+
+ if (cmd.getEnable()) {
+ checkAccess(caller, null, true, owner);
+ Long userId = CallContext.current().getCallingUserId();
+
+ UserTwoFactorAuthenticationSetupResponse response = enableTwoFactorAuthentication(userId, providerName);
+ return response;
+ }
+
+ // Admin can disable 2FA of the users
+ Long userId = cmd.getUserId();
+ UserTwoFactorAuthenticationSetupResponse response = disableTwoFactorAuthentication(userId, caller, owner);
+
+ return response;
+ }
+
+ protected UserTwoFactorAuthenticationSetupResponse enableTwoFactorAuthentication(Long userId, String providerName) {
+ UserAccountVO userAccount = _userAccountDao.findById(userId);
+ UserVO userVO = _userDao.findById(userId);
+
+ if (!enableUserTwoFactorAuthentication.valueIn(userAccount.getDomainId())) {
+ throw new CloudRuntimeException("2FA is not enabled for this domain or at global level");
+ }
+
+ if (StringUtils.isEmpty(providerName)) {
+ throw new InvalidParameterValueException("Provider name is mandatory to setup 2FA");
+ }
+ UserTwoFactorAuthenticator provider = getUserTwoFactorAuthenticationProvider(providerName);
+ String code = provider.setup2FAKey(userAccount);
+ UserVO user = _userDao.createForUpdate();
+ user.setKeyFor2fa(code);
+ user.setUser2faProvider(provider.getName());
+ user.setUser2faEnabled(true);
+ _userDao.update(userId, user);
+
+ UserTwoFactorAuthenticationSetupResponse response = new UserTwoFactorAuthenticationSetupResponse();
+ response.setId(userVO.getUuid());
+ response.setUsername(userAccount.getUsername());
+ response.setSecretCode(code);
+
+ return response;
}
+
+ protected UserTwoFactorAuthenticationSetupResponse disableTwoFactorAuthentication(Long userId, Account caller, Account owner) {
+ UserVO userVO = null;
+ if (userId != null) {
+ userVO = validateUser(userId, caller.getDomainId());
+ if (userVO == null) {
+ throw new InvalidParameterValueException("Unable to find user= " + userVO.getUsername() + " in domain id = " + caller.getDomainId());
+ }
+ owner = _accountService.getActiveAccountById(userVO.getAccountId());
+ } else {
+ userId = CallContext.current().getCallingUserId();
+ userVO = _userDao.findById(userId);
+ }
+ checkAccess(caller, null, true, owner);
+
+ UserVO user = _userDao.createForUpdate();
+ user.setKeyFor2fa(null);
+ user.setUser2faProvider(null);
+ user.setUser2faEnabled(false);
+ _userDao.update(userVO.getId(), user);
+
+ UserTwoFactorAuthenticationSetupResponse response = new UserTwoFactorAuthenticationSetupResponse();
+ response.setId(userVO.getUuid());
+ response.setUsername(userVO.getUsername());
+
+ return response;
+ }
+
+ private UserVO validateUser(Long userId, Long domainId) {
+ UserVO user = null;
+ if (userId != null) {
+ user = _userDao.findById(userId);
+ if (user == null) {
+ throw new InvalidParameterValueException("Invalid user ID provided");
+ }
+ if (_accountDao.findById(user.getAccountId()).getDomainId() != domainId) {
+ throw new InvalidParameterValueException("User doesn't belong to the specified account or domain");
+ }
+ }
+ return user;
+ }
+
+ public UserTwoFactorAuthenticator getUserTwoFactorAuthenticator(final String name) {
+ if (StringUtils.isEmpty(name)) {
+ throw new CloudRuntimeException("Invalid UserTwoFactorAuthenticator name provided");
+ }
+ if (!userTwoFactorAuthenticationProvidersMap.containsKey(name)) {
Review Comment:
is the provider name case-sensitive ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org