You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/09 02:08:43 UTC
[GitHub] [nifi] emiliosetiadarma opened a new pull request, #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
emiliosetiadarma opened a new pull request, #6637:
URL: https://github.com/apache/nifi/pull/6637
…t for NiFi Registry when using OIDC/OAuth 2.0 providers
<!-- 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. -->
# Summary
[NIFI-10177](https://issues.apache.org/jira/browse/NIFI-10177)
- Implemented OAuth 2.0/OIDC logout for NiFi Registry using either revocation endpoint (for OAuth 2.0 providers) or end session endpoints (for OIDC providers).
- Refactored the deletion of the JWT token in the database into the shared `/access/logout` endpoint for both OIDC and non-OIDC logouts.
- Notes on possible follow up work:
1. Add message page to display errors that occur during authentication, the error message page in NiFi registry currently does not exist.
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [x] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 8
- [x] JDK 11
- [x] JDK 17
### Licensing
- [x] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [x] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] thenatog commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
thenatog commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1028535615
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -119,6 +137,7 @@ public AccessResource(
this.oidcService = oidcService;
this.kerberosSpnegoIdentityProvider = kerberosSpnegoIdentityProvider;
this.identityProvider = identityProvider;
+ this.bearerTokenResolver = new DefaultBearerTokenResolver();
Review Comment:
@exceptionfactory I assume we would want to keep using our StandardBearerTokenResolver here, and import the NiFi dependency?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] emiliosetiadarma commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
emiliosetiadarma commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1027555347
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final String callback) {
+ final String oidcRequestIdentifier = UUID.randomUUID().toString();
+ // generate a cookie to associate this login sequence
+ final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER, oidcRequestIdentifier);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse;
+ try {
+ oidcResponse = AuthenticationResponseParser.parse(requestUri);
+ } catch (final ParseException e) {
+ final String loginOrLogoutString = isLogin ? "login" : "logout";
+ logger.error(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+ }
+ return oidcResponse;
+ }
+
+ private void validateOIDCState(final String oidcRequestIdentifier,
+ final AuthenticationSuccessResponse successfulOidcResponse,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ // confirm state
+ final State state = successfulOidcResponse.getState();
+ if (state == null || !oidcService.isStateValid(oidcRequestIdentifier, state)) {
+ final String loginOrLogoutMessage = isLogin ? "login" : "logout";
+ logger.error(String.format("The state value returned by the OpenId Connect Provider does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Purposed state does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
Review Comment:
Changing
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] emiliosetiadarma commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
emiliosetiadarma commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1027390046
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java:
##########
@@ -175,6 +175,20 @@ public String generateSignedToken(String identity, String preferredUsername, Str
}
+ public void deleteKey(final String userIdentity) {
Review Comment:
My bad, I meant to remove the below `logOut()` method and reword the function to `deleteKey` to better describe what the function does.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] thenatog commented on pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6637:
URL: https://github.com/apache/nifi/pull/6637#issuecomment-1332450878
So I've re-tested after the changes and things look good. I guess the only issue right now is that the original ticket requested that the logout feature works for Keycloak (self signed/privately signed certs) but this PR does not address that. I think we could still merge this in, but keep the ticket open, and submit a subsequent PR against the same ticket to address that part.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] thenatog commented on pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6637:
URL: https://github.com/apache/nifi/pull/6637#issuecomment-1332621055
+1 will merge
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] emiliosetiadarma commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
emiliosetiadarma commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1027559752
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -704,20 +707,131 @@ public void oidcLogout(@Context HttpServletRequest httpServletRequest, @Context
throw new IllegalStateException("OpenId Connect is not configured.");
}
- final String tokenHeader = httpServletRequest.getHeader(JwtService.AUTHORIZATION);
- jwtService.logOutUsingAuthHeader(tokenHeader);
+ // Checks if OIDC service supports logout using either by
+ // 1. revoke access token method, or
+ // 2. ID token logout method.
+ // If either of the above methods are supported,
+ // redirects request to OP to request authorization that can be exchanged for a token used for logout
Review Comment:
The access_token part is correct. For the id_token and end_session_endpoint case, the thing is we don't store the id_token anywhere, so what this method does is request an authorization code that can be eventually exchanged for an id_token. The reason I didn't use the existing NiFi token is that in `StandardOidcIdentityProvider.java`, the `convertOIDCTokenToNiFiToken` function converts the OIDC token to an internal NiFi token, which I don't think can be used for the `id_token_hint` parameter that needs to be sent
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] exceptionfactory commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
exceptionfactory commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1028539817
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -119,6 +137,7 @@ public AccessResource(
this.oidcService = oidcService;
this.kerberosSpnegoIdentityProvider = kerberosSpnegoIdentityProvider;
this.identityProvider = identityProvider;
+ this.bearerTokenResolver = new DefaultBearerTokenResolver();
Review Comment:
@thenatog Not necessarily in this case, because the `StandardBearerTokenResolver` also looks at the Cookie, which is not necessary for Registry.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] emiliosetiadarma commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
emiliosetiadarma commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1027557062
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final String callback) {
+ final String oidcRequestIdentifier = UUID.randomUUID().toString();
+ // generate a cookie to associate this login sequence
+ final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER, oidcRequestIdentifier);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
Review Comment:
The nimbusds classes were the ones used previously, just refactored some things to make stuff more reusable. Our implementation also does not have the `getState()` method used to validate OIDC state and the `isSuccess()` method to indicate a successful response. I could look into expanding the capabilities of the NiFi Registry `AuthenticationResponse`, let me know what 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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] emiliosetiadarma commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
emiliosetiadarma commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1027557958
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -687,7 +690,7 @@ public Response oidcExchange(@Context HttpServletRequest httpServletRequest, @Co
return generateOkResponse(jwt).build();
}
- @DELETE
+ @GET
Review Comment:
Based on the specs (https://openid.net/specs/openid-connect-rpinitiated-1_0.html), requests to the OIDC logout endpoint should be made with a `GET` or `POST` request, not a `DELETE` request. OIDC providers also support `GET` or `POST` request with the logout endpoint (for instance KeyCloak: https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/protocol/oidc/endpoints/LogoutEndpoint.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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] thenatog closed pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
thenatog closed pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
URL: https://github.com/apache/nifi/pull/6637
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] thenatog commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
thenatog commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1026848869
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/ApplicationResource.java:
##########
@@ -230,4 +230,10 @@ protected RevisionInfo getRevisionInfo(final LongParameter version, final Client
return revisionInfo;
}
+ private String getNiFiRegistryUri() {
+ final String nifiRegistryApiUrl = generateResourceUri();
+ final String baseUrl = StringUtils.substringBeforeLast(nifiRegistryApiUrl, "/nifi-registry-api");
+
+ return baseUrl + "/nifi-registry/";
Review Comment:
I would check if there's a way to use UriBuilder here to build this path instead of using string manipulation/substring
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java:
##########
@@ -175,6 +175,20 @@ public String generateSignedToken(String identity, String preferredUsername, Str
}
+ public void deleteKey(final String userIdentity) {
Review Comment:
Why does this method duplicate the below 'logOut()' method?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -704,20 +707,131 @@ public void oidcLogout(@Context HttpServletRequest httpServletRequest, @Context
throw new IllegalStateException("OpenId Connect is not configured.");
}
- final String tokenHeader = httpServletRequest.getHeader(JwtService.AUTHORIZATION);
- jwtService.logOutUsingAuthHeader(tokenHeader);
+ // Checks if OIDC service supports logout using either by
+ // 1. revoke access token method, or
+ // 2. ID token logout method.
+ // If either of the above methods are supported,
+ // redirects request to OP to request authorization that can be exchanged for a token used for logout
Review Comment:
Just want to confirm this - my understanding is that if we have an id_token, we can make a simple logout request to end_session_endpoint. If we only received an access_token initially, we need to HTTP request a new access_token and send this to revocation_endpoint. Is this how we are logging out/what this comment is saying?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final String callback) {
+ final String oidcRequestIdentifier = UUID.randomUUID().toString();
+ // generate a cookie to associate this login sequence
+ final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER, oidcRequestIdentifier);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
Review Comment:
Is there a reason these nimbusds classes are being used vs our implementation?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final String callback) {
+ final String oidcRequestIdentifier = UUID.randomUUID().toString();
+ // generate a cookie to associate this login sequence
+ final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER, oidcRequestIdentifier);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse;
+ try {
+ oidcResponse = AuthenticationResponseParser.parse(requestUri);
+ } catch (final ParseException e) {
+ final String loginOrLogoutString = isLogin ? "login" : "logout";
+ logger.error(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+ }
+ return oidcResponse;
+ }
+
+ private void validateOIDCState(final String oidcRequestIdentifier,
+ final AuthenticationSuccessResponse successfulOidcResponse,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ // confirm state
+ final State state = successfulOidcResponse.getState();
+ if (state == null || !oidcService.isStateValid(oidcRequestIdentifier, state)) {
+ final String loginOrLogoutMessage = isLogin ? "login" : "logout";
+ logger.error(String.format("The state value returned by the OpenId Connect Provider does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Purposed state does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+ }
+ }
+
+ /**
+ * Sends a POST request to the revoke endpoint to log out of the ID Provider.
+ *
+ * @param httpServletResponse the servlet response
+ * @param accessToken the OpenID Connect Provider access token
+ * @param revokeEndpoint the name of the cookie
+ * @throws IOException exceptional case for communication error with the OpenId Connect Provider
+ */
+ private void revokeEndpointRequest(@Context HttpServletResponse httpServletResponse, String accessToken, URI revokeEndpoint) throws IOException, NoSuchAlgorithmException {
+ final CloseableHttpClient httpClient = getHttpClient();
+ HttpPost httpPost = new HttpPost(revokeEndpoint);
+
+ List<NameValuePair> params = new ArrayList<>();
+ // Append a query param with the access token
+ params.add(new BasicNameValuePair("token", accessToken));
+ httpPost.setEntity(new UrlEncodedFormEntity(params));
+
+ try (CloseableHttpResponse response = httpClient.execute(httpPost)) {
+ if (response.getStatusLine().getStatusCode() == HTTPResponse.SC_OK) {
+ // redirect to NiFi Registry page after logout completes
+ logger.debug("You are logged out of the OpenId Connect Provider.");
+ final String postLogoutRedirectUri = getNiFiRegistryUri();
+ httpServletResponse.sendRedirect(postLogoutRedirectUri);
+ } else {
+ logger.error("There was an error logging out of the OpenId Connect Provider. " +
+ "Response status: " + response.getStatusLine().getStatusCode());
+ }
+ } finally {
+ httpClient.close();
+ }
+ }
+
+ private CloseableHttpClient getHttpClient() throws NoSuchAlgorithmException {
+ final int msTimeout = 30_000;
Review Comment:
This timeout value could be set as a final member variable and it might be better with a shorter value. In fact it looks like there's a nifi.registry.security.user.oidc.read.timeout and nifi.registry.security.user.oidc.connect.timeout properties that potentially could be used.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final String callback) {
+ final String oidcRequestIdentifier = UUID.randomUUID().toString();
+ // generate a cookie to associate this login sequence
+ final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER, oidcRequestIdentifier);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse;
+ try {
+ oidcResponse = AuthenticationResponseParser.parse(requestUri);
+ } catch (final ParseException e) {
+ final String loginOrLogoutString = isLogin ? "login" : "logout";
+ logger.error(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+ }
+ return oidcResponse;
+ }
+
+ private void validateOIDCState(final String oidcRequestIdentifier,
+ final AuthenticationSuccessResponse successfulOidcResponse,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ // confirm state
+ final State state = successfulOidcResponse.getState();
+ if (state == null || !oidcService.isStateValid(oidcRequestIdentifier, state)) {
+ final String loginOrLogoutMessage = isLogin ? "login" : "logout";
+ logger.error(String.format("The state value returned by the OpenId Connect Provider does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Purposed state does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
Review Comment:
Should this say 'Proposed state' ?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -119,6 +137,7 @@ public AccessResource(
this.oidcService = oidcService;
this.kerberosSpnegoIdentityProvider = kerberosSpnegoIdentityProvider;
this.identityProvider = identityProvider;
+ this.bearerTokenResolver = new DefaultBearerTokenResolver();
Review Comment:
This token resolver is using Spring directly whereas NiFi code also contains a StandardBearerTokenResolver implementation. We might want to confirm if we should use that instead.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -589,44 +611,25 @@ public void oidcCallback(@Context HttpServletRequest httpServletRequest, @Contex
throw new IllegalStateException("OpenId Connect is not configured.");
}
- final String oidcRequestIdentifier = getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ final String oidcRequestIdentifier = getOidcRequestIdentifier(httpServletRequest);
if (oidcRequestIdentifier == null) {
throw new IllegalStateException("The login request identifier was not found in the request. Unable to continue.");
}
- final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse;
- try {
- oidcResponse = AuthenticationResponseParser.parse(getRequestUri());
- } catch (final ParseException e) {
- logger.error("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue login process.");
-
- // remove the oidc request cookie
- removeOidcRequestCookie(httpServletResponse);
- // forward to the error page
- throw new IllegalStateException("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue login process.");
- }
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse =
Review Comment:
Again here, any benefit to using these nimbusds implementations of AuthenticationResponse?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -687,7 +690,7 @@ public Response oidcExchange(@Context HttpServletRequest httpServletRequest, @Co
return generateOkResponse(jwt).build();
}
- @DELETE
+ @GET
Review Comment:
Is there a reason to change this to a GET request? Historically I assume this has been a DELETE request for some time now, therefore this would potentially be changing the API contract.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] emiliosetiadarma commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
emiliosetiadarma commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1027563567
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final String callback) {
+ final String oidcRequestIdentifier = UUID.randomUUID().toString();
+ // generate a cookie to associate this login sequence
+ final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER, oidcRequestIdentifier);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse;
+ try {
+ oidcResponse = AuthenticationResponseParser.parse(requestUri);
+ } catch (final ParseException e) {
+ final String loginOrLogoutString = isLogin ? "login" : "logout";
+ logger.error(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+ }
+ return oidcResponse;
+ }
+
+ private void validateOIDCState(final String oidcRequestIdentifier,
+ final AuthenticationSuccessResponse successfulOidcResponse,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ // confirm state
+ final State state = successfulOidcResponse.getState();
+ if (state == null || !oidcService.isStateValid(oidcRequestIdentifier, state)) {
+ final String loginOrLogoutMessage = isLogin ? "login" : "logout";
+ logger.error(String.format("The state value returned by the OpenId Connect Provider does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Purposed state does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+ }
+ }
+
+ /**
+ * Sends a POST request to the revoke endpoint to log out of the ID Provider.
+ *
+ * @param httpServletResponse the servlet response
+ * @param accessToken the OpenID Connect Provider access token
+ * @param revokeEndpoint the name of the cookie
+ * @throws IOException exceptional case for communication error with the OpenId Connect Provider
+ */
+ private void revokeEndpointRequest(@Context HttpServletResponse httpServletResponse, String accessToken, URI revokeEndpoint) throws IOException, NoSuchAlgorithmException {
+ final CloseableHttpClient httpClient = getHttpClient();
+ HttpPost httpPost = new HttpPost(revokeEndpoint);
+
+ List<NameValuePair> params = new ArrayList<>();
+ // Append a query param with the access token
+ params.add(new BasicNameValuePair("token", accessToken));
+ httpPost.setEntity(new UrlEncodedFormEntity(params));
+
+ try (CloseableHttpResponse response = httpClient.execute(httpPost)) {
+ if (response.getStatusLine().getStatusCode() == HTTPResponse.SC_OK) {
+ // redirect to NiFi Registry page after logout completes
+ logger.debug("You are logged out of the OpenId Connect Provider.");
+ final String postLogoutRedirectUri = getNiFiRegistryUri();
+ httpServletResponse.sendRedirect(postLogoutRedirectUri);
+ } else {
+ logger.error("There was an error logging out of the OpenId Connect Provider. " +
+ "Response status: " + response.getStatusLine().getStatusCode());
+ }
+ } finally {
+ httpClient.close();
+ }
+ }
+
+ private CloseableHttpClient getHttpClient() throws NoSuchAlgorithmException {
+ final int msTimeout = 30_000;
Review Comment:
Thanks for letting me know, will make the changes!
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] thenatog commented on pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
thenatog commented on PR #6637:
URL: https://github.com/apache/nifi/pull/6637#issuecomment-1320218892
Reviewing
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] thenatog commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
thenatog commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1028579874
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final String callback) {
+ final String oidcRequestIdentifier = UUID.randomUUID().toString();
+ // generate a cookie to associate this login sequence
+ final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER, oidcRequestIdentifier);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
Review Comment:
I see now that AuthenticationResponse is a more general class used by all of our authn providers and is not logically equivalent to the nimbusds class, so I guess we'll leave these things as they are and plan that it will be removed when we migrate to use Spring for OIDC.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [nifi] emiliosetiadarma commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
Posted by GitBox <gi...@apache.org>.
emiliosetiadarma commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1027555251
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/ApplicationResource.java:
##########
@@ -230,4 +230,10 @@ protected RevisionInfo getRevisionInfo(final LongParameter version, final Client
return revisionInfo;
}
+ private String getNiFiRegistryUri() {
+ final String nifiRegistryApiUrl = generateResourceUri();
+ final String baseUrl = StringUtils.substringBeforeLast(nifiRegistryApiUrl, "/nifi-registry-api");
+
+ return baseUrl + "/nifi-registry/";
Review Comment:
The previous approach was to use `generateResourceUri("..", "nifi-registry")` which I could revert to. This approach makes it similar to how NiFi gets it done. Let me know how you want me to proceed
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org