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 2022/05/18 11:50:24 UTC
[GitHub] [knox] moresandeep opened a new pull request, #579: KNOX-2726 - Impersonation Params should be configurable
moresandeep opened a new pull request, #579:
URL: https://github.com/apache/knox/pull/579
## What changes were proposed in this pull request?
Impersonation Params should be configurable so that Knox can scrub these params before sending it out to the backend services. There could be cases where some services might use their own impersonation params (similar to doAs used by Knox). Including these parameters in request could have unknown side effects. In order to prevent these issue, Impersonation Params should be configurable.
The PR proposes adding `impersonation.params` parameter to the `identity-assertion` provider. e.g.
```
<provider>
<role>identity-assertion</role>
<name>Default</name>
<enabled>true</enabled>
<param>
<name>impersonation.params</name>
<value>customID</value>
</param>
</provider>
```
Adding this parameter tells Knox to scrub this parameter from the dispatched request.
e.g.
```
2022-05-18 07:06:12,733 cde0df3d-5fc8-4b7a-8b96-0f828f4e524e WARN knox.gateway (IdentityAsserterHttpServletRequestWrapper.java:scrubOfExistingPrincipalParams(212)) - Possible identity spoofing attempt - impersonation parameter removed: customID
```
## How was this patch tested?
This patch was tested locally
Logs without `impersonation.params`
```
2022-05-18 07:11:02,332 aadcfc0f-5159-4c50-bacb-0308a496ad32 DEBUG knox.gateway (UrlRewriteProcessor.java:rewrite(162)) - Rewrote URL: https://localhost:8443/gateway/sandbox/weather/data/2.5/forecast/city?id=524901&APPID=54557732afcfe106bfc955b9da04fb14&customID=somerandomestring, direction: IN via implicit rule: WEATHER/weather/inbound to URL: http://api.openweathermap.org:80/data/2.5/forecast/city?id=524901&APPID=54557732afcfe106bfc955b9da04fb14&customID=somerandomestring
2022-05-18 07:11:02,333 aadcfc0f-5159-4c50-bacb-0308a496ad32 DEBUG knox.gateway (DefaultDispatch.java:executeOutboundRequest(157)) - Dispatch request: GET http://api.openweathermap.org:80/data/2.5/forecast/city?id=524901&APPID=54557732afcfe106bfc955b9da04fb14&customID=somerandomestring&user.name=guest
```
Logs with `impersonation.params`
```
557732afcfe106bfc955b9da04fb14&customID=somerandomestring, direction: IN via implicit rule: WEATHER/weather/inbound to URL: http://api.openweathermap.org:80/data/2.5/forecast/city?id=524901&APPID=54557732afcfe106bfc955b9da04fb14&customID=somerandomestring
2022-05-18 07:06:12,733 cde0df3d-5fc8-4b7a-8b96-0f828f4e524e WARN knox.gateway (IdentityAsserterHttpServletRequestWrapper.java:scrubOfExistingPrincipalParams(212)) - Possible identity spoofing attempt - impersonation parameter removed: customID
2022-05-18 07:06:12,738 cde0df3d-5fc8-4b7a-8b96-0f828f4e524e DEBUG knox.gateway (DefaultDispatch.java:executeOutboundRequest(157)) - Dispatch request: GET http://api.openweathermap.org:80/data/2.5/forecast/city?id=524901&APPID=54557732afcfe106bfc955b9da04fb14&user.name=guest
```
--
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] risdenk commented on a diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
risdenk commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878477770
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
+ <artifactId>shrinkwrap-descriptors-api-javaee</artifactId>
+ <scope>compile</scope>
Review Comment:
Don't set scope to compile. Not needed by default. Thats why you don't see `<scope>compile</scope>` anywhere else.
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878440093
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
ahh, let me change it to test scope and see if it runs.
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878393585
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
Actually, this is compile time, that's why I added scope compile.
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878391668
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
Good point, I don't know, let me check.
--
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] zeroflag commented on a diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878096155
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
Do we only use these dependencies from the test? If yes, can we change the scope?
##########
gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java:
##########
@@ -55,9 +56,16 @@ public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilt
public static final String GROUP_PRINCIPAL_MAPPING = "group.principal.mapping";
public static final String PRINCIPAL_MAPPING = "principal.mapping";
+ /* Service specific impersonation params that needs to be scrubbed */
+ public static final String IMPERSONATION_PARAMS = "impersonation.params";
Review Comment:
Can't we use the same constant here that is defined in the abstract class?
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878446759
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
Has to be compile time
```
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 20.275 s
[INFO] Finished at: 2022-05-20T14:30:24-04:00
[INFO] ------------------------------------------------------------------------
[WARNING] The requested profile "install" could not be activated because it does not exist.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project gateway-provider-identity-assertion-common: Compilation failure
[ERROR] /knox/gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/AbstractIdentityAsserterDeploymentContributor.java:[89,32] cannot access org.jboss.shrinkwrap.descriptor.api.webapp30.WebAppDescriptor
[ERROR] class file for org.jboss.shrinkwrap.descriptor.api.webapp30.WebAppDescriptor not found
```
--
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 #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep merged PR #579:
URL: https://github.com/apache/knox/pull/579
--
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] zeroflag commented on a diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878427534
##########
gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java:
##########
@@ -55,9 +56,16 @@ public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilt
public static final String GROUP_PRINCIPAL_MAPPING = "group.principal.mapping";
public static final String PRINCIPAL_MAPPING = "principal.mapping";
+ /* Service specific impersonation params that needs to be scrubbed */
+ public static final String IMPERSONATION_PARAMS = "impersonation.params";
Review Comment:
@smolnar82 there is already a constant like this in AbstractIdentityAsserterDeploymentContributor.java
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878391153
##########
gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java:
##########
@@ -55,9 +56,16 @@ public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilt
public static final String GROUP_PRINCIPAL_MAPPING = "group.principal.mapping";
public static final String PRINCIPAL_MAPPING = "principal.mapping";
+ /* Service specific impersonation params that needs to be scrubbed */
+ public static final String IMPERSONATION_PARAMS = "impersonation.params";
Review Comment:
@zeroflag which constant are you referring to? or are you suggesting moving this to `AbstractIdentityAssertionFilter`
--
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] zeroflag commented on a diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878434087
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
Isn't compile the default scope in maven? I thought if we don't specify any scope it will be compile. My suggestion was to change it to test scope, if we only use this library from the test code.
--
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] zeroflag commented on a diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878449374
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
ah ok, thanks for trying
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878446759
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
Review Comment:
Can't be test, needed for compilation.
```
[INFO] 1 error
[INFO] -------------------------------------------------------------
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 20.275 s
[INFO] Finished at: 2022-05-20T14:30:24-04:00
[INFO] ------------------------------------------------------------------------
[WARNING] The requested profile "install" could not be activated because it does not exist.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.1:compile (default-compile) on project gateway-provider-identity-assertion-common: Compilation failure
[ERROR] /knox/gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/AbstractIdentityAsserterDeploymentContributor.java:[89,32] cannot access org.jboss.shrinkwrap.descriptor.api.webapp30.WebAppDescriptor
[ERROR] class file for org.jboss.shrinkwrap.descriptor.api.webapp30.WebAppDescriptor not found
```
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878701940
##########
gateway-provider-identity-assertion-common/pom.xml:
##########
@@ -85,8 +85,23 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-util</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.jboss.shrinkwrap.descriptors</groupId>
+ <artifactId>shrinkwrap-descriptors-api-javaee</artifactId>
+ <scope>compile</scope>
Review Comment:
So this is just to be in-line with other declaration in the POM file?
--
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 diff in pull request #579: KNOX-2726 - Impersonation Params should be configurable
Posted by GitBox <gi...@apache.org>.
moresandeep commented on code in PR #579:
URL: https://github.com/apache/knox/pull/579#discussion_r878432317
##########
gateway-provider-identity-assertion-common/src/main/java/org/apache/knox/gateway/identityasserter/common/filter/CommonIdentityAssertionFilter.java:
##########
@@ -55,9 +56,16 @@ public class CommonIdentityAssertionFilter extends AbstractIdentityAssertionFilt
public static final String GROUP_PRINCIPAL_MAPPING = "group.principal.mapping";
public static final String PRINCIPAL_MAPPING = "principal.mapping";
+ /* Service specific impersonation params that needs to be scrubbed */
+ public static final String IMPERSONATION_PARAMS = "impersonation.params";
Review Comment:
I see, I will change it, thanks!
--
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