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/04/20 10:50:03 UTC
[GitHub] [knox] moresandeep commented on a diff in pull request #560: KNOX-2732 Issuer claim in Knox JWTs should be configurable
moresandeep commented on code in PR #560:
URL: https://github.com/apache/knox/pull/560#discussion_r853988760
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTAccessTokenAssertionFilter.java:
##########
@@ -68,6 +70,10 @@ public void init( FilterConfig filterConfig ) throws ServletException {
GatewayServices services = (GatewayServices) filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
authority = services.getService(ServiceType.TOKEN_SERVICE);
sr = services.getService(ServiceType.SERVICE_REGISTRY_SERVICE);
+
+ this.tokenIssuer = filterConfig.getInitParameter(JWTAccessTokenAssertionFilter.ISSUER) != null
Review Comment:
We should check for a blank string too.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTAccessTokenAssertionFilter.java:
##########
@@ -141,7 +147,13 @@ private String getAccessToken(final String principalName, String serviceName, lo
JWT token;
try {
- final JWTokenAttributes jwtAttributes = new JWTokenAttributesBuilder().setUserName(principalName).setAudiences(serviceName).setAlgorithm(signatureAlgorithm).setExpires(expires).build();
+ final JWTokenAttributes jwtAttributes = new JWTokenAttributesBuilder()
Review Comment:
Thanks :)
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTAuthCodeAssertionFilter.java:
##########
@@ -55,6 +57,10 @@ public void init( FilterConfig filterConfig ) throws ServletException {
GatewayServices services = (GatewayServices) filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
authority = services.getService(ServiceType.TOKEN_SERVICE);
sr = services.getService(ServiceType.SERVICE_REGISTRY_SERVICE);
+
+ this.tokenIssuer = filterConfig.getInitParameter(JWTAccessTokenAssertionFilter.ISSUER) != null
+ ? filterConfig.getInitParameter(JWTAccessTokenAssertionFilter.ISSUER)
Review Comment:
As per RFC https://datatracker.ietf.org/doc/html/rfc7519#section-4.1.1 issuer is a `principal` and not principals but could there be cases where we might need to support multiple issuers? if not we are golden. Just wanted to raise this.
##########
gateway-service-knoxtoken/src/main/java/org/apache/knox/gateway/service/knoxtoken/TokenResource.java:
##########
@@ -242,6 +243,9 @@ public void init() throws AliasServiceException, ServiceLifecycleException, KeyL
? true
: Boolean.parseBoolean(includeGroupsInTokenAllowedParam);
+ this.tokenIssuer = context.getInitParameter(KNOX_TOKEN_ISSUER) != null
Review Comment:
Check for blank string.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTAuthCodeAssertionFilter.java:
##########
@@ -42,6 +43,7 @@ public class JWTAuthCodeAssertionFilter extends AbstractIdentityAssertionFilter
private JWTokenAuthority authority;
private ServiceRegistry sr;
+ private String tokenIssuer;
@Override
public void init( FilterConfig filterConfig ) throws ServletException {
Review Comment:
Not related to this change but we have some code duplication here with `JWTAccessTokenAssertionFilter` We can prolly combine it into a common base class. Not required here but would be nice.
##########
gateway-provider-security-jwt/src/main/java/org/apache/knox/gateway/provider/federation/jwt/filter/JWTAuthCodeAssertionFilter.java:
##########
@@ -55,6 +57,10 @@ public void init( FilterConfig filterConfig ) throws ServletException {
GatewayServices services = (GatewayServices) filterConfig.getServletContext().getAttribute(GatewayServices.GATEWAY_SERVICES_ATTRIBUTE);
authority = services.getService(ServiceType.TOKEN_SERVICE);
sr = services.getService(ServiceType.SERVICE_REGISTRY_SERVICE);
+
+ this.tokenIssuer = filterConfig.getInitParameter(JWTAccessTokenAssertionFilter.ISSUER) != null
Review Comment:
We should check for blank string.
--
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