Implement session logout in OAuth2Authenticator #18753

Praveen2112 · 2023-08-21T08:18:58Z

Description

When trying to logout from Trino WebUI, allows the user to be redirected to a specific end_session_endpoint thereby allowing user to logout from the IdP/OP.

Specification - https://openid.net/specs/openid-connect-rpinitiated-1_0.html

Will be sharing a video link on how it worked before and after this change.

Fixes #13060

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Implement session logout when logging out from Trino WebUI . ({issue}`13060`)

lukasz-walkiewicz · 2023-08-22T09:27:20Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Client.java

@@ -34,6 +34,8 @@ Response getOAuth2Response(String code, URI callbackUri, Optional<String> nonce)
    Response refreshTokens(String refreshToken)
            throws ChallengeFailedException;

+    Optional<URI> getLogoutEndPoint(Optional<String> idToken, URI callbackUrl);


nit: endpoint is one word so casing seems off

lukasz-walkiewicz · 2023-08-22T09:33:49Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OidcDiscovery.java

@@ -114,6 +115,7 @@ private OAuth2ServerConfig readConfiguration(String body)
            else {
                userinfoEndpoint = Optional.empty();
            }
+            Optional<String> endSessionEndpoint = getOptionalField("end_session_endpoint", Optional.ofNullable(metadata.getEndSessionEndpointURI()).map(URI::toString), END_SESSION_URL, Optional.empty());


It's required according to the spec:

end_session_endpoint REQUIRED. URL at the OP to which an RP can perform a redirect to request that the End-User be logged out at the OP.

lukasz-walkiewicz · 2023-08-22T09:37:50Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Service.java

-                        .build();
+                                NonceCookie.delete());
+                if (oauth2Response.getIdToken().isPresent()) {
+                    builder.cookie(OAuthIdTokenCookie.create(oauth2Response.getIdToken().get(), oauth2Response.getExpiration()));


Shouldn't expiration be calculated as for OAuthWebUiCookie above?

But in case of OAuthWebUiCookie we do wrap the token as JWT and pass the expiration time for that, which is independent of the ID token. So shouldn't we use them ?

I see. How this should work for refresh tokens in that case? I think we will get expiration for the access token, let say, 1h but since we will have refresh token we will be able to prolong the session for much longer than that. Using the same expiration for ID token cookie lifetime will mean that the this cookie will expire after 1h and we won't be able to perform logout correctly after that.

core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java

lukasz-walkiewicz · 2023-08-22T11:20:07Z

core/trino-main/src/test/java/io/trino/server/security/TestResourceSecurity.java

@@ -1367,6 +1375,29 @@ private static Principal authenticate2(String user, String password)
        throw new AccessDeniedException("Invalid credentials2");
    }

+    private static HttpCookie getWebUiOAuthCookie(CookieManager cookieManager)
+    {
+        return cookieManager.getCookieStore().getCookies().stream()


nit: duplicated fragment, you could extract a method to get the cookie

lukasz-walkiewicz · 2023-08-22T11:24:37Z

core/trino-main/src/test/java/io/trino/server/ui/TestWebUi.java

@@ -836,7 +842,31 @@ public void testCustomPrincipalField()
        }
    }

-    private void assertAuth2Authentication(TestingTrinoServer server, String accessToken, boolean refreshTokensEnabled)
+    @Test
+    public void testOAuth2AuthenticatorWithoutEndsessionEndpoint()


What if http-server.authentication.oauth2.scopes does not contain openid scope but IdP returned end_session_endpoint and the other way around?

IIUC, in case of openid scope then the end_session_endopoint is mandatory right ? But if the scope if non opeid then we could optionally provide the end-point which if configured might be redirected. Am I missing something here ?

Praveen2112 · 2023-08-22T12:10:36Z

@lukasz-walkiewicz Thanks for the review. Have addressed them

atanasenko · 2023-09-18T13:17:16Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiLogoutResource.java

+                .path("logout.html")
+                .build();
+
+        return Response.seeOther(auth2Client.getLogoutEndpoint(idToken, callBackUri).orElse(callBackUri))


