You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/10/19 18:05:54 UTC
[GitHub] [druid] a2l007 opened a new pull request, #13242: Add JWT authenticator support for validating ID Tokens
a2l007 opened a new pull request, #13242:
URL: https://github.com/apache/druid/pull/13242
### Description
Expands the OIDC based auth in Druid by adding a JWT Authenticator that validates [ID Tokens](https://openid.net/specs/openid-connect-core-1_0.html#CodeIDToken) associated with a request. The existing pac4j authenticator works for authenticating web users while accessing the console, whereas this authenticator is for validating Druid API requests made by Direct clients. Services already supporting OIDC can attach their ID tokens to the Druid requests
under the `Authorization` request header.
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "a2l007 (via GitHub)" <gi...@apache.org>.
a2l007 commented on PR #13242:
URL: https://github.com/apache/druid/pull/13242#issuecomment-1483131161
Sorry for the delay @abhishekagarwal87
I've addressed your comments
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nlippis commented on a diff in pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by GitBox <gi...@apache.org>.
nlippis commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1012290118
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/OIDCConfig.java:
##########
@@ -26,6 +26,7 @@
public class OIDCConfig
{
+ private final String DEFAULT_SCOPE = "name";
Review Comment:
While it is a standard claim, it isn't mandatory. Some OIDC compliant auth servers don't include that claim in the ID token even when the `profile` scope is requested.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a diff in pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "a2l007 (via GitHub)" <gi...@apache.org>.
a2l007 commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1147852631
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
+
+ if (claim.isPresent()) {
+ LOG.debug("Authentication successful for " + oidcConfig.getClientID());
+ AuthenticationResult authenticationResult = new AuthenticationResult(
+ claim.get(),
+ authorizerName,
+ name,
+ null
+ );
+ servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
+ } else {
+ LOG.error(
+ "Authentication failed! Please ensure the validity of the ID token and contains the configured claim.");
+ }
+ }
+ }
+ catch (BadJOSEException | JOSEException | ParseException e) {
+ LOG.error(e, "Failed to parse JWT token");
+ }
+ }
+ filterChain.doFilter(servletRequest, servletResponse);
+ }
+
+
+ @Override
+ public void destroy()
+ {
+
+ }
+
+ private static Optional<String> extractBearerToken(HttpServletRequest request)
+ {
+ String header = request.getHeader(HttpConstants.AUTHORIZATION_HEADER);
+ if (header == null || !header.startsWith(HttpConstants.BEARER_HEADER_PREFIX)) {
+ LOG.debug("Invalid prefix for Bearer token");
Review Comment:
Fixed the error message here.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1130685603
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
+
+ if (claim.isPresent()) {
+ LOG.debug("Authentication successful for " + oidcConfig.getClientID());
+ AuthenticationResult authenticationResult = new AuthenticationResult(
+ claim.get(),
+ authorizerName,
+ name,
+ null
+ );
+ servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
+ } else {
+ LOG.error(
+ "Authentication failed! Please ensure the validity of the ID token and contains the configured claim.");
+ }
+ }
+ }
+ catch (BadJOSEException | JOSEException | ParseException e) {
+ LOG.error(e, "Failed to parse JWT token");
+ }
+ }
+ filterChain.doFilter(servletRequest, servletResponse);
+ }
+
+
+ @Override
+ public void destroy()
+ {
+
+ }
+
+ private static Optional<String> extractBearerToken(HttpServletRequest request)
+ {
+ String header = request.getHeader(HttpConstants.AUTHORIZATION_HEADER);
+ if (header == null || !header.startsWith(HttpConstants.BEARER_HEADER_PREFIX)) {
+ LOG.debug("Invalid prefix for Bearer token");
Review Comment:
it doesn't really mean that prefix is invalid. It could be that this is not the right scheme.
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
+
+ if (claim.isPresent()) {
+ LOG.debug("Authentication successful for " + oidcConfig.getClientID());
+ AuthenticationResult authenticationResult = new AuthenticationResult(
+ claim.get(),
+ authorizerName,
+ name,
+ null
+ );
+ servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
+ } else {
+ LOG.error(
+ "Authentication failed! Please ensure the validity of the ID token and contains the configured claim.");
Review Comment:
should we not reject the request at this point? If an id token is present, any error should result in an error. isn't it?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a diff in pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "a2l007 (via GitHub)" <gi...@apache.org>.
a2l007 commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1147864938
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
+
+ if (claim.isPresent()) {
+ LOG.debug("Authentication successful for " + oidcConfig.getClientID());
+ AuthenticationResult authenticationResult = new AuthenticationResult(
+ claim.get(),
+ authorizerName,
+ name,
+ null
+ );
+ servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
+ } else {
+ LOG.error(
+ "Authentication failed! Please ensure the validity of the ID token and contains the configured claim.");
Review Comment:
My initial thought was to not break the authentication chain in case the user has any other custom authenticators further down the chain. But i see that if the request already has an id token, they have signaled intent to use that to authenticate.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "a2l007 (via GitHub)" <gi...@apache.org>.
a2l007 commented on PR #13242:
URL: https://github.com/apache/druid/pull/13242#issuecomment-1448512773
> @a2l007 - is there more work to be done for this PR?
@abhishekagarwal87 No, this is good to go. There will be some work on this extension in a future PR when we upgrade the pac4j library dependencies. I haven't upgraded them yet since the newer versions are not backward compatible with JDK 8.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add JWT authenticator support for validating ID Tokens (druid)
Posted by "xvrl (via GitHub)" <gi...@apache.org>.
xvrl commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1294981564
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ HttpServletResponse httpServletResponse = (HttpServletResponse) servletResponse;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
+
+ if (claim.isPresent()) {
Review Comment:
this is always true since Optional.of will throw an NPE if claims.getStringClaim return null
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add JWT authenticator support for validating ID Tokens (druid)
Posted by "a2l007 (via GitHub)" <gi...@apache.org>.
a2l007 commented on PR #13242:
URL: https://github.com/apache/druid/pull/13242#issuecomment-1684333831
@xvrl Thanks for the catch, I've raised a PR #14872 to fix it
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] nlippis commented on a diff in pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by GitBox <gi...@apache.org>.
nlippis commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1003881341
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/OIDCConfig.java:
##########
@@ -26,6 +26,7 @@
public class OIDCConfig
{
+ private final String DEFAULT_SCOPE = "name";
Review Comment:
The default claim to use for the identity is `sub` since `name` is not a standard OIDC claim
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/OIDCConfig.java:
##########
@@ -35,16 +36,21 @@
@JsonProperty
private final String discoveryURI;
+ @JsonProperty
+ private final String oidcClaim;
+
@JsonCreator
public OIDCConfig(
@JsonProperty("clientID") String clientID,
@JsonProperty("clientSecret") PasswordProvider clientSecret,
- @JsonProperty("discoveryURI") String discoveryURI
+ @JsonProperty("discoveryURI") String discoveryURI,
+ @JsonProperty("oidcClaim") String oidcClaim
Review Comment:
Since this parameter is used to specify an arbitrary claim value as the Druid AuthenticationResult's identity, perhaps we can give it a name such as `identityClaim`
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a diff in pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1130683799
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
+
+ if (claim.isPresent()) {
+ LOG.debug("Authentication successful for " + oidcConfig.getClientID());
+ AuthenticationResult authenticationResult = new AuthenticationResult(
+ claim.get(),
+ authorizerName,
+ name,
+ null
+ );
+ servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
+ } else {
+ LOG.error(
+ "Authentication failed! Please ensure the validity of the ID token and contains the configured claim.");
Review Comment:
```suggestion
"Authentication failed! Please ensure that ID token is valid and it contains the configured claim.");
```
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add JWT authenticator support for validating ID Tokens (druid)
Posted by "xvrl (via GitHub)" <gi...@apache.org>.
xvrl commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1294981039
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ HttpServletResponse httpServletResponse = (HttpServletResponse) servletResponse;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
Review Comment:
Did you mean to use Optional.ofNullable here instead of Optional.of? This will currently throw an NPE if the claims are null.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] a2l007 commented on a diff in pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by GitBox <gi...@apache.org>.
a2l007 commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1008319110
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/OIDCConfig.java:
##########
@@ -26,6 +26,7 @@
public class OIDCConfig
{
+ private final String DEFAULT_SCOPE = "name";
Review Comment:
`name` is a standard claim according to the [OIDC spec](https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims).
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Add JWT authenticator support for validating ID Tokens (druid)
Posted by "xvrl (via GitHub)" <gi...@apache.org>.
xvrl commented on code in PR #13242:
URL: https://github.com/apache/druid/pull/13242#discussion_r1294982361
##########
extensions-core/druid-pac4j/src/main/java/org/apache/druid/security/pac4j/JwtAuthFilter.java:
##########
@@ -0,0 +1,129 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.security.pac4j;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.proc.BadJOSEException;
+import com.nimbusds.jwt.JWTParser;
+import com.nimbusds.openid.connect.sdk.claims.IDTokenClaimsSet;
+import org.apache.druid.java.util.common.logger.Logger;
+import org.apache.druid.server.security.AuthConfig;
+import org.apache.druid.server.security.AuthenticationResult;
+import org.pac4j.core.context.HttpConstants;
+import org.pac4j.oidc.profile.creator.TokenValidator;
+
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.text.ParseException;
+import java.util.Optional;
+
+public class JwtAuthFilter implements Filter
+{
+ private static final Logger LOG = new Logger(JwtAuthFilter.class);
+
+ private final String authorizerName;
+ private final String name;
+ private final OIDCConfig oidcConfig;
+ private final TokenValidator tokenValidator;
+
+ public JwtAuthFilter(String authorizerName, String name, OIDCConfig oidcConfig, TokenValidator tokenValidator)
+ {
+ this.authorizerName = authorizerName;
+ this.name = name;
+ this.oidcConfig = oidcConfig;
+ this.tokenValidator = tokenValidator;
+ }
+
+ @Override
+ public void init(FilterConfig filterConfig)
+ {
+
+ }
+
+ @Override
+ public void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+ throws IOException, ServletException
+ {
+ // Skip this filter if the request has already been authenticated
+ if (servletRequest.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) != null) {
+ filterChain.doFilter(servletRequest, servletResponse);
+ return;
+ }
+
+ HttpServletRequest httpServletRequest = (HttpServletRequest) servletRequest;
+ HttpServletResponse httpServletResponse = (HttpServletResponse) servletResponse;
+ Optional<String> idToken = extractBearerToken(httpServletRequest);
+
+ if (idToken.isPresent()) {
+ try {
+ // Parses the JWT and performs the ID Token validation specified in the OpenID spec: https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
+ IDTokenClaimsSet claims = tokenValidator.validate(JWTParser.parse(idToken.get()), null);
+ if (claims != null) {
+ Optional<String> claim = Optional.of(claims.getStringClaim(oidcConfig.getOidcClaim()));
+
+ if (claim.isPresent()) {
+ LOG.debug("Authentication successful for " + oidcConfig.getClientID());
+ AuthenticationResult authenticationResult = new AuthenticationResult(
+ claim.get(),
+ authorizerName,
+ name,
+ null
+ );
+ servletRequest.setAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT, authenticationResult);
+ } else {
+ LOG.error(
+ "Authentication failed! Please ensure that the ID token is valid and it contains the configured claim.");
+ httpServletResponse.sendError(HttpServletResponse.SC_UNAUTHORIZED);
+ return;
+ }
Review Comment:
it sounds like we are missing a test case that handles the invalid claims code path, since this will never be executed as it is.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 merged pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 merged PR #13242:
URL: https://github.com/apache/druid/pull/13242
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #13242: Add JWT authenticator support for validating ID Tokens
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on PR #13242:
URL: https://github.com/apache/druid/pull/13242#issuecomment-1448304893
@a2l007 - is there more work to be done for this PR?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org