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

stataru8 · 2024-09-20T17:46:27Z

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 · 2024-09-27T12:13:55Z

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 · 2024-09-27T12:18:32Z

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

A group can be empty without problem though.

jbonofre · 2024-09-27T12:20:02Z

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

            throw new IllegalArgumentException("Prefix not permitted: " + GROUP_PREFIX);

-        addUserInternal(username, password);
+        addUserInternal(username, encryptionSupport.encrypt(password));


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

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

jbonofre · 2024-09-27T12:20:39Z

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

-                if (role.equals(rp.getName())) {
-                    return; 
+
+            // groups don't have password and empty should be ignored


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

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 · 2024-09-27T16:47:55Z

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.

jbonofre · 2024-11-08T14:36:28Z

Sorry for the delay in the review/comment. I want to provide some context.

The idea of the first "role" in a group definition, it's not actually a role but just a "prefix" (group,) to indicate that we are in a group.

That's why be default, in etc/users.properties you have:

_g_\:admingroup = group,admin,manager,viewer,systembundles,ssh

You can see that group here is not actually a role in admingroup but a "prefix flag".

I agree it's confusing (including the documentation), but if we "change" to remove the prefix, it means that group would be considered as a role in admingroup.

jbonofre

Can you please update default etc/users.properties to remove group as it's not actually a role ?

stataru8 · 2024-11-08T18:40:25Z

I just realized that group is also used as a "prefix flag" in keys.properties. I wonder if the changes need to be propagated in this login module as well:

karaf/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyBackingEngine.java

Line 273 in a8d9901

addUserInternal(groupName, "group");

Let me know if is that's too much for this PR or out of scope.

If all these changes are too risky, just making the documentation clearer about the "prefix flag" is also an option...

jbonofre · 2024-11-09T09:18:55Z

@stataru8 yes, historically, we use this "flag" in all group/role definition (users, keys, ...). So, we can consider this as a breaking change for users (if we remove the group prefix).
I wonder what's the best option:

Documenting clearly the group prefix (but I wonder if it's still useful to have this prefix)
Remove the use of group prefix for Karaf 4.5.0 (as it's a breaking change)
Thoughts ?

AndreVirtimo · 2024-11-11T07:53:15Z

I don't think we need a flag in the value. Because we already have g in the key.

So I am in favor of a change. I think the risk is low. If users don't migrate their configuration, they get an additional role “group”, which shouldn't hurt.

jbonofre · 2024-11-11T11:29:10Z

@AndreVirtimo i agree. However this PR should be completed to keys file.

stataru8 · 2024-11-11T14:26:05Z

I'll try to commit the changes for keys.properties this week or next. Will request a re-review once it's done.

UPDATE: I'll need more time to test this.

stataru8 · 2024-11-20T15:26:33Z

I see some duplicate code in jaas/modules/src/main/java/org/apache/karaf/jaas/modules/properties/PropertiesBackingEngine.java and jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyBackingEngine.java.
Should we take this opportunity to refactor? Is it safe to do so? I was thinking of creating a parent class named AbstractPropertiesBackingEngine or FileBasedBackingEngine...

jbonofre · 2024-11-20T21:47:36Z

@stataru8 as we target that for 4.5.x, I don't have problem if we include a refactoring (especially a cleanup one 😄 ).

…empty roles

stataru8 · 2025-04-19T13:41:18Z

Hello again, one more question, should the PR also include changes to the .adoc files?

karaf/manual/src/main/asciidoc/user-guide/security.adoc

Line 142 in ad427cd

_g_\:admingroup = group,admin,manager,viewer,ssh

…empty roles - unit tests

…empty roles - update .adoc files

stataru8 · 2025-05-01T12:15:27Z

jaas/command/src/main/java/org/apache/karaf/jaas/command/ListUsersCommand.java

                table.addRow().addContent(userName, group.getName(), roleName);
            }
+        } else {
+            table.addRow().addContent(userName, group.getName(), "");


I don't know if this proposal makes sense. I can revert.

Setup: user foo without any groups or roles declared.

Current behaviour

karaf@trun()> jaas:group-add foo g1 karaf@trun()> jaas:update karaf@trun()> jaas:realm-manage --realm karaf --module org.apache.karaf.jaas.modules.properties.PropertiesLoginModule karaf@trun()> jaas:user-list User Name │ Group │ Role ──────────┼────────────┼────────────── foo │ │

Proposal

karaf@trun()> jaas:group-add foo g1 karaf@trun()> jaas:update karaf@trun()> jaas:realm-manage --realm karaf --module org.apache.karaf.jaas.modules.properties.PropertiesLoginModule karaf@trun()> jaas:user-list User Name │ Group │ Role ──────────┼────────────┼────────────── foo │ g1 │

Proposal - more tests

karaf@trun()> jaas:role-add foo r1 karaf@trun()> jaas:update karaf@trun()> jaas:realm-manage --realm karaf --module org.apache.karaf.jaas.modules.properties.PropertiesLoginModule karaf@trun()> jaas:user-list User Name │ Group │ Role ──────────┼────────────┼────────────── foo │ g1 │ foo │ │ r1 karaf@trun()> jaas:group-role-add g1 r1 karaf@trun()> jaas:update karaf@trun()> jaas:realm-manage --realm karaf --module org.apache.karaf.jaas.modules.properties.PropertiesLoginModule karaf@trun()> jaas:user-list User Name │ Group │ Role ──────────┼────────────┼────────────── foo │ g1 │ r1

Maybe it makes sense for the last output to be

User Name │ Group │ Role ──────────┼────────────┼────────────── foo │ g1 │ r1 foo │ │ r1

stataru8 · 2025-05-01T17:21:45Z

PR is ready for review again.
~~Edit: just discovered another small issue, will have to re-test~~
Edit2: OK, now PR should be good for review, I added the last commit because roles in nested groups are already discoverable

…s and roles with the underlying baking engine

mattrpav · 2025-05-08T15:13:22Z

This should be karaf 4.5.x (or higher) and clearly called out in a Release Notes / Migration Guide.

Make a security-related change to the file and presume the extra role 'group' would not be an issue is not a good practice.

jbonofre · 2025-05-08T15:42:42Z

I asked the question of breaking change. For now I consider a 4.5.x thing.

mattrpav · 2025-07-28T20:21:24Z

Is the bug here really the JAAS karaf commands / Encryption Support need to handle the 'group' placeholder better?

keys.properties, and ability to support other security providers with a similar file format is useful -- this would allow for sharing of JAAS commands to be able to resolve group/role info for non-local security providers, etc.

For example: LDAP support could be improved to move the group/role mapping out of the LDAP config and simply into a properties file of the same format that maps userDN'groupDN to roles, etc.

I'm trying to understand what the real 'problem' is with the current solution? Perhaps the Karaf JAAS commands simply need to be fixed?

jbonofre self-requested a review September 27, 2024 12:15

jbonofre approved these changes Sep 27, 2024

View reviewed changes

jbonofre requested changes Sep 27, 2024

View reviewed changes

stataru8 force-pushed the stataru/fix/KARAF-5014 branch 2 times, most recently from dac3692 to c01d0bc Compare September 27, 2024 16:07

stataru8 force-pushed the stataru/fix/KARAF-5014 branch from c01d0bc to a30ea9d Compare November 7, 2024 17:44

jbonofre requested changes Nov 8, 2024

View reviewed changes

stataru8 force-pushed the stataru/fix/KARAF-5014 branch from a30ea9d to 5bef795 Compare November 8, 2024 18:43

stataru8 marked this pull request as draft November 20, 2024 15:07

stataru8 force-pushed the stataru/fix/KARAF-5014 branch from b9efb56 to c7947db Compare April 18, 2025 09:52

KARAF-5014: consider first group role in users.properties and ignore …

2e57742

…empty roles

stataru8 force-pushed the stataru/fix/KARAF-5014 branch 2 times, most recently from 3b86039 to 3c20e27 Compare April 19, 2025 13:33

KARAF-5014: consider first group role in users.properties and ignore …

ea7e507

…empty roles - unit tests

stataru8 force-pushed the stataru/fix/KARAF-5014 branch from 3c20e27 to ea7e507 Compare May 1, 2025 09:33

stataru8 added 2 commits May 1, 2025 11:51

KARAF-5014: consider first group role in users.properties and ignore …

ced63fe

…empty roles - update .adoc files

KARAF-5014: consider empty groups in the output of jaas:user-list

93ecc45

stataru8 commented May 1, 2025

View reviewed changes

stataru8 marked this pull request as ready for review May 1, 2025 16:49

stataru8 marked this pull request as draft May 1, 2025 18:11

KARAF-5014: align properties login modules logic for retrieving group…

67f8200

…s and roles with the underlying baking engine

stataru8 force-pushed the stataru/fix/KARAF-5014 branch from fa2ab89 to 7d542e9 Compare May 2, 2025 16:21

KARAF-5014: make nested groups discoverable

27b9946

stataru8 force-pushed the stataru/fix/KARAF-5014 branch from 7d542e9 to 27b9946 Compare May 2, 2025 17:05

stataru8 marked this pull request as ready for review May 2, 2025 17:44

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

stataru8 commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndreVirtimo commented Sep 27, 2024

Uh oh!

jbonofre commented Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbonofre Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

stataru8 Sep 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jbonofre Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

stataru8 Sep 27, 2024

Choose a reason for hiding this comment

Uh oh!

stataru8 commented Sep 27, 2024

Uh oh!

jbonofre commented Nov 8, 2024

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

stataru8 commented Nov 8, 2024

Uh oh!

jbonofre commented Nov 9, 2024

Uh oh!

AndreVirtimo commented Nov 11, 2024

Uh oh!

jbonofre commented Nov 11, 2024

Uh oh!

stataru8 commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stataru8 commented Nov 20, 2024

Uh oh!

jbonofre commented Nov 20, 2024

Uh oh!

stataru8 commented Apr 19, 2025

Uh oh!

stataru8 May 1, 2025

Choose a reason for hiding this comment

Uh oh!

stataru8 May 1, 2025

Choose a reason for hiding this comment

Uh oh!

stataru8 commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattrpav commented May 8, 2025

Uh oh!

jbonofre commented May 8, 2025

Uh oh!

mattrpav commented Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stataru8 commented Sep 20, 2024 •

edited

Loading

jbonofre commented Sep 27, 2024 •

edited

Loading

stataru8 Sep 27, 2024 •

edited

Loading

stataru8 commented Nov 11, 2024 •

edited

Loading

stataru8 commented May 1, 2025 •

edited

Loading

mattrpav commented Jul 28, 2025 •

edited

Loading