Is it possible to call sso logout ourselves behind the scenes instead of depending on user browser's doing that?
Otherwise we could end up in situation where the user doesn't follow that redirect for any reason and the value is still valid.

The spec here mentions that

An RP requests that the OP log out the End-User by redirecting the End-User's User Agent to the OP's Logout Endpoint. This URL is normally obtained via the end_session_endpoint element of the OP's Discovery response or may be learned via other mechanisms.

So I guess we should redirect on the User browser instead of handling it. Additionally each IdP can have its own way to redirecting the user right ?

lukasz-walkiewicz

Overall, looks good but I think we lack a bit of coverage, especially wrt support for refresh tokens.

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

lukasz-walkiewicz · 2023-10-02T09:35:24Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Service.java

-                        .build();
+                                NonceCookie.delete());
+                if (oauth2Response.getIdToken().isPresent()) {
+                    builder.cookie(OAuthIdTokenCookie.create(oauth2Response.getIdToken().get(), oauth2Response.getExpiration()));


I see. How this should work for refresh tokens in that case? I think we will get expiration for the access token, let say, 1h but since we will have refresh token we will be able to prolong the session for much longer than that. Using the same expiration for ID token cookie lifetime will mean that the this cookie will expire after 1h and we won't be able to perform logout correctly after that.

...rc/test/java/io/trino/server/security/oauth2/TestOAuth2WebUiAuthenticationFilterWithJwt.java

Praveen2112 · 2023-10-06T17:01:57Z

@lukasz-walkiewicz Thanks a lot for the review. AC

thorbjornsen · 2023-10-09T19:35:11Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OidcDiscovery.java

@@ -114,6 +115,7 @@ private OAuth2ServerConfig readConfiguration(String body)
            else {
                userinfoEndpoint = Optional.empty();
            }
+            Optional<URI> endSessionEndpoint = Optional.of(getRequiredField("end_session_endpoint", metadata.getEndSessionEndpointURI(), END_SESSION_URL, Optional.empty()));


Should provide an override value for END_SESSION_URL which comes from the OidcDiscoveryConfig. Similar to the other URL values. This allows non-standard endpoint values.

That's a good point but OidcDiscoveryConfig actually allows providing urls explicitly (effectively overriding those obtained from metadata discovery) for backward compatibility, allowing for non-standard endpoint values wasn't the objective. We intend to drop these properties #18101
That said, is it something you think we should support? Is this something you use these properties for?

This END_SESSION_URL, Optional.empty()) becomes this END_SESSION_URL, logoutUrl), where logoutUrl is set in the constructor

logoutUrl = requireNonNull(oidcConfig.getEndSessionUrl(), "logoutUrl is null");

I had to make these changes for a customer, who is using Ping. The "standard" endpoint found with end_session_endpoint is not being found because the well-known endpoint is returning ping_end_session_endpoint. So the customer had to be able to specify the logout URL explicitly.

But if we disable the OIDC discovery then we could inject them via StaticConfigurationProvider - we could provide custom end-points right ? cc : @lukasz-walkiewicz Are we removing that endpoint also ?

No, StaticConfigurationProvider is here to stay. I think we ultimately would like to remove this overriding some day and users should provide configuration explicitly if the IdP does not conform OIDC spec.

@thorbjornsen In this case we could still use StaticOAuth2ServerConfiguration but we need to specify the configuration explicitly.

lukasz-walkiewicz

LGTM % minor changes to the docs

lukasz-walkiewicz · 2023-10-09T21:37:50Z

core/trino-main/src/test/java/io/trino/server/security/oauth2/TestingHydraIdentityProvider.java

@@ -175,7 +177,8 @@ public void createClient(
                        "--response-types", "token,code,id_token",
                        "--scope", "openid,offline",
                        "--token-endpoint-auth-method", tokenEndpointAuthMethod.getValue(),
-                        "--callbacks", callbackUrl)
+                        "--callbacks", callbackUrl,
+                        "--post-logout-callbacks", logoutCallbackUrl)


Probably we need to update docs to add information about registering post-logout url

