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

[stataru8]

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]

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]

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

A group can be empty without problem though.

[stataru8]

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]

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.

[stataru8]

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: https://github.com/apache/karaf/blob/a8d990102fd5181bcb8e23ef8fe06a5782db9d0a/jaas/modules/src/main/java/org/apache/karaf/jaas/modules/publickey/PublickeyBackingEngine.java#L273

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]

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

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

[AndreVirtimo]

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]

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

[stataru8]

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]

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]

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

[stataru8]

Hello again, one more question, should the PR also include changes to the .adoc files? https://github.com/apache/karaf/blob/ad427cd12543dc78e095bbaa4608d7ca3d5ea4d8/manual/src/main/asciidoc/user-guide/security.adoc?plain=1#L142

[stataru8]

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

[mattrpav]

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]

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

[mattrpav]

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?