You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2021/10/12 11:00:54 UTC
[GitHub] [knox] moresandeep opened a new pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
moresandeep opened a new pull request #509:
URL: https://github.com/apache/knox/pull/509
## What changes were proposed in this pull request?
This PR tries to address the issue where Knox SSO goes in a redirect loop when the authenticated user is part of too many groups. This happens because Pac4J tries to save all the user entitlements in a profile object that Knox saves as a cookie (for HA deployments). The unfortunate side effect of which is that we are now bound by the cookie size.
This PR introduces the following parameter that can be controlled from knoxsso.xml topology
| Property Name | Default Value | Description |
| ---------------- | ------------- | ------------|
| `pac4j.session.store.exclude.groups` | `true` | Exclude group entitlements from pac4j profile cookie |
| `pac4j.session.store.exclude.roles` | `true` | Exclude roles entitlements from pac4j profile cookie |
| `pac4j.session.store.exclude.permissions` | `true` | Exclude roles permissions from pac4j profile cookie |
Example config from knoxsso.xml
```
<param>
<name>pac4j.session.store.exclude.groups</name>
<value>false</value>
</param>
<param>
<name>pac4j.session.store.exclude.roles</name>
<value>true</value>
</param>
<param>
<name>pac4j.session.store.exclude.permissions</name>
<value>false</value>
</param>
```
## How was this patch tested?
This patch was tested on local Apache Knox instance with Okta
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #509:
URL: https://github.com/apache/knox/pull/509#discussion_r727072226
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/session/KnoxSessionStore.java
##########
@@ -222,6 +240,25 @@ private Object clearUserProfile(final Object value) {
if(value instanceof Map<?,?>) {
final Map<String, CommonProfile> profiles = (Map<String, CommonProfile>) value;
profiles.forEach((name, profile) -> profile.removeLoginData());
+
+ if(sessionStoreConfigs != null &&
Review comment:
```
if (sessionStoreConfigs != null) {
if (sessionStoreConfigs.getOrDefault(PAC4J_SESSION_STORE_EXCLUDE_GROUPS, PAC4J_SESSION_STORE_EXCLUDE_GROUPS_DEFAULT).equalsIgnoreCase("true"))) {
profiles.forEach((name, profile) -> profile.removeAttribute("groups"));
}
...
}
```
I think the code would be cleaner this way but I don't insist on it ;)
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilter.java
##########
@@ -187,6 +200,27 @@ public void init( FilterConfig filterConfig ) throws ServletException {
clientName = CommonHelper.isBlank(clientNameParameter) ? clients.get(0).getName() : clientNameParameter;
+ /* do we need to exclude groups? */
+ if (filterConfig.getInitParameter(PAC4J_SESSION_STORE_EXCLUDE_GROUPS) == null) {
Review comment:
This patter could be replaced w/ a private method:
```
private setSessionStoreConfig(FilterConfig filterConfig, String configName, String configDefault) {
final String configValue = filterConfig.getInitParameter(configName);
sessionStoreConfigs.put(configValue == null ? configDefault : configValue);
}
```
Then you could have the following here:
```
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_GROUPS, PAC4J_SESSION_STORE_EXCLUDE_GROUPS_DEFAULT);
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_ROLES, PAC4J_SESSION_STORE_EXCLUDE_ROLES_DEFAULT);
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_PERMISSIONS, PAC4J_SESSION_STORE_EXCLUDE_PERMISSIONS_DEFAULT);
```
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #509:
URL: https://github.com/apache/knox/pull/509#discussion_r727130966
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilter.java
##########
@@ -187,6 +200,27 @@ public void init( FilterConfig filterConfig ) throws ServletException {
clientName = CommonHelper.isBlank(clientNameParameter) ? clients.get(0).getName() : clientNameParameter;
+ /* do we need to exclude groups? */
+ if (filterConfig.getInitParameter(PAC4J_SESSION_STORE_EXCLUDE_GROUPS) == null) {
Review comment:
+1
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/session/KnoxSessionStore.java
##########
@@ -222,6 +240,25 @@ private Object clearUserProfile(final Object value) {
if(value instanceof Map<?,?>) {
final Map<String, CommonProfile> profiles = (Map<String, CommonProfile>) value;
profiles.forEach((name, profile) -> profile.removeLoginData());
+
+ if(sessionStoreConfigs != null &&
Review comment:
I agree, no need to check sessionStoreConfigs != null multiple times
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep merged pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #509:
URL: https://github.com/apache/knox/pull/509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep merged pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #509:
URL: https://github.com/apache/knox/pull/509
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #509:
URL: https://github.com/apache/knox/pull/509#discussion_r727068387
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilter.java
##########
@@ -187,6 +200,27 @@ public void init( FilterConfig filterConfig ) throws ServletException {
clientName = CommonHelper.isBlank(clientNameParameter) ? clients.get(0).getName() : clientNameParameter;
+ /* do we need to exclude groups? */
+ if (filterConfig.getInitParameter(PAC4J_SESSION_STORE_EXCLUDE_GROUPS) == null) {
Review comment:
This pattern could be replaced w/ a private method:
```
private setSessionStoreConfig(FilterConfig filterConfig, String configName, String configDefault) {
final String configValue = filterConfig.getInitParameter(configName);
sessionStoreConfigs.put(configValue == null ? configDefault : configValue);
}
```
Then you could have the following here:
```
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_GROUPS, PAC4J_SESSION_STORE_EXCLUDE_GROUPS_DEFAULT);
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_ROLES, PAC4J_SESSION_STORE_EXCLUDE_ROLES_DEFAULT);
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_PERMISSIONS, PAC4J_SESSION_STORE_EXCLUDE_PERMISSIONS_DEFAULT);
```
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #509:
URL: https://github.com/apache/knox/pull/509#discussion_r727137704
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/session/KnoxSessionStore.java
##########
@@ -222,6 +240,25 @@ private Object clearUserProfile(final Object value) {
if(value instanceof Map<?,?>) {
final Map<String, CommonProfile> profiles = (Map<String, CommonProfile>) value;
profiles.forEach((name, profile) -> profile.removeLoginData());
+
+ if(sessionStoreConfigs != null &&
Review comment:
+1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] moresandeep commented on a change in pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #509:
URL: https://github.com/apache/knox/pull/509#discussion_r727135022
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilter.java
##########
@@ -187,6 +200,27 @@ public void init( FilterConfig filterConfig ) throws ServletException {
clientName = CommonHelper.isBlank(clientNameParameter) ? clients.get(0).getName() : clientNameParameter;
+ /* do we need to exclude groups? */
+ if (filterConfig.getInitParameter(PAC4J_SESSION_STORE_EXCLUDE_GROUPS) == null) {
Review comment:
Thanks @smolnar82 this is much better!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #509:
URL: https://github.com/apache/knox/pull/509#discussion_r727068387
##########
File path: gateway-provider-security-pac4j/src/main/java/org/apache/knox/gateway/pac4j/filter/Pac4jDispatcherFilter.java
##########
@@ -187,6 +200,27 @@ public void init( FilterConfig filterConfig ) throws ServletException {
clientName = CommonHelper.isBlank(clientNameParameter) ? clients.get(0).getName() : clientNameParameter;
+ /* do we need to exclude groups? */
+ if (filterConfig.getInitParameter(PAC4J_SESSION_STORE_EXCLUDE_GROUPS) == null) {
Review comment:
This pattern could be replaced w/ a private method:
```
private void setSessionStoreConfig(FilterConfig filterConfig, String configName, String configDefault) {
final String configValue = filterConfig.getInitParameter(configName);
sessionStoreConfigs.put(configValue == null ? configDefault : configValue);
}
```
Then you could have the following here:
```
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_GROUPS, PAC4J_SESSION_STORE_EXCLUDE_GROUPS_DEFAULT);
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_ROLES, PAC4J_SESSION_STORE_EXCLUDE_ROLES_DEFAULT);
setSessionStoreConfig(PAC4J_SESSION_STORE_EXCLUDE_PERMISSIONS, PAC4J_SESSION_STORE_EXCLUDE_PERMISSIONS_DEFAULT);
```
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on pull request #509: KNOX-2679 - Remove groups, roles and permissions from SAML profile object for pac4j cookie to save space
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on pull request #509:
URL: https://github.com/apache/knox/pull/509#issuecomment-940905791
Cc. @zeroflag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org