lukasz-walkiewicz · 2023-10-09T21:46:19Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OidcDiscovery.java

@@ -114,6 +115,7 @@ private OAuth2ServerConfig readConfiguration(String body)
            else {
                userinfoEndpoint = Optional.empty();
            }
+            Optional<URI> endSessionEndpoint = Optional.of(getRequiredField("end_session_endpoint", metadata.getEndSessionEndpointURI(), END_SESSION_URL, Optional.empty()));


That's a good point but OidcDiscoveryConfig actually allows providing urls explicitly (effectively overriding those obtained from metadata discovery) for backward compatibility, allowing for non-standard endpoint values wasn't the objective. We intend to drop these properties #18101
That said, is it something you think we should support? Is this something you use these properties for?

Praveen2112 · 2023-10-10T06:21:32Z

@mosabua Can you please check the docs.

Praveen2112 · 2023-10-10T06:21:53Z

@lukasz-walkiewicz Updated the docs

kokosing · 2023-10-10T09:41:59Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2ServerConfigProvider.java

@@ -29,14 +29,16 @@ class OAuth2ServerConfig
        private final URI tokenUrl;


this could be record. Maybe follow up?

I added it as a prep commit

Extract cookieExpiration time computation

If the NodeInternalServer is not set, we tend to pick a random interface from getNetworkInterfaces

- Extracted cookie attribute assertion - Rename assertTrinoCookie to assertTrinoOAuth2Cookie - Extract parseJwsClaims from TestOAuth2WebUiAuthenticationFilterWithJwt to BaseOAuth2WebUiAuthenticationFilterTest

docs/src/main/sphinx/security/oauth2.md

Praveen2112 · 2023-10-10T16:12:50Z

@mosabua AC

When trying to logout from Trino WebUI, allows the user to be redirected to a specific end_session_endpoint thereby allowing user to logout from the IdP/OP. Specification - https://openid.net/specs/openid-connect-rpinitiated-1_0.html

Praveen2112 · 2023-10-10T21:43:13Z

@mosabua AC

mosabua

Docs look good now.

cla-bot bot added the cla-signed label Aug 21, 2023

Praveen2112 requested a review from lukasz-walkiewicz August 21, 2023 08:19

Praveen2112 force-pushed the praveen/rp_initiated_logout branch from db55ab9 to 591f8f8 Compare August 22, 2023 06:01

lukasz-walkiewicz reviewed Aug 22, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/rp_initiated_logout branch from 591f8f8 to bb3cc23 Compare August 22, 2023 12:10

Praveen2112 force-pushed the praveen/rp_initiated_logout branch 3 times, most recently from a60c7f4 to 53d064a Compare September 14, 2023 10:08

Praveen2112 marked this pull request as ready for review September 14, 2023 10:35

atanasenko reviewed Sep 18, 2023

View reviewed changes

lukasz-walkiewicz reviewed Oct 2, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/rp_initiated_logout branch 6 times, most recently from 7a647ed to 9fcc9d0 Compare October 6, 2023 17:01

Praveen2112 force-pushed the praveen/rp_initiated_logout branch from 9fcc9d0 to 0ac0b57 Compare October 9, 2023 07:04

Praveen2112 requested a review from lukasz-walkiewicz October 9, 2023 11:08

thorbjornsen reviewed Oct 9, 2023

View reviewed changes

lukasz-walkiewicz approved these changes Oct 9, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/rp_initiated_logout branch from 0ac0b57 to a97d69f Compare October 10, 2023 06:20

Praveen2112 requested a review from dain October 10, 2023 06:24

github-actions bot added the docs label Oct 10, 2023

kokosing approved these changes Oct 10, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/rp_initiated_logout branch from a97d69f to 8309405 Compare October 10, 2023 12:24

Praveen2112 added 4 commits October 10, 2023 18:41

Cleanup OAuth2Service

340b8c8

Extract cookieExpiration time computation

Convert OAuth2ServerConfig to record

5eccba2

Explicitly set login-server internal address to localhost

c25d71d

If the NodeInternalServer is not set, we tend to pick a random interface from getNetworkInterfaces

