From 5bef7950daf8fb739b25f959c4209e38608bacd6 Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Fri, 8 Nov 2024 19:43:03 +0100 Subject: [PATCH 1/2] KARAF-5014: consider first group role in users.properties and ignore empty roles --- .../resources/resources/etc/users.properties | 2 +- .../properties/DigestPasswordLoginModule.java | 13 +++- .../properties/PropertiesBackingEngine.java | 63 ++++++++++++------ .../properties/PropertiesLoginModule.java | 13 +++- .../PropertiesBackingEngineTest.java | 64 ++++++++++++++++--- .../properties/PropertiesLoginModuleTest.java | 7 +- 6 files changed, 125 insertions(+), 37 deletions(-) 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/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..35816954951 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,11 @@ import java.lang.reflect.Field; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; +import java.security.Principal; import java.util.HashSet; import java.util.Map; +import java.util.Set; + import javax.security.auth.Subject; import javax.security.auth.callback.Callback; import javax.security.auth.callback.CallbackHandler; @@ -214,13 +217,13 @@ public boolean login() throws LoginException { 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())); + for (int j = 0; j < roles.length; j++) { + addRole(principals, roles[j].trim()); } } } else { // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); + addRole(principals, infos[i].trim()); } } @@ -233,4 +236,8 @@ public boolean login() throws LoginException { return true; } + private void addRole(Set principals, String trimmedRole) { + if (!trimmedRole.isEmpty()) + principals.add(new RolePrincipal(trimmedRole)); + } } 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..e8199c0430f 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 @@ -52,14 +52,13 @@ public void addUser(String username, String password) { if (username.startsWith(GROUP_PREFIX)) throw new IllegalArgumentException("Prefix not permitted: " + GROUP_PREFIX); - addUserInternal(username, password); + addUserInternal(username, encryptionSupport.encrypt(password)); } - private void addUserInternal(String username, String password) { + private void addUserInternal(String username, String encPassword) { String[] infos = null; StringBuilder userInfoBuffer = new StringBuilder(); - String encPassword = encryptionSupport.encrypt(password); String userInfos = users.get(username); //If user already exists, update password @@ -139,8 +138,11 @@ 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++) { + for (int i = getFirstRoleIndex(name); i < infos.length; i++) { String roleName = infos[i]; + if(roleName.trim().isEmpty()) + continue; + if (roleName.startsWith(GROUP_PREFIX)) { for (RolePrincipal rp : listRoles(roleName)) { if (!result.contains(rp)) { @@ -157,22 +159,38 @@ private List listRoles(String name) { return result; } + private int getFirstRoleIndex(String name) { + if (name.trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) { + return 0; + } + return 1; + } + @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 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(PropertiesBackingEngine.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; + for (GroupPrincipal gp : listGroups(username)) { + if (role.equals(GROUP_PREFIX + gp.getName())) { + return; + } } + String newUserInfos = userInfos + "," + role; + users.put(username, newUserInfos); } - String newUserInfos = userInfos + "," + role; - users.put(username, newUserInfos); } try { users.save(); @@ -191,12 +209,17 @@ public void deleteRole(String username, String role) { //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++) { + 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)) { - userInfoBuffer.append(","); + if(userInfoBuffer.length() > 0) { + userInfoBuffer.append(","); + } userInfoBuffer.append(infos[i]); } } @@ -222,7 +245,7 @@ private List listGroups(String userName) { String userInfo = users.get(userName); if (userInfo != null) { String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { + 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()))); @@ -236,7 +259,7 @@ private List listGroups(String userName) { public void addGroup(String username, String group) { String groupName = GROUP_PREFIX + group; if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); + addUserInternal(groupName, ""); // groups don't have password } addRole(username, groupName); } @@ -282,7 +305,7 @@ public Map listGroups() { public void createGroup(String group) { String groupName = GROUP_PREFIX + group; if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); + addUserInternal(groupName, ""); // groups don't have password } else { throw new IllegalArgumentException("Group: " + group + " already exist"); } 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..dc946d44002 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,11 @@ import java.io.File; import java.io.IOException; +import java.security.Principal; import java.util.HashSet; import java.util.Map; +import java.util.Set; + import javax.security.auth.Subject; import javax.security.auth.callback.Callback; import javax.security.auth.callback.CallbackHandler; @@ -141,13 +144,13 @@ public boolean login() throws LoginException { 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())); + for (int j = 0; j < roles.length; j++) { + addRole(principals, roles[j].trim()); } } } else { // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); + addRole(principals, infos[i].trim()); } } @@ -160,4 +163,8 @@ public boolean login() throws LoginException { return true; } + private void addRole(Set principals, String trimmedRole) { + if (!trimmedRole.isEmpty()) + principals.add(new RolePrincipal(trimmedRole)); + } } 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); From b9efb569f997e42a47a5f558cfc87c4ee0b77857 Mon Sep 17 00:00:00 2001 From: Stefan Tataru Date: Wed, 20 Nov 2024 15:54:24 +0100 Subject: [PATCH 2/2] KARAF-5014: consider first group role in keys.properties and ignore empty role --- .../distribution/text/etc/keys.properties | 2 +- .../resources/resources/etc/keys.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 +- .../apache/karaf/jaas/modules/JAASUtils.java | 21 ++++- .../properties/PropertiesBackingEngine.java | 23 ++--- .../properties/PropertiesLoginModule.java | 12 +-- .../publickey/PublickeyBackingEngine.java | 46 +++++++--- .../publickey/PublickeyLoginModule.java | 8 +- .../publickey/PublicKeyLoginModuleTest.java | 86 ++++++++++++------- .../karaf/jaas/modules/publickey/pubkey.users | 4 +- 13 files changed, 131 insertions(+), 81 deletions(-) diff --git a/assemblies/apache-karaf/src/main/distribution/text/etc/keys.properties b/assemblies/apache-karaf/src/main/distribution/text/etc/keys.properties index bdec342a222..f80a4b94582 100644 --- a/assemblies/apache-karaf/src/main/distribution/text/etc/keys.properties +++ b/assemblies/apache-karaf/src/main/distribution/text/etc/keys.properties @@ -28,4 +28,4 @@ # with the name "karaf".. # #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 \ No newline at end of file +_g_\:admingroup = admin,manager,viewer,systembundles,ssh \ No newline at end of file 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/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/JAASUtils.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/JAASUtils.java index 4d6317995cc..bb439b04397 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,10 @@ */ package org.apache.karaf.jaas.modules; +import org.apache.karaf.jaas.boot.principal.RolePrincipal; +import java.security.Principal; import java.util.Map; +import java.util.Set; public final class JAASUtils { @@ -30,4 +33,20 @@ public static String getString(Map options, String key) { return (String)val; } -} + /** + * Determines the starting index of role and group definitions for a given key in a file-based login module. + * @param name the property key to evaluate, representing a group or a username + * @return 0 if the key starts with the group prefix, otherwise 1 + */ + public static int getFirstRoleIndex(String name) { + if (name.trim().startsWith(BackingEngine.GROUP_PREFIX)) + return 0; + return 1; + } + + public static void addRole(Set principals, String role) { + role = role.trim(); + if (!role.isEmpty()) + principals.add(new RolePrincipal(role.trim())); + } +} \ 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 e8199c0430f..abe9d5c33fb 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 @@ -26,6 +26,7 @@ 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.JAASUtils; import org.apache.karaf.jaas.modules.encryption.EncryptionSupport; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -138,11 +139,10 @@ private 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++) { + for (int i = JAASUtils.getFirstRoleIndex(name); i < infos.length; i++) { String roleName = infos[i]; - if(roleName.trim().isEmpty()) + if (roleName.trim().isEmpty()) continue; - if (roleName.startsWith(GROUP_PREFIX)) { for (RolePrincipal rp : listRoles(roleName)) { if (!result.contains(rp)) { @@ -159,24 +159,15 @@ private List listRoles(String name) { return result; } - private int getFirstRoleIndex(String name) { - if (name.trim().startsWith(PropertiesBackingEngine.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() + if (userInfos.trim().isEmpty() && username.trim().startsWith(PropertiesBackingEngine.GROUP_PREFIX)) { users.put(username, role); - } else { for (RolePrincipal rp : listRoles(username)) { if (role.equals(rp.getName())) { @@ -210,8 +201,8 @@ public void deleteRole(String username, String role) { if (userInfos != null && userInfos.length() > 0) { infos = userInfos.split(","); - int firstRoleIndex = getFirstRoleIndex(username); - if(firstRoleIndex == 1) {// index 0 is password + int firstRoleIndex = JAASUtils.getFirstRoleIndex(username); + if (firstRoleIndex == 1) {// index 0 is password String password = infos[0]; userInfoBuffer.append(password); } @@ -245,7 +236,7 @@ private List listGroups(String userName) { String userInfo = users.get(userName); if (userInfo != null) { String[] infos = userInfo.split(","); - for (int i = getFirstRoleIndex(userName); i < infos.length; i++) { + for (int i = JAASUtils.getFirstRoleIndex(userName); i < infos.length; i++) { String name = infos[i]; 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/properties/PropertiesLoginModule.java b/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java index dc946d44002..9fbf2b39e07 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,10 +18,8 @@ import java.io.File; import java.io.IOException; -import java.security.Principal; import java.util.HashSet; import java.util.Map; -import java.util.Set; import javax.security.auth.Subject; import javax.security.auth.callback.Callback; @@ -34,7 +32,6 @@ 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; @@ -145,12 +142,12 @@ public boolean login() throws LoginException { if (groupInfo != null) { String[] roles = groupInfo.split(","); for (int j = 0; j < roles.length; j++) { - addRole(principals, roles[j].trim()); + JAASUtils.addRole(principals, roles[j]); } } } else { // it's an user reference - addRole(principals, infos[i].trim()); + JAASUtils.addRole(principals, infos[i]); } } @@ -162,9 +159,4 @@ public boolean login() throws LoginException { succeeded = true; return true; } - - private void addRole(Set principals, String trimmedRole) { - if (!trimmedRole.isEmpty()) - principals.add(new RolePrincipal(trimmedRole)); - } } 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..240dcee4f9d 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 @@ -26,6 +26,7 @@ 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.JAASUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -132,8 +133,10 @@ 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++) { + for (int i = JAASUtils.getFirstRoleIndex(name); i < infos.length; i++) { String roleName = infos[i]; + if (roleName.trim().isEmpty()) + continue; if (roleName.startsWith(GROUP_PREFIX)) { for (RolePrincipal rp : listRoles(roleName)) { if (!result.contains(rp)) { @@ -154,13 +157,25 @@ private List listRoles(String name) { 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 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); } - String newUserInfos = userInfos + "," + role; - users.put(username, newUserInfos); } try { users.save(); @@ -179,12 +194,17 @@ public void deleteRole(String username, String role) { //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++) { + int firstRoleIndex = JAASUtils.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)) { - userInfoBuffer.append(","); + if(userInfoBuffer.length() > 0) { + userInfoBuffer.append(","); + } userInfoBuffer.append(infos[i]); } } @@ -210,7 +230,7 @@ private List listGroups(String userName) { String userInfo = users.get(userName); if (userInfo != null) { String[] infos = userInfo.split(","); - for (int i = 1; i < infos.length; i++) { + for (int i = JAASUtils.getFirstRoleIndex(userName); i < infos.length; i++) { String name = infos[i]; if (name.startsWith(GROUP_PREFIX)) { result.add(new GroupPrincipal(name.substring(GROUP_PREFIX.length()))); @@ -224,7 +244,7 @@ private List listGroups(String userName) { public void addGroup(String username, String group) { String groupName = GROUP_PREFIX + group; if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); + addUserInternal(groupName, ""); // groups don't have public key } addRole(username, groupName); } @@ -270,7 +290,7 @@ public Map listGroups() { public void createGroup(String group) { String groupName = GROUP_PREFIX + group; if (users.get(groupName) == null) { - addUserInternal(groupName, "group"); + addUserInternal(groupName, ""); // groups don't have public key } else { throw new IllegalArgumentException("Group: " + group + " already exist"); } 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..170f4bdcc7a 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 @@ -53,9 +53,9 @@ 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; @@ -160,13 +160,13 @@ public boolean login() throws LoginException { 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())); + for (int j = 0; j < roles.length; j++) { + JAASUtils.addRole(principals, roles[j]); } } } else { // it's an user reference - principals.add(new RolePrincipal(infos[i].trim())); + JAASUtils.addRole(principals, infos[i]); } } 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..38a906f7343 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,6 +15,7 @@ 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; @@ -48,22 +49,24 @@ 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"; + @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 modulus = new BigInteger(RSA_KNOWN_MODULUS); BigInteger exponent = new BigInteger("65537"); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); @@ -77,12 +80,44 @@ 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); + BigInteger exponent = new BigInteger("65537"); + KeyFactory keyFactory = KeyFactory.getInstance("RSA"); + KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, 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,15 +192,8 @@ 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 modulus = new BigInteger(RSA_KNOWN_MODULUS); BigInteger exponent = new BigInteger("65537"); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); @@ -188,15 +216,13 @@ 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); + String known_modulus = RSA_KNOWN_MODULUS.substring(0, RSA_KNOWN_MODULUS.length() - 1) + "3"; + assertEquals(known_modulus.length(), RSA_KNOWN_MODULUS.length()); + assertTrue(known_modulus.charAt(RSA_KNOWN_MODULUS.length() - 1) != + RSA_KNOWN_MODULUS.charAt(RSA_KNOWN_MODULUS.length() - 1)); + + BigInteger modulus = new BigInteger(known_modulus); BigInteger exponent = new BigInteger("65537"); KeyFactory keyFactory = KeyFactory.getInstance("RSA"); KeySpec publicKeySpec = new RSAPublicKeySpec(modulus, exponent); @@ -274,14 +300,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