Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

KARAF-5014: consider first group role in users.properties and ignore empty roles #1863

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

KARAF-5014: consider first group role in users.properties and ignore empty roles #1863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion assemblies/features/base/src/main/resources/resources/etc/users.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
13 changes: 10 additions & 3 deletions ...les/src/main/java/org/apache/karaf/jaas/modules/properties/DigestPasswordLoginModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}

Expand All @@ -233,4 +236,8 @@ public boolean login() throws LoginException {
return true;
}

private void addRole(Set<Principal> principals, String trimmedRole) {
if (!trimmedRole.isEmpty())
principals.add(new RolePrincipal(trimmedRole));
}
}
63 changes: 43 additions & 20 deletions ...dules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why forcing encryption here ? It's an optional feature.

Copy link
Author

@stataru8 stataru8 Sep 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just moved this call from its original location in addUserInternal:

karaf/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java

Line 62 in e9b9c97

String encPassword = encryptionSupport.encrypt(password);

The call is only needed when adding a user and shouldn't be made when adding a group:

karaf/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java

Line 262 in c01d0bc

addUserInternal(groupName, ""); // groups don't have password
There is the risk of encrypting "", which with the defaults, results in {CRYPT}ABC...{CRYPT}. After jaas:group-add karaf newGrup,
jaas:user-list will return

User Name | Group   | Role
----------+---------+-------------------------------------------------------------------------------
karaf     | newGrup | {CRYPT}ABC...{CRYPT}

Maybe at this point, we should create another addUserInternal with just the username as an argument: private void addUserInternal(String username), or another function just for groups...

}

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
Expand Down Expand Up @@ -139,8 +138,11 @@ private List<RolePrincipal> listRoles(String name) {
List<RolePrincipal> 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)) {
Expand All @@ -157,22 +159,38 @@ private List<RolePrincipal> 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();
Expand All @@ -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]);
}
}
Expand All @@ -222,7 +245,7 @@ private List<GroupPrincipal> 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())));
Expand All @@ -236,7 +259,7 @@ private List<GroupPrincipal> 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);
}
Expand Down Expand Up @@ -282,7 +305,7 @@ public Map<GroupPrincipal, String> 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");
}
Expand Down
13 changes: 10 additions & 3 deletions ...modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
}
}

Expand All @@ -160,4 +163,8 @@ public boolean login() throws LoginException {
return true;
}

private void addRole(Set<Principal> principals, String trimmedRole) {
if (!trimmedRole.isEmpty())
principals.add(new RolePrincipal(trimmedRole));
}
}
64 changes: 56 additions & 8 deletions ...s/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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");
Expand All @@ -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();

Expand All @@ -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");
Expand All @@ -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) {
Expand All @@ -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()) {
Expand Down
7 changes: 5 additions & 2 deletions ...les/src/test/java/org/apache/karaf/jaas/modules/properties/PropertiesLoginModuleTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String> options = new HashMap<>();
Expand All @@ -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);
Expand Down