KNOX-3350: Populate groups in KnoxSSO cookie when configured

[smolnar82]

KNOX-3350 - Add group information into the generated JWT in WebSSOResource

What changes were proposed in this pull request?

This PR introduces the ability to include group information in the JWT tokens generated by WebSSOResource (KnoxSSO).

Key changes include:

• Added a new configuration parameter knoxsso.token.include.groups to control whether group information should be included in the issued JWT.
• Updated getAuthenticationToken in WebSSOResource to populate the groups claim in JWTokenAttributes when the feature is enabled.
• Standardized group retrieval in TokenResource by utilizing SubjectUtils.getCurrentGroupPrincipalNames().

How was this patch tested?

The changes were verified by adding comprehensive unit tests in WebSSOResourceTest:

• testIncludeGroupsTrue: Verifies that groups are correctly included in the JWT when knoxsso.token.include.groups is set to true.
• testIncludeGroupsFalse: Verifies that groups are excluded when the parameter is set to false.
• testIncludeGroupsOmitted: Verifies that the default behavior (when the parameter is missing) is to exclude groups.

Integration Tests

I ran manual testing using my local Knox instance against the knoxsso topology. I logged in as recursiveUser, verified hadoop-jwt (extracted from DEV tools in Chrome) is generated, then checked its content on jwt.io:

1. Without knoxsso.token.include.groups:

2. knoxsso.token.include.groups = false:

3. knoxsso.token.include.groups = true:

UI changes

N/A

[github-actions]

Test Results

22 tests   22 ✅  2s ⏱️
 1 suites   0 💤
 1 files     0 ❌

Results for commit 43eeff0.

[moresandeep]

@smolnar82 did you check https://issues.apache.org/jira/browse/KNOX-2679?
AFAIK we already have support for groups but it is not hooked up to JWT. We have a param to turn off groups pac4j.session.store.exclude.groups

Looks like this PR includes groups from java Subject. For SSO groups are populated from SAML right? can you elaborate on how this will Subject groups are populated.

I think we should also support hooking up SAML groups here if pac4j.session.store.exclude.groups=false (currently they only show up in pac4j profile cookie)

[smolnar82]

@moresandeep - Thanks for your review; I replied back to the default value above.

Subject. For SSO groups are populated from SAML right? can you elaborate on how this will Subject groups are populated.

Nope. SAML is one way to authenticate. But we do support other authN mechanisms, such as LDAP.
In case of LDAP, Knox needs to be configured with the HadoopGroupProvider for group lookup. If that's configured, Knox will place GroupPrincipal items in the current Subject during request processing flow. Ultimately, we arrive to the KNOXSSO service (WebSSOResource in the codebase), which is a terminating-service (i.e. non-proxying), but at this phase the Subject is already decorated and we can read what groups were resolved by Knox.

[moresandeep]

@moresandeep - Thanks for your review; I replied back to the default value above.

Subject. For SSO groups are populated from SAML right? can you elaborate on how this will Subject groups are populated.

Nope. SAML is one way to authenticate. But we do support other authN mechanisms, such as LDAP. In case of LDAP, Knox needs to be configured with the HadoopGroupProvider for group lookup. If that's configured, Knox will place GroupPrincipal items in the current Subject during request processing flow. Ultimately, we arrive to the KNOXSSO service (WebSSOResource in the codebase), which is a terminating-service (i.e. non-proxying), but at this phase the Subject is already decorated and we can read what groups were resolved by Knox.

Ahh, i see, that makes sense, don't you think we should also support SAML and not just LDAP? IMO we should support a way to fetch groups from SAML too if we are supporting fetching groups from other ways too. There have been internal requests for this.

[lmccay]

Please note that the reason we have an option to turn off groups in the cookie is that the cookies get too big and SSO will therefore not work. I don't actually think we should be passing all of the users' group memberships around on every request from the browser and allow the to get stale based on the TTL/expiry of the token and cookie. Also, we shouldn't be getting group memberships from different sources in a deployment. If we get this from the SSO SAML assertion and others are using LDAP Proxy Server then there could be divergence. Overall, I am more -1 on this as a change but if someone wants to use it and they make sure that all group lookup gets the same groups then we can add it but I think that it needs to be disabled by default.

[smolnar82]

@lmccay made a point: cookie size can be an issue.

Modern browsers support cookies up to 4K:

Which opens the following question: shall we include groups only, and only if, when the cummulated cookie size doesn't exceed 4K?

[lmccay]

@lmccay made a point: cookie size can be an issue.

Modern browsers support cookies up to 4K: Which opens the following question: shall we include groups only, and only if, when the cummulated cookie size doesn't exceed 4K?

Well, it isn't really clear to me where those groups are even going to be used and what authorization check will see them.
Are we going to change JWTFederationFilter to extract them from the token and set them as GroupPrincipals?

Depending on the specific need, we may be able to handle this in another way.
We already have the ability to add a header for groups to a dispatched request, if this usecase in question here is for a proxied service that wants to get groups from Knox.

[moresandeep]

If we decide to go with this PR, we need to add a filter for groups something like preauth.group.filter.pattern to prevent issues like KNOX-2679

[smolnar82]

Depending on the specific need, we may be able to handle this in another way.
We already have the ability to add a header for groups to a dispatched request, if this usecase in question here is for a proxied service that wants to get groups from Knox.

That's a good idea. Let's figure that out...
I'll keep you posted.