KNOX-2961 - Knox SSO cookie Invalidation - Phase I #797
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smolnar82
force-pushed
the
KNOX-2961
branch
3 times, most recently
from
4b17ffa
to
2865226
Compare
smolnar82
force-pushed
the
KNOX-2961
branch
3 times, most recently
from
800bb03
to
8f3f220
Compare
zeroflag
requested changes
Oct 2, 2023
|
|
||
| private final Set<SessionInvalidator> sessionInvalidators = new HashSet<>(); | ||
|
|
||
| public void registerSessionInvalidator(SessionInvalidator sessionInvalidator) { |
There was a problem hiding this comment.
What happens after a topology undeployent? Do we need to unregister the previously registered invalidator?
There was a problem hiding this comment.
Hmm...that's a good question. I'd believe the previously registered one will be destroyed by Jetty, but it's worth a try to see if this really happens.
| SessionInvalidators.KNOX_SSO_INVALIDATOR.getSessionInvalidators().forEach(sessionInvalidator -> { | ||
| sessionInvalidator.onAuthenticationError(request, response); | ||
| }); | ||
| final boolean doGlobalLogout = request.getAttribute("doGlobalLogout") == null ? false |
There was a problem hiding this comment.
doGlobalLogout should be a constant, it is used in Pac4jDispatcherFilter too.
| if (!disabledKnoxSsoCookies.isEmpty()) { | ||
| log.removingDisabledKnoxSsoCookiesFromDatabase(disabledKnoxSsoCookies.size(), | ||
| String.join(", ", disabledKnoxSsoCookies.stream().map(tokenId -> Tokens.getTokenIDDisplayText(tokenId)).collect(Collectors.toSet()))); | ||
| for (String tokenId : disabledKnoxSsoCookies) { |
There was a problem hiding this comment.
How many tokens do we expect to have normally? If it's a lot then maybe having a dedicated sql delete statement would be better.
There was a problem hiding this comment.
Good argument. I also had the same idea but I wanted to have the PR out ASAP so that you guys can review it. Let me submit a new PS with the updated DELETE statement.
pzampino
reviewed
Oct 2, 2023
...rver/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
Outdated
Show resolved
Hide resolved
...rver/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
Outdated
Show resolved
Hide resolved
...-server/src/main/java/org/apache/knox/gateway/services/token/impl/JDBCTokenStateService.java
Outdated
Show resolved
Hide resolved
...way-server/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateDatabase.java
Outdated
Show resolved
Hide resolved
...ver/src/main/java/org/apache/knox/gateway/services/token/impl/TokenStateServiceMessages.java
Show resolved
Hide resolved
gateway-spi/src/main/java/org/apache/knox/gateway/session/SessionInvalidator.java
Show resolved
Hide resolved
stoty
pushed a commit
to stoty/knox
that referenced
this pull request
May 14, 2024
…#797) Change-Id: I67a65224dfac4178e5aedd187a3a1cdafb162727
stoty
pushed a commit
to stoty/knox
that referenced
this pull request
May 14, 2024
…into cdpd-master * changes: CDPD-62588, KNOX-2972: Session resource can generate application logout URL with profile/topologies query parameters (apache#808) CDPD-62595, KNOX-2970: Removing KnoxSSO cookie from the token state service upon logout (apache#806) CDPD-62598, KNOX-2971: Applying word wrapping in the comment and metadata columns on the Token Management UI (apache#807) CDPD-62592, KNOX-2969: KnoxSSO Cookies should be ignored while calculating token limit per user (apache#805) CDPD-62585, KNOX-2968: Batch token enable action should succeed even if enabled KnoxSSO cookies are selected (apache#804) CDPD-61809, KNOX-2961: Knox SSO cookie Invalidation - Phase II (apache#799) CDPD-61184, KNOX-2961: Knox SSO cookie Invalidation - Phase I (apache#797)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
The
KNOXSSOservice is modified in a way such that it saves the generated SSO cookie using Knox's token state service capabilities in case token management is enabled inKNOXSSO's configuration (using the well-knownknox.token.exp.server-managed=trueparameter).This is only the SSO cookie generation side of the feature. The verification side also needs to be configured the same way: the
SSOCookieProviderconfiguration must have the same parameter to enable this new feature.In addition to the save/verify changes, the Token Management page is updated:
How was this patch tested?
Manually tested using PAM, LDAP, and PAC4J (SAML2 and OIDC) authentication mechanisms. I flipped the
knox.token.exp.server-managedtotrueinknoxsso,homepage, andmanagertopologies.Logged into the Knox home page (contains some valid UI links in the sandbox topology) as well as opened the Token Management and Token Generation pages. I also opened a private window with the Token Management page to mimic a system administrator (a superuser who has the power to disable tokens for other users).
Confirmed that I can proxy some UIs from the Knox Home page and could generate manage tokens as usual.
Then, in the private window, I disabled the previously generated SSO cookie for my session in the non-private window and confirmed that I was redirected to the Knox login page or to my configured global logout redirect in case of Pac4j authentication (for this feature to work with Pac4j, the
knox.global.logout.page.urlconfiguration is a must-have parameter ingateway-site.xml).This is how the Token Management page looks like with my new changes:
Important note: as the commit message suggests, this is only the 1st phase of the job. In the 2nd one, two new improvements are coming: