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.

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

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

Conversation

stataru8
Copy link

@stataru8 stataru8 commented Sep 20, 2024

https://issues.apache.org/jira/browse/KARAF-5014

This issue is quite old and only affects those who directly modify the users file. I'm not sure if it is still relevant, or if the current behaviour should be considered acceptable.

Current behaviour:

  • the first role in a group is ignored
  • when a group is created via jaas commands, the password/role group is added by default

Proposal:

  • consider the first role in a group
  • when a group is created using jaas commands, add password/role "" by default, leaving the group information empty
  • ignore empty roles in both users and groups

@AndreVirtimo
Copy link
Contributor

Hi @stataru8 , thank you for picking up this issue.

Why do you propose "when a group is created using jaas commands, add an empty role by default"? Is this because there is no role which could be assigned during creation? Is it not possible to have an empty group?

Regards
Andre

@jbonofre jbonofre self-requested a review September 27, 2024 12:15
@jbonofre
Copy link
Member

jbonofre commented Sep 27, 2024

I think @stataru8 meant add empty password for role (as a role doesn't have password).

A group can be empty without problem though.

@@ -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

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:

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

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...

if (role.equals(rp.getName())) {
return;

// groups don't have password and empty should be ignored
Copy link
Member

Choose a reason for hiding this comment

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

A group can be empty (no role, no group, no user).

Copy link
Author

Choose a reason for hiding this comment

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

My original comment was a bit misleading, what I meant is:

  • If a user info is empty, we shouldn't replace "" by role, or else role becomes the user's password.
  • If a group info is empty, we should replace "" by role.

@stataru8
Copy link
Author

Hello @AndreVirtimo, as far as I know, we have two commands that allow us to create groups:

  • jaas:group-add user myGroup1
  • jaas:group-create myGroup2

In both cases, there is no mandatory argument for the role. Today, the role/password group is added by default. The proposal is to add "" instead, leaving the group information empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants