From 2e5774219b8c5b4492fefe90d1ac1e8ab534ec65 Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Fri, 18 Apr 2025 17:11:27 +0200 Subject: [PATCH 1/6] KARAF-5014: consider first group role in users.properties and ignore empty roles --- .../resources/resources/etc/keys.properties | 2 +- .../resources/resources/etc/users.properties | 2 +- .../src/test/resources/etc1/users.properties | 2 +- .../src/test/resources/etc2/users.properties | 2 +- .../src/test/resources/etc/users.properties | 2 +- .../src/test/resources/etc/users.properties | 2 +- .../AbstractPropertiesBackingEngine.java | 296 ++++++++++++++++++ .../apache/karaf/jaas/modules/JAASUtils.java | 37 ++- .../properties/DigestPasswordLoginModule.java | 28 +- .../properties/PropertiesBackingEngine.java | 260 +-------------- .../properties/PropertiesLoginModule.java | 26 +- .../publickey/PublickeyBackingEngine.java | 253 +-------------- .../publickey/PublickeyLoginModule.java | 25 +- .../PropertiesBackingEngineTest.java | 64 +++- .../properties/PropertiesLoginModuleTest.java | 7 +- .../publickey/PublicKeyLoginModuleTest.java | 101 +++--- .../karaf/jaas/modules/publickey/pubkey.users | 4 +- 17 files changed, 481 insertions(+), 632 deletions(-) create mode 100644 jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java diff --git a/assemblies/features/base/src/main/resources/resources/etc/keys.properties b/assemblies/features/base/src/main/resources/resources/etc/keys.properties index e0538ff234b..108587147ed 100644 --- a/assemblies/features/base/src/main/resources/resources/etc/keys.properties +++ b/assemblies/features/base/src/main/resources/resources/etc/keys.properties @@ -33,4 +33,4 @@ # The user guide describes how to generate/update the key. # #karaf=AAAAB3NzaC1kc3MAAACBAP1/U4EddRIpUt9KnC7s5Of2EbdSPO9EAMMeP4C2USZpRV1AIlH7WT2NWPq/xfW6MPbLm1Vs14E7gB00b/JmYLdrmVClpJ+f6AR7ECLCT7up1/63xhv4O1fnxqimFQ8E+4P208UewwI1VBNaFpEy9nXzrith1yrv8iIDGZ3RSAHHAAAAFQCXYFCPFSMLzLKSuYKi64QL8Fgc9QAAAIEA9+GghdabPd7LvKtcNrhXuXmUr7v6OuqC+VdMCz0HgmdRWVeOutRZT+ZxBxCBgLRJFnEj6EwoFhO3zwkyjMim4TwWeotUfI0o4KOuHiuzpnWRbqN/C/ohNWLx+2J6ASQ7zKTxvqhRkImog9/hWuWfBpKLZl6Ae1UlZAFMO/7PSSoAAACBAKKSU2PFl/qOLxIwmBZPPIcJshVe7bVUpFvyl3BbJDow8rXfskl8wO63OzP/qLmcJM0+JbcRU/53JjTuyk31drV2qxhIOsLDC9dGCWj47Y7TyhPdXh/0dthTRBy6bqGtRPxGa7gJov1xm/UuYYXPIUR/3x9MAZvZ5xvE0kYXO+rx,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh diff --git a/assemblies/features/base/src/main/resources/resources/etc/users.properties b/assemblies/features/base/src/main/resources/resources/etc/users.properties index 189118356c2..f8567bf45fd 100644 --- a/assemblies/features/base/src/main/resources/resources/etc/users.properties +++ b/assemblies/features/base/src/main/resources/resources/etc/users.properties @@ -30,4 +30,4 @@ # with the name "karaf". # #karaf = karaf,_g_:admingroup -#_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +#_g_\:admingroup = admin,manager,viewer,systembundles,ssh diff --git a/client/src/test/resources/etc1/users.properties b/client/src/test/resources/etc1/users.properties index 67a65106ee4..ef2801d163b 100644 --- a/client/src/test/resources/etc1/users.properties +++ b/client/src/test/resources/etc1/users.properties @@ -18,4 +18,4 @@ ################################################################################ karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles +_g_\:admingroup = admin,manager,viewer,systembundles diff --git a/client/src/test/resources/etc2/users.properties b/client/src/test/resources/etc2/users.properties index 02bfd0ac43b..bf67c5fc5ef 100644 --- a/client/src/test/resources/etc2/users.properties +++ b/client/src/test/resources/etc2/users.properties @@ -17,7 +17,7 @@ # ################################################################################ -_g_\:admingroup = group,admin,manager,viewer,systembundles +_g_\:admingroup = admin,manager,viewer,systembundles test = admin,_g_:admingroup admin = admin,_g_:admingroup karaf = karaf,_g_:admingroup diff --git a/examples/karaf-itest-example/src/test/resources/etc/users.properties b/examples/karaf-itest-example/src/test/resources/etc/users.properties index ace22826cc8..e829a617ea5 100644 --- a/examples/karaf-itest-example/src/test/resources/etc/users.properties +++ b/examples/karaf-itest-example/src/test/resources/etc/users.properties @@ -30,4 +30,4 @@ # with the name "karaf". # karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh diff --git a/itests/test/src/test/resources/etc/users.properties b/itests/test/src/test/resources/etc/users.properties index ace22826cc8..e829a617ea5 100644 --- a/itests/test/src/test/resources/etc/users.properties +++ b/itests/test/src/test/resources/etc/users.properties @@ -30,4 +30,4 @@ # with the name "karaf". # karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java new file mode 100644 index 00000000000..5eef6ca942d --- /dev/null +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java @@ -0,0 +1,296 @@ +/* + * 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.karaf.jaas.modules; + +import org.apache.felix.utils.properties.Properties; +import org.apache.karaf.jaas.boot.principal.GroupPrincipal; +import org.apache.karaf.jaas.boot.principal.RolePrincipal; +import org.apache.karaf.jaas.boot.principal.UserPrincipal; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import java.security.Principal; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public abstract class AbstractPropertiesBackingEngine implements BackingEngine { + + private static final Logger LOGGER = LoggerFactory.getLogger(AbstractPropertiesBackingEngine.class); + + private Properties users; + + public AbstractPropertiesBackingEngine(Properties users) { + this.users = users; + } + + protected void addUserInternal(String username, String password) { + String[] infos = null; + StringBuilder userInfoBuffer = new StringBuilder(); + + String userInfos = users.get(username); + + // if user already exists, update password + if (userInfos != null && userInfos.length() > 0) { + infos = userInfos.split(","); + userInfoBuffer.append(password); + + for (int i = 1; i < infos.length; i++) { + userInfoBuffer.append(","); + userInfoBuffer.append(infos[i]); + } + String newUserInfo = userInfoBuffer.toString(); + users.put(username, newUserInfo); + } else { + users.put(username, password); + } + + try { + users.save(); + } catch (Exception ex) { + LOGGER.error("Cannot update users file,", ex); + } + } + + @Override + public void deleteUser(String username) { + // delete all its groups first, for garbage collection of the groups + for (GroupPrincipal gp : listGroups(username)) { + deleteGroup(username, gp.getName()); + } + + users.remove(username); + + try { + users.save(); + } catch (Exception ex) { + LOGGER.error("Cannot remove users file,", ex); + } + } + + @Override + public List listUsers() { + List result = new ArrayList<>(); + + for (Object user : users.keySet()) { + String userName = (String) user; + if (userName.startsWith(GROUP_PREFIX)) + continue; + + UserPrincipal userPrincipal = new UserPrincipal(userName); + result.add(userPrincipal); + } + return result; + } + + @Override + public UserPrincipal lookupUser(String username) { + for (UserPrincipal userPrincipal : listUsers()) { + if (userPrincipal.getName().equals(username)) { + return userPrincipal; + } + } + return null; + } + + @Override + public List listRoles(Principal principal) { + String userName = principal.getName(); + if (principal instanceof GroupPrincipal) { + userName = GROUP_PREFIX + userName; + } + return listRoles(userName); + } + + protected List listRoles(String name) { + List result = new ArrayList<>(); + + String userInfo = users.get(name); + String[] infos = userInfo.split(","); + for (int i = getFirstRoleIndex(name); i < infos.length; i++) { + String roleName = infos[i]; + if (roleName.trim().isEmpty()) { + // ignore + } else if (roleName.startsWith(GROUP_PREFIX)) { + for (RolePrincipal rp : listRoles(roleName)) { + if (!result.contains(rp)) { + result.add(rp); + } + } + } else { + RolePrincipal rp = new RolePrincipal(roleName); + if (!result.contains(rp)) { + result.add(rp); + } + } + } + return result; + } + + /** + * Determines the starting index of role definitions for a given property name. + * @param name the property to evaluate, can represent either a group or a user + * @return 0 if the property starts with the group prefix, otherwise 1 + */ + private int getFirstRoleIndex(String name) { + if (name.trim().startsWith(GROUP_PREFIX)) + return 0; + return 1; + } + + @Override + public void addRole(String username, String role) { + String userInfos = users.get(username); + if (userInfos != null) { + // for groups, empty info should be replaced with role + // for users, empty info means empty password and role should be appended + if (userInfos.trim().isEmpty() && username.trim().startsWith(GROUP_PREFIX)) { + users.put(username, role); + } else { + for (RolePrincipal rp : listRoles(username)) { + if (role.equals(rp.getName())) { + return; + } + } + for (GroupPrincipal gp : listGroups(username)) { + if (role.equals(GROUP_PREFIX + gp.getName())) { + return; + } + } + String newUserInfos = userInfos + "," + role; + users.put(username, newUserInfos); + } + } + try { + users.save(); + } catch (Exception ex) { + LOGGER.error("Cannot update users file,", ex); + } + } + + @Override + public void deleteRole(String username, String role) { + String[] infos = null; + StringBuilder userInfoBuffer = new StringBuilder(); + + String userInfos = users.get(username); + + // if user already exists, remove the role + if (userInfos != null && userInfos.length() > 0) { + infos = userInfos.split(","); + + int firstRoleIndex = getFirstRoleIndex(username); + if (firstRoleIndex == 1) { // index 0 is password + String password = infos[0]; + userInfoBuffer.append(password); + } + for (int i = firstRoleIndex; i < infos.length; i++) { + if (infos[i] != null && !infos[i].equals(role)) { + if(userInfoBuffer.length() > 0) { + userInfoBuffer.append(","); + } + userInfoBuffer.append(infos[i]); + } + } + String newUserInfo = userInfoBuffer.toString(); + users.put(username, newUserInfo); + } + + try { + users.save(); + } catch (Exception ex) { + LOGGER.error("Cannot update users file,", ex); + } + } + + @Override + public List listGroups(UserPrincipal user) { + String userName = user.getName(); + return listGroups(userName); + } + + private List listGroups(String userName) { + List result = new ArrayList<>(); + String userInfo = users.get(userName); + if (userInfo != null) { + String[] infos = userInfo.split(","); + for (int i = getFirstRoleIndex(userName); i < infos.length; i++) { + String name = infos[i]; + if (name.startsWith(GROUP_PREFIX)) { + result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length()))); + } + } + } + return result; + } + + @Override + public void addGroup(String username, String group) { + String groupName = GROUP_PREFIX + group; + if (users.get(groupName) == null) { + addUserInternal(groupName, ""); // groups don't have password + } + addRole(username, groupName); + } + + @Override + public void deleteGroup(String username, String group) { + deleteRole(username, GROUP_PREFIX + group); + + // garbage collection, clean up the groups if needed + for (UserPrincipal user : listUsers()) { + for (GroupPrincipal g : listGroups(user)) { + if (group.equals(g.getName())) { + // there is another user of this group, nothing to clean up + return; + } + } + } + + // nobody is using this group any more, remove it + deleteUser(GROUP_PREFIX + group); + } + + @Override + public void addGroupRole(String group, String role) { + addRole(GROUP_PREFIX + group, role); + } + + @Override + public void deleteGroupRole(String group, String role) { + deleteRole(GROUP_PREFIX + group, role); + } + + public Map listGroups() { + Map result = new HashMap<>(); + for (String name : users.keySet()) { + if (name.startsWith(GROUP_PREFIX)) { + result.put(new GroupPrincipal(name.substring(GROUP_PREFIX.length())), users.get(name)); + } + } + return result; + } + + @Override + public void createGroup(String group) { + String groupName = GROUP_PREFIX + group; + if (users.get(groupName) == null) { + addUserInternal(groupName, ""); // groups don't have password + } else { + throw new IllegalArgumentException("Group: " + group + " already exist"); + } + } +} \ No newline at end of file diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java index 4d6317995cc..1659783ae84 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java @@ -14,7 +14,14 @@ */ package org.apache.karaf.jaas.modules; +import org.apache.felix.utils.properties.Properties; +import org.apache.karaf.jaas.boot.principal.GroupPrincipal; +import org.apache.karaf.jaas.boot.principal.RolePrincipal; +import org.apache.karaf.jaas.boot.principal.UserPrincipal; +import java.security.Principal; +import java.util.HashSet; import java.util.Map; +import java.util.Set; public final class JAASUtils { @@ -30,4 +37,32 @@ public static String getString(Map options, String key) { return (String)val; } -} + public static Set getPrincipals(String user, Properties users, String[] infos) { + Set principals = new HashSet<>(); + principals.add(new UserPrincipal(user)); + + for (int i = 1; i < infos.length; i++) { + if (infos[i].trim().startsWith(BackingEngine.GROUP_PREFIX)) { + // it's a group reference + principals.add(new GroupPrincipal(infos[i].trim().substring(BackingEngine.GROUP_PREFIX.length()))); + String groupInfo = users.get(infos[i].trim()); + if (groupInfo != null) { + String[] roles = groupInfo.split(","); + for (int j = 0; j < roles.length; j++) { + addRole(principals, roles[j]); + } + } + } else { + // it's an user reference + addRole(principals, infos[i]); + } + } + return principals; + } + + static void addRole(Set principals, String role) { + role = role.trim(); + if (!role.isEmpty()) + principals.add(new RolePrincipal(role)); + } +} \ No newline at end of file diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java index e4c33475aa1..ae463cc9fc6 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java @@ -21,8 +21,8 @@ import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; -import java.util.HashSet; import java.util.Map; + import javax.security.auth.Subject; import javax.security.auth.callback.Callback; import javax.security.auth.callback.CallbackHandler; @@ -34,9 +34,6 @@ import org.apache.commons.codec.binary.Base64; import org.apache.felix.utils.properties.Properties; -import org.apache.karaf.jaas.boot.principal.GroupPrincipal; -import org.apache.karaf.jaas.boot.principal.RolePrincipal; -import org.apache.karaf.jaas.boot.principal.UserPrincipal; import org.apache.karaf.jaas.modules.AbstractKarafLoginModule; import org.apache.karaf.jaas.modules.JAASUtils; import org.slf4j.Logger; @@ -45,7 +42,7 @@ /** * JAAS Login module for user / password, based on two properties files. */ -public class DigestPasswordLoginModule extends AbstractKarafLoginModule { +public class DigestPasswordLoginModule extends AbstractKarafLoginModule { private static final transient Logger LOGGER = LoggerFactory.getLogger(DigestPasswordLoginModule.class); @@ -205,24 +202,7 @@ public boolean login() throws LoginException { } } - principals = new HashSet<>(); - principals.add(new UserPrincipal(user)); - for (int i = 1; i < infos.length; i++) { - if (infos[i].trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) { - // it's a group reference - principals.add(new GroupPrincipal(infos[i].trim().substring(PropertiesBackingEngine.GROUP_PREFIX.length()))); - String groupInfo = users.get(infos[i].trim()); - if (groupInfo != null) { - String[] roles = groupInfo.split(","); - for (int j = 1; j < roles.length; j++) { - principals.add(new RolePrincipal(roles[j].trim())); - } - } - } else { - // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); - } - } + principals = JAASUtils.getPrincipals(user, users, infos); users.clear(); @@ -233,4 +213,4 @@ public boolean login() throws LoginException { return true; } -} +} \ No newline at end of file diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java index 9329a0a8bc6..aee378cae74 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java @@ -15,35 +15,21 @@ */ package org.apache.karaf.jaas.modules.properties; -import java.security.Principal; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - import org.apache.felix.utils.properties.Properties; -import org.apache.karaf.jaas.boot.principal.GroupPrincipal; -import org.apache.karaf.jaas.boot.principal.RolePrincipal; -import org.apache.karaf.jaas.boot.principal.UserPrincipal; -import org.apache.karaf.jaas.modules.BackingEngine; +import org.apache.karaf.jaas.modules.AbstractPropertiesBackingEngine; import org.apache.karaf.jaas.modules.encryption.EncryptionSupport; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -public class PropertiesBackingEngine implements BackingEngine { - private static final transient Logger LOGGER = LoggerFactory.getLogger(PropertiesBackingEngine.class); +public class PropertiesBackingEngine extends AbstractPropertiesBackingEngine { - private Properties users; private EncryptionSupport encryptionSupport; public PropertiesBackingEngine(Properties users) { - this.users = users; + super(users); this.encryptionSupport = EncryptionSupport.noEncryptionSupport(); } public PropertiesBackingEngine(Properties users, EncryptionSupport encryptionSupport) { - this.users = users; + super(users); this.encryptionSupport = encryptionSupport; } @@ -52,240 +38,6 @@ public void addUser(String username, String password) { if (username.startsWith(GROUP_PREFIX)) throw new IllegalArgumentException("Prefix not permitted: " + GROUP_PREFIX); - addUserInternal(username, password); - } - - private void addUserInternal(String username, String password) { - String[] infos = null; - StringBuilder userInfoBuffer = new StringBuilder(); - - String encPassword = encryptionSupport.encrypt(password); - String userInfos = users.get(username); - - //If user already exists, update password - if (userInfos != null && userInfos.length() > 0) { - infos = userInfos.split(","); - userInfoBuffer.append(encPassword); - - for (int i = 1; i < infos.length; i++) { - userInfoBuffer.append(","); - userInfoBuffer.append(infos[i]); - } - String newUserInfo = userInfoBuffer.toString(); - users.put(username, newUserInfo); - } else { - users.put(username, encPassword); - } - - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); - } + addUserInternal(username, encryptionSupport.encrypt(password)); } - - @Override - public void deleteUser(String username) { - // delete all its groups first, for garbage collection of the groups - for (GroupPrincipal gp : listGroups(username)) { - deleteGroup(username, gp.getName()); - } - - users.remove(username); - - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot remove users file,", ex); - } - } - - @Override - public List listUsers() { - List result = new ArrayList<>(); - - for (Object user : users.keySet()) { - String userName = (String) user; - if (userName.startsWith(GROUP_PREFIX)) - continue; - - UserPrincipal userPrincipal = new UserPrincipal(userName); - result.add(userPrincipal); - } - return result; - } - - @Override - public UserPrincipal lookupUser(String username) { - for (UserPrincipal userPrincipal : listUsers()) { - if (userPrincipal.getName().equals(username)) { - return userPrincipal; - } - } - return null; - } - - @Override - public List listRoles(Principal principal) { - String userName = principal.getName(); - if (principal instanceof GroupPrincipal) { - userName = GROUP_PREFIX + userName; - } - return listRoles(userName); - } - - private List listRoles(String name) { - - List result = new ArrayList<>(); - String userInfo = users.get(name); - String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { - String roleName = infos[i]; - if (roleName.startsWith(GROUP_PREFIX)) { - for (RolePrincipal rp : listRoles(roleName)) { - if (!result.contains(rp)) { - result.add(rp); - } - } - } else { - RolePrincipal rp = new RolePrincipal(roleName); - if (!result.contains(rp)) { - result.add(rp); - } - } - } - return result; - } - - @Override - public void addRole(String username, String role) { - String userInfos = users.get(username); - if (userInfos != null) { - for (RolePrincipal rp : listRoles(username)) { - if (role.equals(rp.getName())) { - return; - } - } - for (GroupPrincipal gp : listGroups(username)) { - if (role.equals(GROUP_PREFIX + gp.getName())) { - return; - } - } - String newUserInfos = userInfos + "," + role; - users.put(username, newUserInfos); - } - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); - } - } - - @Override - public void deleteRole(String username, String role) { - String[] infos = null; - StringBuilder userInfoBuffer = new StringBuilder(); - - String userInfos = users.get(username); - - //If user already exists, remove the role - if (userInfos != null && userInfos.length() > 0) { - infos = userInfos.split(","); - String password = infos[0]; - userInfoBuffer.append(password); - - for (int i = 1; i < infos.length; i++) { - if (infos[i] != null && !infos[i].equals(role)) { - userInfoBuffer.append(","); - userInfoBuffer.append(infos[i]); - } - } - String newUserInfo = userInfoBuffer.toString(); - users.put(username, newUserInfo); - } - - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); - } - } - - @Override - public List listGroups(UserPrincipal user) { - String userName = user.getName(); - return listGroups(userName); - } - - private List listGroups(String userName) { - List result = new ArrayList<>(); - String userInfo = users.get(userName); - if (userInfo != null) { - String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { - String name = infos[i]; - if (name.startsWith(GROUP_PREFIX)) { - result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length()))); - } - } - } - return result; - } - - @Override - public void addGroup(String username, String group) { - String groupName = GROUP_PREFIX + group; - if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); - } - addRole(username, groupName); - } - - @Override - public void deleteGroup(String username, String group) { - deleteRole(username, GROUP_PREFIX + group); - - // garbage collection, clean up the groups if needed - for (UserPrincipal user : listUsers()) { - for (GroupPrincipal g : listGroups(user)) { - if (group.equals(g.getName())) { - // there is another user of this group, nothing to clean up - return; - } - } - } - - // nobody is using this group any more, remove it - deleteUser(GROUP_PREFIX + group); - } - - @Override - public void addGroupRole(String group, String role) { - addRole(GROUP_PREFIX + group, role); - } - - @Override - public void deleteGroupRole(String group, String role) { - deleteRole(GROUP_PREFIX + group, role); - } - - public Map listGroups() { - Map result = new HashMap<>(); - for (String name : users.keySet()) { - if (name.startsWith(GROUP_PREFIX)) { - result.put(new GroupPrincipal(name.substring(GROUP_PREFIX.length())), users.get(name)); - } - } - return result; - } - - public void createGroup(String group) { - String groupName = GROUP_PREFIX + group; - if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); - } else { - throw new IllegalArgumentException("Group: " + group + " already exist"); - } - } - -} +} \ No newline at end of file diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java index 556c0fa1d40..bc5bf4087a0 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java @@ -18,8 +18,8 @@ import java.io.File; import java.io.IOException; -import java.util.HashSet; import java.util.Map; + import javax.security.auth.Subject; import javax.security.auth.callback.Callback; import javax.security.auth.callback.CallbackHandler; @@ -30,9 +30,6 @@ import javax.security.auth.login.LoginException; import org.apache.felix.utils.properties.Properties; -import org.apache.karaf.jaas.boot.principal.GroupPrincipal; -import org.apache.karaf.jaas.boot.principal.RolePrincipal; -import org.apache.karaf.jaas.boot.principal.UserPrincipal; import org.apache.karaf.jaas.modules.AbstractKarafLoginModule; import org.apache.karaf.jaas.modules.JAASUtils; import org.slf4j.Logger; @@ -132,24 +129,7 @@ public boolean login() throws LoginException { } } - principals = new HashSet<>(); - principals.add(new UserPrincipal(user)); - for (int i = 1; i < infos.length; i++) { - if (infos[i].trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) { - // it's a group reference - principals.add(new GroupPrincipal(infos[i].trim().substring(PropertiesBackingEngine.GROUP_PREFIX.length()))); - String groupInfo = users.get(infos[i].trim()); - if (groupInfo != null) { - String[] roles = groupInfo.split(","); - for (int j = 1; j < roles.length; j++) { - principals.add(new RolePrincipal(roles[j].trim())); - } - } - } else { - // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); - } - } + principals = JAASUtils.getPrincipals(user, users, infos); users.clear(); @@ -160,4 +140,4 @@ public boolean login() throws LoginException { return true; } -} +} \ No newline at end of file diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyBackingEngine.java index 503780a5d47..866931b3add 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyBackingEngine.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyBackingEngine.java @@ -15,28 +15,13 @@ */ package org.apache.karaf.jaas.modules.publickey; -import java.security.Principal; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - import org.apache.felix.utils.properties.Properties; -import org.apache.karaf.jaas.boot.principal.GroupPrincipal; -import org.apache.karaf.jaas.boot.principal.RolePrincipal; -import org.apache.karaf.jaas.boot.principal.UserPrincipal; -import org.apache.karaf.jaas.modules.BackingEngine; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -public class PublickeyBackingEngine implements BackingEngine { +import org.apache.karaf.jaas.modules.AbstractPropertiesBackingEngine; - private static final transient Logger LOGGER = LoggerFactory.getLogger(PublickeyBackingEngine.class); - - private Properties users; +public class PublickeyBackingEngine extends AbstractPropertiesBackingEngine { public PublickeyBackingEngine(Properties users) { - this.users = users; + super(users); } @Override @@ -46,234 +31,4 @@ public void addUser(String username, String publickey) { addUserInternal(username, publickey); } - - private void addUserInternal(String username, String publickey) { - String[] infos = null; - StringBuilder userInfoBuffer = new StringBuilder(); - - String newPublickey = publickey; - - String userInfos = users.get(username); - - //If user already exists, update publickey - if (userInfos != null && userInfos.length() > 0) { - infos = userInfos.split(","); - userInfoBuffer.append(newPublickey); - - for (int i = 1; i < infos.length; i++) { - userInfoBuffer.append(","); - userInfoBuffer.append(infos[i]); - } - String newUserInfo = userInfoBuffer.toString(); - users.put(username, newUserInfo); - } else { - users.put(username, newPublickey); - } - - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); - } - } - - @Override - public void deleteUser(String username) { - // delete all its groups first, for garbage collection of the groups - for (GroupPrincipal gp : listGroups(username)) { - deleteGroup(username, gp.getName()); - } - - users.remove(username); - - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot remove users file,", ex); - } - } - - @Override - public List listUsers() { - List result = new ArrayList<>(); - - for (Object user : users.keySet()) { - String userName = (String) user; - if (userName.startsWith(GROUP_PREFIX)) - continue; - - UserPrincipal userPrincipal = new UserPrincipal(userName); - result.add(userPrincipal); - } - return result; - } - - @Override - public UserPrincipal lookupUser(String username) { - for (UserPrincipal userPrincipal : listUsers()) { - if (userPrincipal.getName().equals(username)) { - return userPrincipal; - } - } - return null; - } - - @Override - public List listRoles(Principal principal) { - String userName = principal.getName(); - if (principal instanceof GroupPrincipal) { - userName = GROUP_PREFIX + userName; - } - return listRoles(userName); - } - - private List listRoles(String name) { - - List result = new ArrayList<>(); - String userInfo = users.get(name); - String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { - String roleName = infos[i]; - if (roleName.startsWith(GROUP_PREFIX)) { - for (RolePrincipal rp : listRoles(roleName)) { - if (!result.contains(rp)) { - result.add(rp); - } - } - } else { - RolePrincipal rp = new RolePrincipal(roleName); - if (!result.contains(rp)) { - result.add(rp); - } - } - } - return result; - } - - @Override - public void addRole(String username, String role) { - String userInfos = users.get(username); - if (userInfos != null) { - for (RolePrincipal rp : listRoles(username)) { - if (role.equals(rp.getName())) { - return; - } - } - String newUserInfos = userInfos + "," + role; - users.put(username, newUserInfos); - } - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); - } - } - - @Override - public void deleteRole(String username, String role) { - String[] infos = null; - StringBuilder userInfoBuffer = new StringBuilder(); - - String userInfos = users.get(username); - - //If user already exists, remove the role - if (userInfos != null && userInfos.length() > 0) { - infos = userInfos.split(","); - String password = infos[0]; - userInfoBuffer.append(password); - - for (int i = 1; i < infos.length; i++) { - if (infos[i] != null && !infos[i].equals(role)) { - userInfoBuffer.append(","); - userInfoBuffer.append(infos[i]); - } - } - String newUserInfo = userInfoBuffer.toString(); - users.put(username, newUserInfo); - } - - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); - } - } - - @Override - public List listGroups(UserPrincipal user) { - String userName = user.getName(); - return listGroups(userName); - } - - private List listGroups(String userName) { - List result = new ArrayList<>(); - String userInfo = users.get(userName); - if (userInfo != null) { - String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { - String name = infos[i]; - if (name.startsWith(GROUP_PREFIX)) { - result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length()))); - } - } - } - return result; - } - - @Override - public void addGroup(String username, String group) { - String groupName = GROUP_PREFIX + group; - if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); - } - addRole(username, groupName); - } - - @Override - public void deleteGroup(String username, String group) { - deleteRole(username, GROUP_PREFIX + group); - - // garbage collection, clean up the groups if needed - for (UserPrincipal user : listUsers()) { - for (GroupPrincipal g : listGroups(user)) { - if (group.equals(g.getName())) { - // there is another user of this group, nothing to clean up - return; - } - } - } - - // nobody is using this group any more, remote it - deleteUser(GROUP_PREFIX + group); - } - - @Override - public void addGroupRole(String group, String role) { - addRole(GROUP_PREFIX + group, role); - } - - @Override - public void deleteGroupRole(String group, String role) { - deleteRole(GROUP_PREFIX + group, role); - } - - public Map listGroups() { - Map result = new HashMap<>(); - for (String name : users.keySet()) { - if (name.startsWith(GROUP_PREFIX)) { - result.put(new GroupPrincipal(name.substring(GROUP_PREFIX.length())), users.get(name)); - } - } - return result; - } - - public void createGroup(String group) { - String groupName = GROUP_PREFIX + group; - if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); - } else { - throw new IllegalArgumentException("Group: " + group + " already exist"); - } - } - -} +} \ No newline at end of file diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java index 644fa4f1b43..a4f39df5c42 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java @@ -40,7 +40,6 @@ import java.util.Base64; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import javax.security.auth.Subject; import javax.security.auth.callback.Callback; @@ -51,11 +50,8 @@ import javax.security.auth.login.LoginException; import org.apache.felix.utils.properties.Properties; -import org.apache.karaf.jaas.modules.BackingEngine; -import org.apache.karaf.jaas.boot.principal.GroupPrincipal; -import org.apache.karaf.jaas.boot.principal.RolePrincipal; -import org.apache.karaf.jaas.boot.principal.UserPrincipal; import org.apache.karaf.jaas.modules.AbstractKarafLoginModule; +import org.apache.karaf.jaas.modules.JAASUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -151,24 +147,7 @@ public boolean login() throws LoginException { } } - principals = new HashSet<>(); - principals.add(new UserPrincipal(user)); - for (int i = 1; i < infos.length; i++) { - if (infos[i].trim().startsWith(BackingEngine.GROUP_PREFIX)) { - // it's a group reference - principals.add(new GroupPrincipal(infos[i].trim().substring(BackingEngine.GROUP_PREFIX.length()))); - String groupInfo = users.get(infos[i].trim()); - if (groupInfo != null) { - String[] roles = groupInfo.split(","); - for (int j = 1; j < roles.length; j++) { - principals.add(new RolePrincipal(roles[j].trim())); - } - } - } else { - // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); - } - } + principals = JAASUtils.getPrincipals(user, users, infos); users.clear(); diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java index 1cb6a28fa8d..1fe4b2d5318 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java @@ -18,12 +18,16 @@ import static org.apache.karaf.jaas.modules.PrincipalHelper.names; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; +import java.util.Arrays; import java.util.List; import java.util.stream.Collectors; @@ -55,7 +59,7 @@ public void testUserRoles() throws IOException { engine.addRole("a", "role2"); UserPrincipal upa = getUser(engine, "a"); - Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); engine.addGroup("a", "g"); engine.addGroupRole("g", "role2"); @@ -64,8 +68,8 @@ public void testUserRoles() throws IOException { engine.addGroup("b", "g2"); engine.addGroupRole("g2", "role4"); - Assert.assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b")); - Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); + assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b")); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); checkLoading(); @@ -79,11 +83,11 @@ public void testUserRoles() throws IOException { GroupPrincipal gp = engine.listGroups(upa).iterator().next(); engine.deleteGroupRole("g", "role2"); - Assert.assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3")); + assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3")); // role2 should still be there as it was added to the user directly too - Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); - Assert.assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4")); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); + assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4")); engine.deleteGroup("b", "g"); engine.deleteGroup("b", "g2"); @@ -101,10 +105,10 @@ private void checkLoading() throws IOException { UserPrincipal upb_2 = getUser(engine, "b"); assertEquals(3, engine.listRoles(upa_2).size()); - Assert.assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3")); + assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3")); assertEquals(3, engine.listRoles(upb_2).size()); - Assert.assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4")); + assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4")); } private UserPrincipal getUser(PropertiesBackingEngine engine, String name) { @@ -114,6 +118,50 @@ private UserPrincipal getUser(PropertiesBackingEngine engine, String name) { return matchingUsers.iterator().next(); } + @Test + public void testUserPassword() throws IOException { + Properties p = new Properties(f); + PropertiesBackingEngine engine = new PropertiesBackingEngine(p); + + // update password when user has no roles + engine.addUser("a", "pass1"); + engine.addUser("a", "pass2"); + assertThat(Arrays.asList(p.get("a").split(",")), contains("pass2")); + UserPrincipal upa = getUser(engine, "a"); + assertTrue(engine.listRoles(upa).isEmpty()); + + // update empty password when user has no roles + engine.addUser("b", ""); + engine.addUser("b", "pass3"); + assertThat(Arrays.asList(p.get("b").split(",")), contains("pass3")); + UserPrincipal upb = getUser(engine, "b"); + assertTrue(engine.listRoles(upb).isEmpty()); + + // update password when user has roles + engine.addUser("c", "pass4"); + engine.addRole("c", "role1"); + engine.addGroup("c", "g1"); + engine.addGroupRole("g1", "role2"); + engine.addUser("c", "pass5"); + assertThat(Arrays.asList(p.get("c").split(",")), + contains("pass5", "role1", PropertiesBackingEngine.GROUP_PREFIX + "g1")); + UserPrincipal upc = getUser(engine, "c"); + assertThat(names(engine.listRoles(upc)), containsInAnyOrder("role1", "role2")); + assertThat(names(engine.listGroups(upc)), containsInAnyOrder("g1")); + + // update empty password when user has roles + engine.addUser("d", ""); + engine.addRole("d", "role3"); + engine.addGroup("d", "g2"); + engine.addGroupRole("g2", "role4"); + engine.addUser("d", "pass6"); + assertThat(Arrays.asList(p.get("d").split(",")), + contains("pass6", "role3", PropertiesBackingEngine.GROUP_PREFIX + "g2")); + UserPrincipal upd = getUser(engine, "d"); + assertThat(names(engine.listRoles(upd)), containsInAnyOrder("role3", "role4")); + assertThat(names(engine.listGroups(upd)), containsInAnyOrder("g2")); + } + @After public void cleanup() { if (!f.delete()) { diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java index 9d43fbaa008..b02dce858d9 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java @@ -110,8 +110,11 @@ public void testLoginWithGroups() throws Exception { pbe.addUser("abc", "xyz"); pbe.addRole("abc", "myrole"); pbe.addUser("pqr", "abc"); + pbe.addRole("pqr", ""); // should be ignored pbe.addGroup("pqr", "group1"); pbe.addGroupRole("group1", "r1"); + pbe.addGroupRole("group1", ""); // should be ignored + pbe.addGroupRole("group1", "r2"); PropertiesLoginModule module = new PropertiesLoginModule(); Map options = new HashMap<>(); @@ -123,10 +126,10 @@ public void testLoginWithGroups() throws Exception { Assert.assertTrue(module.login()); Assert.assertTrue(module.commit()); - Assert.assertEquals(3, subject.getPrincipals().size()); + Assert.assertEquals(4, subject.getPrincipals().size()); assertThat(names(subject.getPrincipals(UserPrincipal.class)), containsInAnyOrder("pqr")); assertThat(names(subject.getPrincipals(GroupPrincipal.class)), containsInAnyOrder("group1")); - assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1")); + assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1", "r2")); } finally { if (!f.delete()) { Assert.fail("Could not delete temporary file: " + f); diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java index a0caf13b9ed..7bcfdec5e47 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java @@ -15,12 +15,9 @@ package org.apache.karaf.jaas.modules.publickey; import static org.apache.karaf.jaas.modules.PrincipalHelper.names; +import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.isIn; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertThat; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.junit.Assert.*; import java.io.File; import java.io.IOException; @@ -48,25 +45,27 @@ public class PublicKeyLoginModuleTest { private static final String PK_PROPERTIES_FILE = "org/apache/karaf/jaas/modules/publickey/pubkey.properties"; + private static final String PK_USERS_FILE = "org/apache/karaf/jaas/modules/publickey/pubkey.users"; - @Test - public void testRSALogin() throws Exception { - Properties options = getLoginModuleOptions(); - PublickeyLoginModule module = new PublickeyLoginModule(); - Subject subject = new Subject(); - - String knownModulus = "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" + private static final String RSA_KNOWN_MODULUS = + "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" + "979130785790337621243914845149325143098632580183245971502051291613503136182182218708721890923769091345704" + "119963221758691543226829294312457492456071842409242817598014777158790065648435489978774648853589909638928" + "448069481622573966178879417253888452317622624006445863588961367514293886664167742695648199055900918338245" + "701727653606086096756173044470526840851957391900922886984556493506186438991284463663361749451775578708454" + "0181594148839238901052763862484299588887844606103377160953183624788815045644521767391398467190125279747"; + private static final BigInteger RSA_EXPONENT = new BigInteger("65537"); + + @Test + public void testRSALogin() throws Exception { + Properties options = getLoginModuleOptions(); + PublickeyLoginModule module = new PublickeyLoginModule(); + Subject subject = new Subject(); // Generate a PublicKey using the known values - BigInteger modulus = new BigInteger(knownModulus); - BigInteger exponent = new BigInteger("65537"); + BigInteger modulus = new BigInteger(RSA_KNOWN_MODULUS); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); - KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); module.initialize(subject, new NamePubkeyCallbackHandler("rsa", publicKey), null, options); @@ -77,12 +76,43 @@ public void testRSALogin() throws Exception { assertFalse(subject.getPrincipals().isEmpty()); assertThat("rsa", isIn(names(subject.getPrincipals(UserPrincipal.class)))); - assertThat("ssh", isIn(names(subject.getPrincipals(RolePrincipal.class)))); assertTrue(module.logout()); assertEquals("Principals should be gone as the user has logged out", 0, subject.getPrincipals().size()); } + @Test + public void testRSALoginWithGroups() throws Exception { + // add groups + Properties users = loadFile(PK_USERS_FILE); + PublickeyBackingEngine pbe = new PublickeyBackingEngine(users); + pbe.addRole("rsa", "r1"); + pbe.addGroup("rsa", "group1"); + pbe.addRole("rsa", ""); // should be ignored + pbe.addGroupRole("group1", "r2"); + pbe.addGroupRole("group1", ""); // should be ignored + pbe.addGroupRole("group1", "r3"); + + PublickeyLoginModule module = new PublickeyLoginModule(); + Subject subject = new Subject(); + + // generate a PublicKey using the known values + BigInteger modulus = new BigInteger(RSA_KNOWN_MODULUS); + KeyFactory keyFactory = KeyFactory.getInstance("RSA"); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); + PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); + + module.initialize(subject, new NamePubkeyCallbackHandler("rsa", publicKey), null, getLoginModuleOptions()); + + assertEquals("Precondition", 0, subject.getPrincipals().size()); + assertTrue(module.login()); + assertTrue(module.commit()); + + assertEquals(5, subject.getPrincipals().size()); + assertThat("rsa", isIn(names(subject.getPrincipals(UserPrincipal.class)))); + assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1", "r2", "r3")); + } + @Test public void testDSALogin() throws Exception { Properties options = getLoginModuleOptions(); @@ -157,18 +187,10 @@ public void testUnknownUser() throws Exception { PublickeyLoginModule module = new PublickeyLoginModule(); Subject subject = new Subject(); - String knownModulus = "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" - + "979130785790337621243914845149325143098632580183245971502051291613503136182182218708721890923769091345704" - + "119963221758691543226829294312457492456071842409242817598014777158790065648435489978774648853589909638928" - + "448069481622573966178879417253888452317622624006445863588961367514293886664167742695648199055900918338245" - + "701727653606086096756173044470526840851957391900922886984556493506186438991284463663361749451775578708454" - + "0181594148839238901052763862484299588887844606103377160953183624788815045644521767391398467190125279747"; - // Generate a PublicKey using the known values - BigInteger modulus = new BigInteger(knownModulus); - BigInteger exponent = new BigInteger("65537"); + BigInteger modulus = new BigInteger(RSA_KNOWN_MODULUS); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); - KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); module.initialize(subject, new NamePubkeyCallbackHandler("unknown", publicKey), null, options); @@ -188,18 +210,15 @@ public void testUnknownKeyRSA() throws Exception { PublickeyLoginModule module = new PublickeyLoginModule(); Subject subject = new Subject(); - String knownModulus = "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" - + "979130785790337621243914845149325143098632580183245971502051291613503136182182218708721890923769091345704" - + "119963221758691543226829294312457492456071842409242817598014777158790065648435489978774648853589909638928" - + "448069481622573966178879417253888452317622624006445863588961367514293886664167742695648199055900918338245" - + "701727653606086096756173044470526840851957391900922886984556493506186438991284463663361749451775578708454" - + "0181594148839238901052763862484299588887844606103377160953183624788815045644521767391398467190125279745"; - // Generate a PublicKey using the known values - BigInteger modulus = new BigInteger(knownModulus); - BigInteger exponent = new BigInteger("65537"); + String anotherKeyKnownModulus = RSA_KNOWN_MODULUS.substring(0, RSA_KNOWN_MODULUS.length() - 1) + "3"; + assertEquals(anotherKeyKnownModulus.length(), RSA_KNOWN_MODULUS.length()); + assertNotEquals(anotherKeyKnownModulus.charAt(RSA_KNOWN_MODULUS.length() - 1), + RSA_KNOWN_MODULUS.charAt(RSA_KNOWN_MODULUS.length() - 1)); + + BigInteger modulus = new BigInteger(anotherKeyKnownModulus); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); - KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); module.initialize(subject, new NamePubkeyCallbackHandler("rsa", publicKey), null, options); @@ -274,14 +293,16 @@ public void testUnknownKeyEC() throws Exception { } } - protected Properties getLoginModuleOptions() throws IOException { + return loadFile(PK_PROPERTIES_FILE); + } + + private Properties loadFile(String name) throws IOException { String basedir = System.getProperty("basedir"); if (basedir == null) { basedir = new File(".").getCanonicalPath(); } - File file = new File(basedir + "/target/test-classes/" + PK_PROPERTIES_FILE); + File file = new File(basedir + "/target/test-classes/" + name); return new Properties(file); } - -} +} \ No newline at end of file diff --git a/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users b/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users index 3ea58e464ee..3fcde0accfc 100644 --- a/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users +++ b/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users @@ -17,7 +17,7 @@ # ################################################################################ -rsa=AAAAB3NzaC1yc2EAAAADAQABAAABAQDGX4CpCL49sWHaIuDE4VbGkdTMhsDLV3b8MDZ37Llsx3kRBs/x7G3OhSvQPhIjMNcbnUnCr+6O6poKjRcFI1Aj76TiSSYlvz9QbsWqc50ZwCuR39h6F9u8f9k62AV7IVA4aNVSJBFn2nOA00HOWvDDrU3ykG0cPeJcmP1lPeOO9WJVG7dc37v3soZZniIH+uop/UFQ4Ga0zWy4xjggAy2rE2p0BYHchrJb43ovInh5cGgXx2vNVwURsAf0TAPJwn7GLNpMYr3IFbRC3Tbe1wPdy9YM4rFlKL78o/dFbvUOH+Vd1BlYDofoxT4kHxod7W5wPALBr/Bm8CD2tR6OLLoD,_g_:admingroup +rsa=AAAAB3NzaC1yc2EAAAADAQABAAABAQDGX4CpCL49sWHaIuDE4VbGkdTMhsDLV3b8MDZ37Llsx3kRBs/x7G3OhSvQPhIjMNcbnUnCr+6O6poKjRcFI1Aj76TiSSYlvz9QbsWqc50ZwCuR39h6F9u8f9k62AV7IVA4aNVSJBFn2nOA00HOWvDDrU3ykG0cPeJcmP1lPeOO9WJVG7dc37v3soZZniIH+uop/UFQ4Ga0zWy4xjggAy2rE2p0BYHchrJb43ovInh5cGgXx2vNVwURsAf0TAPJwn7GLNpMYr3IFbRC3Tbe1wPdy9YM4rFlKL78o/dFbvUOH+Vd1BlYDofoxT4kHxod7W5wPALBr/Bm8CD2tR6OLLoD dsa=AAAAB3NzaC1kc3MAAACBAJlAn/bPWpugKCLyoQpe8AbSZiIxdEJhl+VV8YEH6jfb9lLPA9JkQAf/lnG1Jx01UM65RRyKtnMAiBpkhrPy3DbqJ4FgYBmc1Sdiufomilq6zSbE0esJEMyxEvSNDQLqIiUcSwVyJJj1vpV6ZPA6ihipTIaiSV+rmfKcS05i27UlAAAAFQCg3ZtIytPmGILQ7OEifIJvCSlS5QAAAIBUbgpjk7vSWVNICgKG6OrXeK0kJYRG6AaUZSiB2neoABMyGIHQ8dBCk+jtYqRMYyoc+OPi5q43VcDMxgzR/cHGjZi60w/I3M83072dAdaoi0cleL/V8NaH+SOvkkYkAG57OIa3ly9PVpPfeXRnbbjkz1EsrvXIelqb5enLhlIgXgAAAIA11rUkN/J3K7nw/BiolhpZR3MVhWWIJFjJyU7ZC0yO8a+3AExuhTI6YQvsyvlY69KCwAwZsZvx9DryDE5xTfhzYa5kV4mM4AJSrE8/GtxLUVPZLwV6eoZLv1RIqP543ihZtoFyVmMaTQFj45Qo8uAuVDjx5mpk/Rk1pYPUd0lc1Q== ec=AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBL4+Vytknywh/XuOluxIqcHRoBsZHa12z+jpKpwuGFlzlq3yatwC8DqUaywJjzSnoGKSge9GBjuFYwvHN17hq8U=,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh From ea7e507099947bebc821456c89e74f9562e5e53f Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Thu, 1 May 2025 11:33:16 +0200 Subject: [PATCH 2/6] KARAF-5014: consider first group role in users.properties and ignore empty roles - unit tests --- .../AbstractPropertiesBackingEngine.java | 2 +- .../AbstractPropertiesBackingEngineTest.java | 174 ++++++++++++++++++ .../karaf/jaas/modules/PrincipalHelper.java | 20 +- .../PropertiesBackingEngineTest.java | 82 ++------- .../publickey/PublicKeyLoginModuleTest.java | 101 ++++------ .../karaf/jaas/modules/publickey/pubkey.users | 2 +- 6 files changed, 248 insertions(+), 133 deletions(-) create mode 100644 jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java index 5eef6ca942d..7d3694f94d5 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java @@ -199,7 +199,7 @@ public void deleteRole(String username, String role) { } for (int i = firstRoleIndex; i < infos.length; i++) { if (infos[i] != null && !infos[i].equals(role)) { - if(userInfoBuffer.length() > 0) { + if(firstRoleIndex == 1 || userInfoBuffer.length() > 0) { userInfoBuffer.append(","); } userInfoBuffer.append(infos[i]); diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java new file mode 100644 index 00000000000..86c4033c839 --- /dev/null +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java @@ -0,0 +1,174 @@ +/* + * Licensed 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. + * under the License. + */ +package org.apache.karaf.jaas.modules; + +import static org.apache.karaf.jaas.modules.PrincipalHelper.names; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import org.apache.felix.utils.properties.Properties; +import org.apache.karaf.jaas.boot.principal.GroupPrincipal; +import org.apache.karaf.jaas.boot.principal.RolePrincipal; +import org.apache.karaf.jaas.boot.principal.UserPrincipal; +import org.apache.karaf.jaas.modules.properties.PropertiesBackingEngine; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import java.io.File; +import java.io.IOException; +import java.util.List; + +public class AbstractPropertiesBackingEngineTest { + + private File f; + private Properties p; + private PropertiesBackingEngine engine; + + @Before + public void start() throws IOException { + f = File.createTempFile(getClass().getName(), ".tmp"); + p = new Properties(f); + engine = new PropertiesBackingEngine(p); + } + + @Test + public void addUserInternalTest() { + // non-existing user + engine.addUserInternal("a", ""); + + // update empty password on existing user with no roles + engine.addUserInternal("a", "pass1"); + assertThat(List.of(p.get("a").split(",")), contains("pass1")); + UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); + assertTrue(engine.listGroups(upa).isEmpty()); + assertTrue(engine.listRoles(upa).isEmpty()); + + // update password on existing user with no roles + engine.addUserInternal("a", "pass2"); + assertThat(List.of(p.get("a").split(",")), contains("pass2")); + upa = PrincipalHelper.getUser(engine, "a"); + assertTrue(engine.listGroups(upa).isEmpty()); + assertTrue(engine.listRoles(upa).isEmpty()); + + // update password on existing user with roles + engine.addRole("a", "role1"); + engine.addGroup("a", "g1"); + engine.addGroupRole("g1", "role2"); + engine.addUserInternal("a", "pass3"); + assertThat(List.of(p.get("a").split(",")), + contains("pass3", "role1", PropertiesBackingEngine.GROUP_PREFIX + "g1")); + upa = PrincipalHelper.getUser(engine, "a"); + assertThat(names(engine.listGroups(upa)), containsInAnyOrder("g1")); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); + } + + @Test + public void lookupUserTest() { + engine.addUser("a", "pass1"); + engine.addGroup("a", "g1"); + + assertEquals("a", engine.lookupUser("a").getName()); + assertNull(engine.lookupUser("g1")); + assertNull(engine.lookupUser("test")); + } + + @Test + public void listRolesTest() { + // empty roles in groups + p.put(PropertiesBackingEngine.GROUP_PREFIX + "g1", ",,,"); // simulate manual editing of the properties file + GroupPrincipal gpg1 = PrincipalHelper.getGroup(engine, "g1"); + assertTrue(engine.listRoles(gpg1).isEmpty()); + + // empty roles in users + p.put("a", "pass1,,,"); + UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); + assertTrue(engine.listRoles(upa).isEmpty()); + + // duplicate role + p.put("a", "pass1,role1,role1"); + upa = PrincipalHelper.getUser(engine, "a"); + List roles = engine.listRoles(upa); + assertEquals(1, roles.size()); + assertEquals("role1", roles.get(0).getName()); + } + + @Test + public void addRoleTest() { + // empty info in groups + engine.createGroup("g1"); + engine.addGroupRole("g1", "role1"); + GroupPrincipal gpg1 = PrincipalHelper.getGroup(engine, "g1"); + assertThat(names(engine.listRoles(gpg1)), contains("role1")); + + // empty info in users + engine.addUser("a", ""); + engine.addRole("a", "role2"); + UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); + assertThat(names(engine.listRoles(upa)), contains("role2")); + // user empty password is preserved + assertThat(List.of(p.get("a").split(",")), containsInAnyOrder("", "role2")); + + // duplicate role + engine.addRole("a", "role2"); + upa = PrincipalHelper.getUser(engine, "a"); + assertThat(names(engine.listRoles(upa)), contains("role2")); + assertThat(List.of(p.get("a").split(",")), containsInAnyOrder("", "role2")); + + // duplicate group + engine.addUser("b", ""); + engine.addGroup("b", "g2"); + engine.addGroup("b", "g2"); + UserPrincipal upb = PrincipalHelper.getUser(engine, "b"); + assertThat(names(engine.listGroups(upb)), contains("g2")); + assertThat(List.of(p.get("b").split(",")), + containsInAnyOrder("", PropertiesBackingEngine.GROUP_PREFIX + "g2")); + } + + @Test + public void deleteRoleTest() { + // delete in group + engine.createGroup("g1"); + engine.addGroupRole("g1", "role1"); + engine.addGroupRole("g1", "role2"); + engine.addGroupRole("g1", "role3"); + engine.deleteGroupRole("g1", "role1"); + GroupPrincipal gpg1 = PrincipalHelper.getGroup(engine, "g1"); + assertThat(names(engine.listRoles(gpg1)), containsInAnyOrder("role2", "role3")); + + // delete in user + engine.addUser("a", ""); + engine.addRole("a", "role4"); + engine.addRole("a", "role5"); + engine.addRole("a", "role6"); + engine.deleteRole("a", "role4"); + UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); + assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role5", "role6")); + // user empty password is preserved + assertThat(List.of(p.get("a").split(",")), containsInAnyOrder("", "role5", "role6")); + } + + @After + public void cleanup() { + if (!f.delete()) { + fail("Could not delete temporary file: " + f); + } + } + +} \ No newline at end of file diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/PrincipalHelper.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/PrincipalHelper.java index 313fdff010e..68b7bbc78c9 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/PrincipalHelper.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/PrincipalHelper.java @@ -16,14 +16,32 @@ import static java.util.stream.Collectors.toList; +import org.apache.karaf.jaas.boot.principal.GroupPrincipal; +import org.apache.karaf.jaas.boot.principal.UserPrincipal; +import org.junit.Assert; import java.security.Principal; import java.util.Collection; import java.util.List; +import java.util.stream.Collectors; public class PrincipalHelper { public static List names(Collection principals) { return principals.stream().map(Principal::getName).collect(toList()); } + + public static UserPrincipal getUser(AbstractPropertiesBackingEngine engine, String name) { + List matchingUsers = engine.listUsers().stream() + .filter(user -> name.equals(user.getName())).collect(Collectors.toList()); + Assert.assertFalse("User with name " + name + " was not found", matchingUsers.isEmpty()); + return matchingUsers.iterator().next(); + } + + public static GroupPrincipal getGroup(AbstractPropertiesBackingEngine engine, String name) { + List matchingGroups = engine.listGroups().keySet().stream() + .filter(group -> name.equals(group.getName())).collect(Collectors.toList()); + Assert.assertFalse("Group with name " + name + " was not found", matchingGroups.isEmpty()); + return matchingGroups.iterator().next(); + } -} +} \ No newline at end of file diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java index 1fe4b2d5318..d42148d6858 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java @@ -18,22 +18,17 @@ import static org.apache.karaf.jaas.modules.PrincipalHelper.names; import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.hamcrest.Matchers.contains; -import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; -import java.util.Arrays; -import java.util.List; -import java.util.stream.Collectors; import org.apache.felix.utils.properties.Properties; import org.apache.karaf.jaas.boot.principal.GroupPrincipal; import org.apache.karaf.jaas.boot.principal.UserPrincipal; +import org.apache.karaf.jaas.modules.PrincipalHelper; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -58,8 +53,8 @@ public void testUserRoles() throws IOException { engine.addRole("a", "role1"); engine.addRole("a", "role2"); - UserPrincipal upa = getUser(engine, "a"); - assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); + UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); + Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); engine.addGroup("a", "g"); engine.addGroupRole("g", "role2"); @@ -68,8 +63,8 @@ public void testUserRoles() throws IOException { engine.addGroup("b", "g2"); engine.addGroupRole("g2", "role4"); - assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b")); - assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); + Assert.assertThat(names(engine.listUsers()), containsInAnyOrder("a", "b")); + Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); checkLoading(); @@ -77,17 +72,17 @@ public void testUserRoles() throws IOException { assertEquals("a", engine.lookupUser("a").getName()); // removing some stuff - UserPrincipal upb = getUser(engine, "b"); + UserPrincipal upb = PrincipalHelper.getUser(engine, "b"); assertEquals(1, engine.listGroups(upa).size()); assertEquals(2, engine.listGroups(upb).size()); GroupPrincipal gp = engine.listGroups(upa).iterator().next(); engine.deleteGroupRole("g", "role2"); - assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3")); + Assert.assertThat(names(engine.listRoles(gp)), containsInAnyOrder("role3")); // role2 should still be there as it was added to the user directly too - assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); - assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4")); + Assert.assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2", "role3")); + Assert.assertThat(names(engine.listRoles(upb)), containsInAnyOrder("role3", "role4")); engine.deleteGroup("b", "g"); engine.deleteGroup("b", "g2"); @@ -101,65 +96,14 @@ public void testUserRoles() throws IOException { private void checkLoading() throws IOException { PropertiesBackingEngine engine = new PropertiesBackingEngine(new Properties(f)); assertEquals(2, engine.listUsers().size()); - UserPrincipal upa_2 = getUser(engine, "a"); - UserPrincipal upb_2 = getUser(engine, "b"); + UserPrincipal upa_2 = PrincipalHelper.getUser(engine, "a"); + UserPrincipal upb_2 = PrincipalHelper.getUser(engine, "b"); assertEquals(3, engine.listRoles(upa_2).size()); - assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3")); + Assert.assertThat(names(engine.listRoles(upa_2)), containsInAnyOrder("role1", "role2", "role3")); assertEquals(3, engine.listRoles(upb_2).size()); - assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4")); - } - - private UserPrincipal getUser(PropertiesBackingEngine engine, String name) { - List matchingUsers = engine.listUsers().stream() - .filter(user->name.equals(user.getName())).collect(Collectors.toList()); - Assert.assertFalse("User with name " + name + " was not found", matchingUsers.isEmpty()); - return matchingUsers.iterator().next(); - } - - @Test - public void testUserPassword() throws IOException { - Properties p = new Properties(f); - PropertiesBackingEngine engine = new PropertiesBackingEngine(p); - - // update password when user has no roles - engine.addUser("a", "pass1"); - engine.addUser("a", "pass2"); - assertThat(Arrays.asList(p.get("a").split(",")), contains("pass2")); - UserPrincipal upa = getUser(engine, "a"); - assertTrue(engine.listRoles(upa).isEmpty()); - - // update empty password when user has no roles - engine.addUser("b", ""); - engine.addUser("b", "pass3"); - assertThat(Arrays.asList(p.get("b").split(",")), contains("pass3")); - UserPrincipal upb = getUser(engine, "b"); - assertTrue(engine.listRoles(upb).isEmpty()); - - // update password when user has roles - engine.addUser("c", "pass4"); - engine.addRole("c", "role1"); - engine.addGroup("c", "g1"); - engine.addGroupRole("g1", "role2"); - engine.addUser("c", "pass5"); - assertThat(Arrays.asList(p.get("c").split(",")), - contains("pass5", "role1", PropertiesBackingEngine.GROUP_PREFIX + "g1")); - UserPrincipal upc = getUser(engine, "c"); - assertThat(names(engine.listRoles(upc)), containsInAnyOrder("role1", "role2")); - assertThat(names(engine.listGroups(upc)), containsInAnyOrder("g1")); - - // update empty password when user has roles - engine.addUser("d", ""); - engine.addRole("d", "role3"); - engine.addGroup("d", "g2"); - engine.addGroupRole("g2", "role4"); - engine.addUser("d", "pass6"); - assertThat(Arrays.asList(p.get("d").split(",")), - contains("pass6", "role3", PropertiesBackingEngine.GROUP_PREFIX + "g2")); - UserPrincipal upd = getUser(engine, "d"); - assertThat(names(engine.listRoles(upd)), containsInAnyOrder("role3", "role4")); - assertThat(names(engine.listGroups(upd)), containsInAnyOrder("g2")); + Assert.assertThat(names(engine.listRoles(upb_2)), containsInAnyOrder("role2", "role3", "role4")); } @After diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java index 7bcfdec5e47..a0caf13b9ed 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/publickey/PublicKeyLoginModuleTest.java @@ -15,9 +15,12 @@ package org.apache.karaf.jaas.modules.publickey; import static org.apache.karaf.jaas.modules.PrincipalHelper.names; -import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.isIn; -import static org.junit.Assert.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; @@ -45,16 +48,6 @@ public class PublicKeyLoginModuleTest { private static final String PK_PROPERTIES_FILE = "org/apache/karaf/jaas/modules/publickey/pubkey.properties"; - private static final String PK_USERS_FILE = "org/apache/karaf/jaas/modules/publickey/pubkey.users"; - - private static final String RSA_KNOWN_MODULUS = - "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" - + "979130785790337621243914845149325143098632580183245971502051291613503136182182218708721890923769091345704" - + "119963221758691543226829294312457492456071842409242817598014777158790065648435489978774648853589909638928" - + "448069481622573966178879417253888452317622624006445863588961367514293886664167742695648199055900918338245" - + "701727653606086096756173044470526840851957391900922886984556493506186438991284463663361749451775578708454" - + "0181594148839238901052763862484299588887844606103377160953183624788815045644521767391398467190125279747"; - private static final BigInteger RSA_EXPONENT = new BigInteger("65537"); @Test public void testRSALogin() throws Exception { @@ -62,10 +55,18 @@ public void testRSALogin() throws Exception { PublickeyLoginModule module = new PublickeyLoginModule(); Subject subject = new Subject(); + String knownModulus = "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" + + "979130785790337621243914845149325143098632580183245971502051291613503136182182218708721890923769091345704" + + "119963221758691543226829294312457492456071842409242817598014777158790065648435489978774648853589909638928" + + "448069481622573966178879417253888452317622624006445863588961367514293886664167742695648199055900918338245" + + "701727653606086096756173044470526840851957391900922886984556493506186438991284463663361749451775578708454" + + "0181594148839238901052763862484299588887844606103377160953183624788815045644521767391398467190125279747"; + // Generate a PublicKey using the known values - BigInteger modulus = new BigInteger(RSA_KNOWN_MODULUS); + BigInteger modulus = new BigInteger(knownModulus); + BigInteger exponent = new BigInteger("65537"); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); - KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); module.initialize(subject, new NamePubkeyCallbackHandler("rsa", publicKey), null, options); @@ -76,43 +77,12 @@ public void testRSALogin() throws Exception { assertFalse(subject.getPrincipals().isEmpty()); assertThat("rsa", isIn(names(subject.getPrincipals(UserPrincipal.class)))); + assertThat("ssh", isIn(names(subject.getPrincipals(RolePrincipal.class)))); assertTrue(module.logout()); assertEquals("Principals should be gone as the user has logged out", 0, subject.getPrincipals().size()); } - @Test - public void testRSALoginWithGroups() throws Exception { - // add groups - Properties users = loadFile(PK_USERS_FILE); - PublickeyBackingEngine pbe = new PublickeyBackingEngine(users); - pbe.addRole("rsa", "r1"); - pbe.addGroup("rsa", "group1"); - pbe.addRole("rsa", ""); // should be ignored - pbe.addGroupRole("group1", "r2"); - pbe.addGroupRole("group1", ""); // should be ignored - pbe.addGroupRole("group1", "r3"); - - PublickeyLoginModule module = new PublickeyLoginModule(); - Subject subject = new Subject(); - - // generate a PublicKey using the known values - BigInteger modulus = new BigInteger(RSA_KNOWN_MODULUS); - KeyFactory keyFactory = KeyFactory.getInstance("RSA"); - KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); - PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); - - module.initialize(subject, new NamePubkeyCallbackHandler("rsa", publicKey), null, getLoginModuleOptions()); - - assertEquals("Precondition", 0, subject.getPrincipals().size()); - assertTrue(module.login()); - assertTrue(module.commit()); - - assertEquals(5, subject.getPrincipals().size()); - assertThat("rsa", isIn(names(subject.getPrincipals(UserPrincipal.class)))); - assertThat(names(subject.getPrincipals(RolePrincipal.class)), containsInAnyOrder("r1", "r2", "r3")); - } - @Test public void testDSALogin() throws Exception { Properties options = getLoginModuleOptions(); @@ -187,10 +157,18 @@ public void testUnknownUser() throws Exception { PublickeyLoginModule module = new PublickeyLoginModule(); Subject subject = new Subject(); + String knownModulus = "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" + + "979130785790337621243914845149325143098632580183245971502051291613503136182182218708721890923769091345704" + + "119963221758691543226829294312457492456071842409242817598014777158790065648435489978774648853589909638928" + + "448069481622573966178879417253888452317622624006445863588961367514293886664167742695648199055900918338245" + + "701727653606086096756173044470526840851957391900922886984556493506186438991284463663361749451775578708454" + + "0181594148839238901052763862484299588887844606103377160953183624788815045644521767391398467190125279747"; + // Generate a PublicKey using the known values - BigInteger modulus = new BigInteger(RSA_KNOWN_MODULUS); + BigInteger modulus = new BigInteger(knownModulus); + BigInteger exponent = new BigInteger("65537"); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); - KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); module.initialize(subject, new NamePubkeyCallbackHandler("unknown", publicKey), null, options); @@ -210,15 +188,18 @@ public void testUnknownKeyRSA() throws Exception { PublickeyLoginModule module = new PublickeyLoginModule(); Subject subject = new Subject(); - // Generate a PublicKey using the known values - String anotherKeyKnownModulus = RSA_KNOWN_MODULUS.substring(0, RSA_KNOWN_MODULUS.length() - 1) + "3"; - assertEquals(anotherKeyKnownModulus.length(), RSA_KNOWN_MODULUS.length()); - assertNotEquals(anotherKeyKnownModulus.charAt(RSA_KNOWN_MODULUS.length() - 1), - RSA_KNOWN_MODULUS.charAt(RSA_KNOWN_MODULUS.length() - 1)); + String knownModulus = "2504227846033126752625313329217708474924890377669312098933267135871562327792150810915433595733" + + "979130785790337621243914845149325143098632580183245971502051291613503136182182218708721890923769091345704" + + "119963221758691543226829294312457492456071842409242817598014777158790065648435489978774648853589909638928" + + "448069481622573966178879417253888452317622624006445863588961367514293886664167742695648199055900918338245" + + "701727653606086096756173044470526840851957391900922886984556493506186438991284463663361749451775578708454" + + "0181594148839238901052763862484299588887844606103377160953183624788815045644521767391398467190125279745"; - BigInteger modulus = new BigInteger(anotherKeyKnownModulus); + // Generate a PublicKey using the known values + BigInteger modulus = new BigInteger(knownModulus); + BigInteger exponent = new BigInteger("65537"); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); - KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, RSA_EXPONENT); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); PublicKey publicKey = keyFactory.generatePublic(publicKeySpec); module.initialize(subject, new NamePubkeyCallbackHandler("rsa", publicKey), null, options); @@ -293,16 +274,14 @@ public void testUnknownKeyEC() throws Exception { } } - protected Properties getLoginModuleOptions() throws IOException { - return loadFile(PK_PROPERTIES_FILE); - } - private Properties loadFile(String name) throws IOException { + protected Properties getLoginModuleOptions() throws IOException { String basedir = System.getProperty("basedir"); if (basedir == null) { basedir = new File(".").getCanonicalPath(); } - File file = new File(basedir + "/target/test-classes/" + name); + File file = new File(basedir + "/target/test-classes/" + PK_PROPERTIES_FILE); return new Properties(file); } -} \ No newline at end of file + +} diff --git a/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users b/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users index 3fcde0accfc..9f45ca53236 100644 --- a/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users +++ b/jaas/modules/src/test/resources/org/apache/karaf/jaas/modules/publickey/pubkey.users @@ -17,7 +17,7 @@ # ################################################################################ -rsa=AAAAB3NzaC1yc2EAAAADAQABAAABAQDGX4CpCL49sWHaIuDE4VbGkdTMhsDLV3b8MDZ37Llsx3kRBs/x7G3OhSvQPhIjMNcbnUnCr+6O6poKjRcFI1Aj76TiSSYlvz9QbsWqc50ZwCuR39h6F9u8f9k62AV7IVA4aNVSJBFn2nOA00HOWvDDrU3ykG0cPeJcmP1lPeOO9WJVG7dc37v3soZZniIH+uop/UFQ4Ga0zWy4xjggAy2rE2p0BYHchrJb43ovInh5cGgXx2vNVwURsAf0TAPJwn7GLNpMYr3IFbRC3Tbe1wPdy9YM4rFlKL78o/dFbvUOH+Vd1BlYDofoxT4kHxod7W5wPALBr/Bm8CD2tR6OLLoD +rsa=AAAAB3NzaC1yc2EAAAADAQABAAABAQDGX4CpCL49sWHaIuDE4VbGkdTMhsDLV3b8MDZ37Llsx3kRBs/x7G3OhSvQPhIjMNcbnUnCr+6O6poKjRcFI1Aj76TiSSYlvz9QbsWqc50ZwCuR39h6F9u8f9k62AV7IVA4aNVSJBFn2nOA00HOWvDDrU3ykG0cPeJcmP1lPeOO9WJVG7dc37v3soZZniIH+uop/UFQ4Ga0zWy4xjggAy2rE2p0BYHchrJb43ovInh5cGgXx2vNVwURsAf0TAPJwn7GLNpMYr3IFbRC3Tbe1wPdy9YM4rFlKL78o/dFbvUOH+Vd1BlYDofoxT4kHxod7W5wPALBr/Bm8CD2tR6OLLoD,_g_:admingroup dsa=AAAAB3NzaC1kc3MAAACBAJlAn/bPWpugKCLyoQpe8AbSZiIxdEJhl+VV8YEH6jfb9lLPA9JkQAf/lnG1Jx01UM65RRyKtnMAiBpkhrPy3DbqJ4FgYBmc1Sdiufomilq6zSbE0esJEMyxEvSNDQLqIiUcSwVyJJj1vpV6ZPA6ihipTIaiSV+rmfKcS05i27UlAAAAFQCg3ZtIytPmGILQ7OEifIJvCSlS5QAAAIBUbgpjk7vSWVNICgKG6OrXeK0kJYRG6AaUZSiB2neoABMyGIHQ8dBCk+jtYqRMYyoc+OPi5q43VcDMxgzR/cHGjZi60w/I3M83072dAdaoi0cleL/V8NaH+SOvkkYkAG57OIa3ly9PVpPfeXRnbbjkz1EsrvXIelqb5enLhlIgXgAAAIA11rUkN/J3K7nw/BiolhpZR3MVhWWIJFjJyU7ZC0yO8a+3AExuhTI6YQvsyvlY69KCwAwZsZvx9DryDE5xTfhzYa5kV4mM4AJSrE8/GtxLUVPZLwV6eoZLv1RIqP543ihZtoFyVmMaTQFj45Qo8uAuVDjx5mpk/Rk1pYPUd0lc1Q== ec=AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBL4+Vytknywh/XuOluxIqcHRoBsZHa12z+jpKpwuGFlzlq3yatwC8DqUaywJjzSnoGKSge9GBjuFYwvHN17hq8U=,_g_:admingroup _g_\:admingroup = admin,manager,viewer,systembundles,ssh From ced63fe2df207510a20b52389beccf3d8f53b3ca Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Thu, 1 May 2025 11:51:34 +0200 Subject: [PATCH 3/6] KARAF-5014: consider first group role in users.properties and ignore empty roles - update .adoc files --- manual/src/main/asciidoc/user-guide/monitoring.adoc | 2 +- manual/src/main/asciidoc/user-guide/remote.adoc | 2 +- manual/src/main/asciidoc/user-guide/security.adoc | 8 ++++---- manual/src/main/asciidoc/user-guide/webconsole.adoc | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/manual/src/main/asciidoc/user-guide/monitoring.adoc b/manual/src/main/asciidoc/user-guide/monitoring.adoc index 019825d4a5a..d92e046d153 100644 --- a/manual/src/main/asciidoc/user-guide/monitoring.adoc +++ b/manual/src/main/asciidoc/user-guide/monitoring.adoc @@ -44,7 +44,7 @@ For security reason, by default, `karaf` user is disabled. To allow the logon, y ---- karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh ---- ==== diff --git a/manual/src/main/asciidoc/user-guide/remote.adoc b/manual/src/main/asciidoc/user-guide/remote.adoc index ebcf732b34c..f23c4580b28 100644 --- a/manual/src/main/asciidoc/user-guide/remote.adoc +++ b/manual/src/main/asciidoc/user-guide/remote.adoc @@ -26,7 +26,7 @@ For security reason, by default, `karaf` user is disabled. To allow the logon, y ---- karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh ---- ==== diff --git a/manual/src/main/asciidoc/user-guide/security.adoc b/manual/src/main/asciidoc/user-guide/security.adoc index a3365778f27..40c8b5e303b 100644 --- a/manual/src/main/asciidoc/user-guide/security.adoc +++ b/manual/src/main/asciidoc/user-guide/security.adoc @@ -35,7 +35,7 @@ For security reason, by default, `karaf` user is disabled. To allow the logon, y ---- karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh ---- ==== @@ -139,7 +139,7 @@ The initial `etc/users.properties` file contains: # with the name "karaf". # karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,ssh +_g_\:admingroup = admin,manager,viewer,ssh ---- We can see in this file, that we have one user by default: `karaf`. @@ -149,7 +149,7 @@ The `karaf` user is member of one group: the `admingroup`. A group is always prefixed by `_g_:`. An entry without this prefix is a user. -A group defines a set of roles. By default, the `admingroup` defines `group`, `admin`, `manager`, and `viewer` +A group defines a set of roles. By default, the `admingroup` defines `admin`, `manager`, and `viewer` roles. It means that the `karaf` user will have the roles defined by the `admingroup`. @@ -420,7 +420,7 @@ You copy the key from `karaf.id_rsa.pub` file in the `etc/keys.properties`: ---- karaf=AAAAB3NzaC1yc2EAAAADAQABAAABAQCtXN9ZZ+K67UFbxZMxHsoR69vHNbN8qi17v/5jF83FUexRqu8FvWCInBoW7eVFyeIiFXGd/zaCrDHrZsqpwXNEha3ifvfGTY2+gMLfZZFgh2LFubXBH6G725XKs9aus+KLrwC8u/uPr9Sw3YeSb0zxrjiXqv6hGYhaAHnskAgRCUxa+P4/JYNVS/2+ZrvBrVMAwgEuwt1Y1IDYsXQRmLJPn5ayMCfzPTANXfgB7Hix 72f2XpHV3FdnKTbYwdA32Bg4ptJkuvyMXnBy5y7ChRU150YGRToC4ETcPF2DB0EPOcbOLsQlNTKKRYuNR1zEpp6RAfiWD65kmYK766CE8AbB,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh ---- and specify to the client to use the `karaf.id_rsa` private key: diff --git a/manual/src/main/asciidoc/user-guide/webconsole.adoc b/manual/src/main/asciidoc/user-guide/webconsole.adoc index 4df6652f901..0ff53369495 100644 --- a/manual/src/main/asciidoc/user-guide/webconsole.adoc +++ b/manual/src/main/asciidoc/user-guide/webconsole.adoc @@ -64,7 +64,7 @@ For security reason, by default, `karaf` user is disabled. To allow the logon, y ---- karaf = karaf,_g_:admingroup -_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh +_g_\:admingroup = admin,manager,viewer,systembundles,ssh ---- ==== From 93ecc454e2c283ec05e15c661bfbf3b5eb2092b8 Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Thu, 1 May 2025 13:49:48 +0200 Subject: [PATCH 4/6] KARAF-5014: consider empty groups in the output of jaas:user-list --- .../org/apache/karaf/jaas/command/ListUsersCommand.java | 7 +++++-- .../jaas/modules/AbstractPropertiesBackingEngine.java | 1 + 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/jaas/command/src/main/java/org/apache/karaf/jaas/command/ListUsersCommand.java b/jaas/command/src/main/java/org/apache/karaf/jaas/command/ListUsersCommand.java index ecf158894a1..4a4f28c5b3f 100644 --- a/jaas/command/src/main/java/org/apache/karaf/jaas/command/ListUsersCommand.java +++ b/jaas/command/src/main/java/org/apache/karaf/jaas/command/ListUsersCommand.java @@ -70,7 +70,8 @@ protected Object doExecute(BackingEngine engine) throws Exception { List reportedRoles = new ArrayList<>(); String userName = user.getName(); - for (GroupPrincipal group : engine.listGroups(user)) { + List groups = engine.listGroups(user); + for (GroupPrincipal group : groups) { reportedRoles.addAll(displayGroupRoles(engine, userName, group, table)); } @@ -83,7 +84,7 @@ protected Object doExecute(BackingEngine engine) throws Exception { table.addRow().addContent(userName, "", roleName); } - if (reportedRoles.size() == 0) { + if (reportedRoles.size() == 0 && groups.size() == 0) { table.addRow().addContent(userName, "", ""); } @@ -104,6 +105,8 @@ private List displayGroupRoles(BackingEngine engine, String userName, Gr names.add(roleName); table.addRow().addContent(userName, group.getName(), roleName); } + } else { + table.addRow().addContent(userName, group.getName(), ""); } return names; } diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java index 7d3694f94d5..e0d8c61f4f4 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java @@ -274,6 +274,7 @@ public void deleteGroupRole(String group, String role) { deleteRole(GROUP_PREFIX + group, role); } + @Override public Map listGroups() { Map result = new HashMap<>(); for (String name : users.keySet()) { From 67f820016e55e816e066769718832b175967f9b1 Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Fri, 2 May 2025 09:24:20 +0200 Subject: [PATCH 5/6] KARAF-5014: align properties login modules logic for retrieving groups and roles with the underlying baking engine --- .../AbstractPropertiesBackingEngine.java | 24 +++++++-------- .../apache/karaf/jaas/modules/JAASUtils.java | 29 ++++--------------- .../properties/DigestPasswordLoginModule.java | 2 +- .../properties/PropertiesLoginModule.java | 2 +- .../publickey/PublickeyLoginModule.java | 2 +- 5 files changed, 21 insertions(+), 38 deletions(-) diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java index e0d8c61f4f4..b86f94fecd6 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java @@ -68,7 +68,7 @@ protected void addUserInternal(String username, String password) { @Override public void deleteUser(String username) { // delete all its groups first, for garbage collection of the groups - for (GroupPrincipal gp : listGroups(username)) { + for (GroupPrincipal gp : listGroups(users, username)) { deleteGroup(username, gp.getName()); } @@ -112,20 +112,20 @@ public List listRoles(Principal principal) { if (principal instanceof GroupPrincipal) { userName = GROUP_PREFIX + userName; } - return listRoles(userName); + return listRoles(users, userName); } - protected List listRoles(String name) { + public static List listRoles(Properties users, String name) { List result = new ArrayList<>(); String userInfo = users.get(name); String[] infos = userInfo.split(","); for (int i = getFirstRoleIndex(name); i < infos.length; i++) { - String roleName = infos[i]; - if (roleName.trim().isEmpty()) { + String roleName = infos[i].trim(); + if (roleName.isEmpty()) { // ignore } else if (roleName.startsWith(GROUP_PREFIX)) { - for (RolePrincipal rp : listRoles(roleName)) { + for (RolePrincipal rp : listRoles(users, roleName)) { if (!result.contains(rp)) { result.add(rp); } @@ -145,7 +145,7 @@ protected List listRoles(String name) { * @param name the property to evaluate, can represent either a group or a user * @return 0 if the property starts with the group prefix, otherwise 1 */ - private int getFirstRoleIndex(String name) { + private static int getFirstRoleIndex(String name) { if (name.trim().startsWith(GROUP_PREFIX)) return 0; return 1; @@ -160,12 +160,12 @@ public void addRole(String username, String role) { if (userInfos.trim().isEmpty() && username.trim().startsWith(GROUP_PREFIX)) { users.put(username, role); } else { - for (RolePrincipal rp : listRoles(username)) { + for (RolePrincipal rp : listRoles(users, username)) { if (role.equals(rp.getName())) { return; } } - for (GroupPrincipal gp : listGroups(username)) { + for (GroupPrincipal gp : listGroups(users, username)) { if (role.equals(GROUP_PREFIX + gp.getName())) { return; } @@ -219,16 +219,16 @@ public void deleteRole(String username, String role) { @Override public List listGroups(UserPrincipal user) { String userName = user.getName(); - return listGroups(userName); + return listGroups(users, userName); } - private List listGroups(String userName) { + public static List listGroups(Properties users, String userName) { List result = new ArrayList<>(); String userInfo = users.get(userName); if (userInfo != null) { String[] infos = userInfo.split(","); for (int i = getFirstRoleIndex(userName); i < infos.length; i++) { - String name = infos[i]; + String name = infos[i].trim(); if (name.startsWith(GROUP_PREFIX)) { result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length()))); } diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java index 1659783ae84..1ee15995f42 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java @@ -37,32 +37,15 @@ public static String getString(Map options, String key) { return (String)val; } - public static Set getPrincipals(String user, Properties users, String[] infos) { + public static Set getPrincipals(String user, Properties users) { Set principals = new HashSet<>(); principals.add(new UserPrincipal(user)); - for (int i = 1; i < infos.length; i++) { - if (infos[i].trim().startsWith(BackingEngine.GROUP_PREFIX)) { - // it's a group reference - principals.add(new GroupPrincipal(infos[i].trim().substring(BackingEngine.GROUP_PREFIX.length()))); - String groupInfo = users.get(infos[i].trim()); - if (groupInfo != null) { - String[] roles = groupInfo.split(","); - for (int j = 0; j < roles.length; j++) { - addRole(principals, roles[j]); - } - } - } else { - // it's an user reference - addRole(principals, infos[i]); - } - } - return principals; - } + AbstractPropertiesBackingEngine.listGroups(users, user) + .forEach(group -> principals.add(new GroupPrincipal(group.getName()))); + AbstractPropertiesBackingEngine.listRoles(users, user) + .forEach(role -> principals.add(new RolePrincipal(role.getName()))); - static void addRole(Set principals, String role) { - role = role.trim(); - if (!role.isEmpty()) - principals.add(new RolePrincipal(role)); + return principals; } } \ No newline at end of file diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java index ae463cc9fc6..ce97efc79e2 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java @@ -202,7 +202,7 @@ public boolean login() throws LoginException { } } - principals = JAASUtils.getPrincipals(user, users, infos); + principals = JAASUtils.getPrincipals(user, users); users.clear(); diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java index bc5bf4087a0..6437dc1e690 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java @@ -129,7 +129,7 @@ public boolean login() throws LoginException { } } - principals = JAASUtils.getPrincipals(user, users, infos); + principals = JAASUtils.getPrincipals(user, users); users.clear(); diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java index a4f39df5c42..6dbee4eab77 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyLoginModule.java @@ -147,7 +147,7 @@ public boolean login() throws LoginException { } } - principals = JAASUtils.getPrincipals(user, users, infos); + principals = JAASUtils.getPrincipals(user, users); users.clear(); From 27b9946c313daa0e10cb59fb2e5c6e8c2e2fa7ab Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Fri, 2 May 2025 19:03:57 +0200 Subject: [PATCH 6/6] KARAF-5014: make nested groups discoverable --- .../AbstractPropertiesBackingEngine.java | 73 +++++--- .../AbstractPropertiesBackingEngineTest.java | 168 +++++++++++++----- 2 files changed, 166 insertions(+), 75 deletions(-) diff --git a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java index b86f94fecd6..f05741711dd 100644 --- a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java +++ b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngine.java @@ -119,22 +119,24 @@ public static List listRoles(Properties users, String name) { List result = new ArrayList<>(); String userInfo = users.get(name); - String[] infos = userInfo.split(","); - for (int i = getFirstRoleIndex(name); i < infos.length; i++) { - String roleName = infos[i].trim(); - if (roleName.isEmpty()) { - // ignore - } else if (roleName.startsWith(GROUP_PREFIX)) { - for (RolePrincipal rp : listRoles(users, roleName)) { + if (userInfo != null) { + String[] infos = userInfo.split(","); + for (int i = getFirstRoleIndex(name); i < infos.length; i++) { + String roleName = infos[i].trim(); + if (roleName.isEmpty()) { + // ignore + } else if (roleName.startsWith(GROUP_PREFIX)) { + for (RolePrincipal rp : listRoles(users, roleName)) { + if (!result.contains(rp)) { + result.add(rp); + } + } + } else { + RolePrincipal rp = new RolePrincipal(roleName); if (!result.contains(rp)) { result.add(rp); } } - } else { - RolePrincipal rp = new RolePrincipal(roleName); - if (!result.contains(rp)) { - result.add(rp); - } } } return result; @@ -173,24 +175,22 @@ public void addRole(String username, String role) { String newUserInfos = userInfos + "," + role; users.put(username, newUserInfos); } - } - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); + try { + users.save(); + } catch (Exception ex) { + LOGGER.error("Cannot update users file,", ex); + } } } @Override public void deleteRole(String username, String role) { - String[] infos = null; - StringBuilder userInfoBuffer = new StringBuilder(); - String userInfos = users.get(username); // if user already exists, remove the role if (userInfos != null && userInfos.length() > 0) { - infos = userInfos.split(","); + StringBuilder userInfoBuffer = new StringBuilder(); + String[] infos = userInfos.split(","); int firstRoleIndex = getFirstRoleIndex(username); if (firstRoleIndex == 1) { // index 0 is password @@ -207,12 +207,11 @@ public void deleteRole(String username, String role) { } String newUserInfo = userInfoBuffer.toString(); users.put(username, newUserInfo); - } - - try { - users.save(); - } catch (Exception ex) { - LOGGER.error("Cannot update users file,", ex); + try { + users.save(); + } catch (Exception ex) { + LOGGER.error("Cannot update users file,", ex); + } } } @@ -230,7 +229,15 @@ public static List listGroups(Properties users, String userName) for (int i = getFirstRoleIndex(userName); i < infos.length; i++) { String name = infos[i].trim(); if (name.startsWith(GROUP_PREFIX)) { - result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length()))); + GroupPrincipal group = new GroupPrincipal(name.substring(GROUP_PREFIX.length())); + if (!result.contains(group)) { + result.add(group); + for (GroupPrincipal g : listGroups(users, name)) { + if (!result.contains(g)) { + result.add(g); + } + } + } } } } @@ -259,6 +266,16 @@ public void deleteGroup(String username, String group) { } } } + for (GroupPrincipal g : listGroups().keySet()) { + if (!group.equals(g.getName())) { + for (GroupPrincipal nestedGroup : listGroups(users, GROUP_PREFIX + g.getName())) { + if (group.equals(nestedGroup.getName())) { + // there is another group referencing this group, nothing to clean up + return; + } + } + } + } // nobody is using this group any more, remove it deleteUser(GROUP_PREFIX + group); diff --git a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java index 86c4033c839..442f87917b2 100644 --- a/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java +++ b/jaas/modules/src/test/java/org/apache/karaf/jaas/modules/AbstractPropertiesBackingEngineTest.java @@ -14,14 +14,12 @@ */ package org.apache.karaf.jaas.modules; +import static org.apache.karaf.jaas.modules.BackingEngine.GROUP_PREFIX; import static org.apache.karaf.jaas.modules.PrincipalHelper.names; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsInAnyOrder; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; +import static org.junit.Assert.*; import org.apache.felix.utils.properties.Properties; import org.apache.karaf.jaas.boot.principal.GroupPrincipal; @@ -52,31 +50,34 @@ public void start() throws IOException { public void addUserInternalTest() { // non-existing user engine.addUserInternal("a", ""); + List uaInfo = List.of(p.get("a").split(",")); + assertEquals(1, uaInfo.size()); + assertEquals("", uaInfo.get(0)); // update empty password on existing user with no roles engine.addUserInternal("a", "pass1"); - assertThat(List.of(p.get("a").split(",")), contains("pass1")); - UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); - assertTrue(engine.listGroups(upa).isEmpty()); - assertTrue(engine.listRoles(upa).isEmpty()); + uaInfo = List.of(p.get("a").split(",")); + assertEquals(1, uaInfo.size()); + assertEquals("pass1", uaInfo.get(0)); // update password on existing user with no roles engine.addUserInternal("a", "pass2"); - assertThat(List.of(p.get("a").split(",")), contains("pass2")); - upa = PrincipalHelper.getUser(engine, "a"); - assertTrue(engine.listGroups(upa).isEmpty()); - assertTrue(engine.listRoles(upa).isEmpty()); + uaInfo = List.of(p.get("a").split(",")); + assertEquals(1, uaInfo.size()); + assertEquals("pass2", uaInfo.get(0)); - // update password on existing user with roles + // update password on existing user with roles and groups engine.addRole("a", "role1"); engine.addGroup("a", "g1"); engine.addGroupRole("g1", "role2"); engine.addUserInternal("a", "pass3"); - assertThat(List.of(p.get("a").split(",")), - contains("pass3", "role1", PropertiesBackingEngine.GROUP_PREFIX + "g1")); - upa = PrincipalHelper.getUser(engine, "a"); - assertThat(names(engine.listGroups(upa)), containsInAnyOrder("g1")); - assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role1", "role2")); + uaInfo = List.of(p.get("a").split(",")); + assertEquals(3, uaInfo.size()); + assertThat(uaInfo, contains("pass3", "role1", getGroupRef("g1"))); + } + + private String getGroupRef(String groupName) { + return GROUP_PREFIX + groupName; } @Test @@ -91,66 +92,81 @@ public void lookupUserTest() { @Test public void listRolesTest() { + // non-existing user + assertTrue(engine.listRoles(p, "a").isEmpty()); + // empty roles in groups - p.put(PropertiesBackingEngine.GROUP_PREFIX + "g1", ",,,"); // simulate manual editing of the properties file + p.put(getGroupRef("g1"), ""); GroupPrincipal gpg1 = PrincipalHelper.getGroup(engine, "g1"); assertTrue(engine.listRoles(gpg1).isEmpty()); + p.put(getGroupRef("g1"), ",,,"); + assertTrue(engine.listRoles(gpg1).isEmpty()); // empty roles in users - p.put("a", "pass1,,,"); + p.put("a", "pass1"); UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); assertTrue(engine.listRoles(upa).isEmpty()); + p.put("a", "pass1,,,"); + assertTrue(engine.listRoles(upa).isEmpty()); // duplicate role p.put("a", "pass1,role1,role1"); - upa = PrincipalHelper.getUser(engine, "a"); List roles = engine.listRoles(upa); assertEquals(1, roles.size()); assertEquals("role1", roles.get(0).getName()); + + // roles in nested group + p.put("a", "pass1,role1,role1," + getGroupRef("g1")); + p.put(getGroupRef("g1"), getGroupRef("g2")); + p.put(getGroupRef("g2"), "role2"); + roles = engine.listRoles(upa); + assertEquals(2, roles.size()); + assertThat(names(roles), containsInAnyOrder("role1", "role2")); } @Test public void addRoleTest() { - // empty info in groups - engine.createGroup("g1"); - engine.addGroupRole("g1", "role1"); - GroupPrincipal gpg1 = PrincipalHelper.getGroup(engine, "g1"); - assertThat(names(engine.listRoles(gpg1)), contains("role1")); + // non-existing user + engine.addRole("a", "role1"); + assertNull(p.get("a")); // empty info in users engine.addUser("a", ""); - engine.addRole("a", "role2"); - UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); - assertThat(names(engine.listRoles(upa)), contains("role2")); + engine.addRole("a", "role1"); // user empty password is preserved - assertThat(List.of(p.get("a").split(",")), containsInAnyOrder("", "role2")); + List uaInfo = List.of(p.get("a").split(",")); + assertEquals(2, uaInfo.size()); + assertThat(uaInfo, containsInAnyOrder("", "role1")); // duplicate role - engine.addRole("a", "role2"); - upa = PrincipalHelper.getUser(engine, "a"); - assertThat(names(engine.listRoles(upa)), contains("role2")); - assertThat(List.of(p.get("a").split(",")), containsInAnyOrder("", "role2")); + engine.addRole("a", "role1"); + uaInfo = List.of(p.get("a").split(",")); + assertEquals(2, uaInfo.size()); + assertThat(uaInfo, containsInAnyOrder("", "role1")); - // duplicate group - engine.addUser("b", ""); - engine.addGroup("b", "g2"); - engine.addGroup("b", "g2"); - UserPrincipal upb = PrincipalHelper.getUser(engine, "b"); - assertThat(names(engine.listGroups(upb)), contains("g2")); - assertThat(List.of(p.get("b").split(",")), - containsInAnyOrder("", PropertiesBackingEngine.GROUP_PREFIX + "g2")); + // empty info in groups + engine.createGroup("g1"); + engine.addGroupRole("g1", "role2"); + List g1Info = List.of(p.get(getGroupRef("g1")).split(",")); + assertEquals(1, g1Info.size()); + assertEquals("role2", g1Info.get(0)); } @Test public void deleteRoleTest() { + // non-existing user + engine.deleteRole("a", "role1"); + assertNull(p.get("a")); + // delete in group engine.createGroup("g1"); engine.addGroupRole("g1", "role1"); engine.addGroupRole("g1", "role2"); engine.addGroupRole("g1", "role3"); engine.deleteGroupRole("g1", "role1"); - GroupPrincipal gpg1 = PrincipalHelper.getGroup(engine, "g1"); - assertThat(names(engine.listRoles(gpg1)), containsInAnyOrder("role2", "role3")); + List g1Info = List.of(p.get(getGroupRef("g1")).split(",")); + assertEquals(2, g1Info.size()); + assertThat(g1Info, containsInAnyOrder("role2", "role3")); // delete in user engine.addUser("a", ""); @@ -158,10 +174,68 @@ public void deleteRoleTest() { engine.addRole("a", "role5"); engine.addRole("a", "role6"); engine.deleteRole("a", "role4"); - UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); - assertThat(names(engine.listRoles(upa)), containsInAnyOrder("role5", "role6")); // user empty password is preserved - assertThat(List.of(p.get("a").split(",")), containsInAnyOrder("", "role5", "role6")); + List uaInfo = List.of(p.get("a").split(",")); + assertEquals(3, uaInfo.size()); + assertThat(uaInfo, containsInAnyOrder("", "role5", "role6")); + } + + @Test + public void listGroupsTest() { + // non-existing user + assertTrue(engine.listGroups(p, "a").isEmpty()); + + // duplicate group + p.put("a", "pass1," + getGroupRef("g1") + "," + getGroupRef("g1")); + UserPrincipal upa = PrincipalHelper.getUser(engine, "a"); + List groups = engine.listGroups(upa); + assertEquals(1, groups.size()); + assertEquals("g1", groups.get(0).getName()); + + // nested group + p.put(getGroupRef("g1"), getGroupRef("g2")); + groups = engine.listGroups(upa); + assertEquals(2, groups.size()); + assertThat(names(groups), containsInAnyOrder("g1", "g2")); + + // duplicate nested group + p.put(getGroupRef("g2"), getGroupRef("g3") + "," + getGroupRef("g3")); + groups = engine.listGroups(upa); + assertEquals(3, groups.size()); + assertThat(names(groups), containsInAnyOrder("g1", "g2", "g3")); + } + + @Test + public void addGroupTest() { + // duplicate group + engine.addUser("a", ""); + engine.addGroup("a", "g1"); + engine.addGroup("a", "g1"); + List uaInfo = List.of(p.get("a").split(",")); + assertEquals(2, uaInfo.size()); + assertThat(uaInfo, containsInAnyOrder("", getGroupRef("g1"))); + } + + @Test + public void deleteGroupTest() { + // group has only 1 user reference + engine.addUser("a", ""); + engine.addGroup("a", "g1"); + engine.deleteGroup("a", "g1"); + assertNull(p.get(getGroupRef("g1"))); + + // group is referenced by multiple users + engine.addGroup("a", "g1"); + engine.addUser("b", ""); + engine.addGroup("b", "g1"); + engine.deleteGroup("a", "g1"); + assertNotNull(p.get(getGroupRef("g1"))); + + // group is referenced by other groups + engine.addGroup("a", "g2"); + p.put(getGroupRef("g3"), getGroupRef("g2")); + engine.deleteGroup("a", "g2"); + assertNotNull(p.get(getGroupRef("g2"))); } @After