Minor cleanup in BaseOAuth2WebUiAuthenticationFilterTest

180b3ab

- Extracted cookie attribute assertion - Rename assertTrinoCookie to assertTrinoOAuth2Cookie - Extract parseJwsClaims from TestOAuth2WebUiAuthenticationFilterWithJwt to BaseOAuth2WebUiAuthenticationFilterTest

Praveen2112 force-pushed the praveen/rp_initiated_logout branch from 8309405 to 370f00b Compare October 10, 2023 13:12

kokosing approved these changes Oct 10, 2023

View reviewed changes

mosabua requested changes Oct 10, 2023

View reviewed changes

docs/src/main/sphinx/security/oauth2.md Outdated Show resolved Hide resolved

docs/src/main/sphinx/security/oauth2.md Outdated Show resolved Hide resolved

docs/src/main/sphinx/security/oauth2.md Outdated Show resolved Hide resolved

Praveen2112 force-pushed the praveen/rp_initiated_logout branch 2 times, most recently from 4df11c9 to b904eb0 Compare October 10, 2023 16:09

Praveen2112 requested a review from mosabua October 10, 2023 16:12

Implement session logout in OAuth2Authenticator

e066e29

When trying to logout from Trino WebUI, allows the user to be redirected to a specific end_session_endpoint thereby allowing user to logout from the IdP/OP. Specification - https://openid.net/specs/openid-connect-rpinitiated-1_0.html

Praveen2112 force-pushed the praveen/rp_initiated_logout branch from b904eb0 to e066e29 Compare October 10, 2023 21:42

mosabua approved these changes Oct 11, 2023

View reviewed changes

Praveen2112 merged commit 6adfde2 into trinodb:master Oct 11, 2023
69 checks passed

github-actions bot added this to the 429 milestone Oct 11, 2023

colebow mentioned this pull request Oct 11, 2023

Add Trino 429 release notes #19357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement session logout in OAuth2Authenticator #18753

Implement session logout in OAuth2Authenticator #18753

Praveen2112 commented Aug 21, 2023 •

edited

Loading

lukasz-walkiewicz Aug 22, 2023

lukasz-walkiewicz Aug 22, 2023

lukasz-walkiewicz Aug 22, 2023

Praveen2112 Aug 22, 2023

lukasz-walkiewicz Oct 2, 2023

lukasz-walkiewicz Aug 22, 2023

lukasz-walkiewicz Aug 22, 2023

Praveen2112 Aug 22, 2023

Praveen2112 commented Aug 22, 2023

atanasenko Sep 18, 2023

Praveen2112 Sep 18, 2023

lukasz-walkiewicz left a comment

lukasz-walkiewicz Oct 2, 2023

Praveen2112 commented Oct 6, 2023

thorbjornsen Oct 9, 2023

lukasz-walkiewicz Oct 9, 2023

thorbjornsen Oct 9, 2023

Praveen2112 Oct 10, 2023

lukasz-walkiewicz Oct 10, 2023

Praveen2112 Oct 10, 2023

lukasz-walkiewicz left a comment

lukasz-walkiewicz Oct 9, 2023

lukasz-walkiewicz Oct 9, 2023

Praveen2112 commented Oct 10, 2023

Praveen2112 commented Oct 10, 2023

kokosing Oct 10, 2023

Praveen2112 Oct 10, 2023

Praveen2112 commented Oct 10, 2023

Praveen2112 commented Oct 10, 2023

mosabua left a comment

		@@ -29,14 +29,16 @@ class OAuth2ServerConfig
		private final URI tokenUrl;

Implement session logout in OAuth2Authenticator #18753

Implement session logout in OAuth2Authenticator #18753

Conversation

Praveen2112 commented Aug 21, 2023 • edited Loading

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-walkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Oct 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-walkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Oct 10, 2023

Praveen2112 commented Oct 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Oct 10, 2023

Praveen2112 commented Oct 10, 2023

mosabua left a comment

Choose a reason for hiding this comment

Praveen2112 commented Aug 21, 2023 •

edited

Loading