Fix HTTP_Status on OAuth2 refresh token redirect #15336

huberty89 · 2022-12-07T17:47:25Z

Description

When refresh token is retrieved for UI, currently we were sending HTTP Status 303, assuming that all the request will just repeat the call on the Location header. When this works for GET/PUT verbs, it does not for non-idempotent ones like POST, as every JS HTTP client should do a GET on LOCATION after 303 on POST. Due to that I change it to 307, that should force every client to repeat the exactly the same request, no matter the verb.

This PR replaces #15184

Release notes

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

# Security
* Fix HTTP redirect after OAuth2 token refresh. ({issue}`15336`)

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

lukasz-walkiewicz · 2022-12-08T11:02:49Z

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

@@ -772,6 +811,24 @@ private void assertAuth2Authentication(HttpServerInfo httpServerInfo, String acc
        assertResponseCode(client, getLocation(baseUri, "/ui/unknown"), SC_NOT_FOUND);
        assertResponseCode(client, getLocation(baseUri, "/ui/api/unknown"), SC_NOT_FOUND);

+        if (refreshTokensEnabled) {
+            // wait till auth token expires
+            Thread.sleep(Duration.between(ZonedDateTime.now(), authTokenExpiration).toMillis());


Could you issue an already expired token? To get rid of this the sleep.

lukasz-walkiewicz · 2022-12-08T11:11:49Z

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

        String state = newJwtBuilder()
                .signWith(hmacShaKeyFor(Hashing.sha256().hashString(STATE_KEY, UTF_8).asBytes()))
                .setAudience("trino_oauth_ui")
-                .setExpiration(Date.from(ZonedDateTime.now().plusMinutes(10).toInstant()))
+                .setExpiration(Date.from(authTokenExpiration.toInstant()))


This change is probably not needed, state is only a part of the authorization challenge https://www.rfc-editor.org/rfc/rfc6749#section-4.1.1 so its expiration shouldn't matter.

Right, I mixed them up, reverted

lukasz-walkiewicz · 2022-12-08T11:13:07Z

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

@@ -772,6 +811,24 @@ private void assertAuth2Authentication(HttpServerInfo httpServerInfo, String acc
        assertResponseCode(client, getLocation(baseUri, "/ui/unknown"), SC_NOT_FOUND);
        assertResponseCode(client, getLocation(baseUri, "/ui/api/unknown"), SC_NOT_FOUND);

+        if (refreshTokensEnabled) {


nit: extract to a method?

Extracted because now I use this part in two places

Praveen2112 · 2022-12-08T12:29:59Z

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

@@ -148,6 +152,8 @@
    private static final String TEST_PASSWORD2 = "test-password2";
    private static final String HMAC_KEY = Resources.getResource("hmac_key.txt").getPath();
    private static final PrivateKey JWK_PRIVATE_KEY;
+    private static final String REFRESH_TOKEN = "REFRESH_TOKEN";
+    private static final int REFRESH_TOKEN_TIMEOUT_MINUTES = 5;


nit: How about using a Duration

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

Praveen2112 · 2022-12-08T12:34:54Z

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

+                        .putAll(OAUTH2_PROPERTIES)
+                        .put("http-server.authentication.oauth2.jwks-url", jwkServer.getBaseUrl().toString())
+                        .put("http-server.authentication.oauth2.refresh-tokens", "true")
+                        .put("http-server.authentication.oauth2.refresh-tokens.issued-token.timeout", REFRESH_TOKEN_TIMEOUT_MINUTES + "m")


This is a timeout for the token minted by us (i.e with access token and refresh token) - can we have a test where the minted refresh token is expired ?

I added additional test to test if it really expires: testOAuth2AuthenticatorRefreshTokenExpiration

huberty89 · 2022-12-08T21:53:18Z

I pushed updated version.
@Praveen2112 @lukasz-walkiewicz @skrzypo987 please take a look again

lukasz-walkiewicz

LGMT 👍

lukasz-walkiewicz · 2022-12-09T10:49:03Z

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

        {
            claims = new DefaultClaims(createClaims());
            claims.putAll(additionalClaims);
+            this.accessTokenValidity = accessTokenValidity;


huberty89 · 2022-12-09T11:42:24Z

I hit flaky test #15293

Praveen2112 · 2022-12-12T04:50:56Z

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

+    private static void assertCookieWithRefreshToken(TestingTrinoServer server, HttpCookie authCookie, String accessToken)
+    {
+        TokenPairSerializer tokenPairSerializer = server.getInstance(Key.get(TokenPairSerializer.class));
+        TokenPairSerializer.TokenPair deserialize = tokenPairSerializer.deserialize(authCookie.getValue());


nit: Can we import TokenPair directly ?

Praveen2112 · 2022-12-12T04:59:40Z

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

+            HttpCookie cookie = getOnlyElement(cookieManager.getCookieStore().getCookies());
+            assertCookieWithRefreshToken(server, cookie, oauthClient.getAccessToken());
+
+            String location = getLocation(baseUri, "/ui/api/query/20221206_145344_00001_qe6gw/killed");


Is it possible for us to check with an existing API. The QueryId doesn't exists in the testing server right ?

Can we use validApiLocation here ?

I could replace all of this with:

assertResponseCode(client, getValidApiLocation(baseUri), SC_TEMPORARY_REDIRECT); assertOk(client, getValidApiLocation(baseUri));

but getValidApiLocation returns an URL to GET endpoint and here I used PUT method.
Ok, I will replace that. It's much more readable.

When refresh token is retrieved for UI, currently we were sending HTTP Status 303, assuming that all the request will just repeat the call on the Location header. When this works for GET/PUT verbs, it does not for non-idempotent ones like POST, as every js http client should do a GET on LOCATION after 303 on POST. Due to that I change it to 307, that should force every client to repeat exactly the same request, no matter the verb. Co-authored-by: s2lomon <[email protected]>

kokosing · 2022-12-12T12:45:53Z

Merged, thanks!

cla-bot bot added the cla-signed label Dec 7, 2022

huberty89 requested review from lukasz-walkiewicz, Praveen2112, skrzypo987 and kokosing and removed request for lukasz-walkiewicz December 7, 2022 17:47

skrzypo987 reviewed Dec 8, 2022

View reviewed changes

core/trino-main/src/test/java/io/trino/server/ui/TestWebUi.java Outdated Show resolved Hide resolved

huberty89 force-pushed the huberty89/fix-oauth-refresh-on-post-method branch 2 times, most recently from 389c3c1 to bb62f43 Compare December 8, 2022 09:22

lukasz-walkiewicz reviewed Dec 8, 2022

View reviewed changes

Praveen2112 reviewed Dec 8, 2022

View reviewed changes

huberty89 force-pushed the huberty89/fix-oauth-refresh-on-post-method branch 2 times, most recently from e6fd9b0 to 523e1f3 Compare December 8, 2022 21:30

huberty89 force-pushed the huberty89/fix-oauth-refresh-on-post-method branch from 523e1f3 to 9a7593c Compare December 9, 2022 07:10

lukasz-walkiewicz approved these changes Dec 9, 2022

View reviewed changes

huberty89 force-pushed the huberty89/fix-oauth-refresh-on-post-method branch from 9a7593c to a0241a8 Compare December 9, 2022 10:52

Praveen2112 approved these changes Dec 12, 2022

View reviewed changes

huberty89 force-pushed the huberty89/fix-oauth-refresh-on-post-method branch from a0241a8 to fe1e1c8 Compare December 12, 2022 10:53

kokosing approved these changes Dec 12, 2022

View reviewed changes

kokosing merged commit dfe33c7 into trinodb:master Dec 12, 2022

huberty89 mentioned this pull request Dec 12, 2022

Fix HTTP_Status on OAuth2 refresh token redirect #15184

Closed

github-actions bot added this to the 404 milestone Dec 12, 2022

colebow mentioned this pull request Dec 14, 2022

Add Trino 405 release notes #15139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HTTP_Status on OAuth2 refresh token redirect #15336

Fix HTTP_Status on OAuth2 refresh token redirect #15336

huberty89 commented Dec 7, 2022 •

edited

Loading

lukasz-walkiewicz Dec 8, 2022

huberty89 Dec 8, 2022

lukasz-walkiewicz Dec 8, 2022

huberty89 Dec 8, 2022

lukasz-walkiewicz Dec 8, 2022

huberty89 Dec 8, 2022

Praveen2112 Dec 8, 2022

huberty89 Dec 8, 2022

Praveen2112 Dec 8, 2022

huberty89 Dec 8, 2022 •

edited

Loading

huberty89 commented Dec 8, 2022

lukasz-walkiewicz left a comment

lukasz-walkiewicz Dec 9, 2022

huberty89 Dec 9, 2022

huberty89 commented Dec 9, 2022

Praveen2112 Dec 12, 2022

huberty89 Dec 12, 2022

Praveen2112 Dec 12, 2022

Praveen2112 Dec 12, 2022

huberty89 Dec 12, 2022

kokosing commented Dec 12, 2022

Fix HTTP_Status on OAuth2 refresh token redirect #15336

Fix HTTP_Status on OAuth2 refresh token redirect #15336

Conversation

huberty89 commented Dec 7, 2022 • edited Loading

Description

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

Choose a reason for hiding this comment

huberty89 Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

huberty89 commented Dec 8, 2022

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

huberty89 commented Dec 9, 2022

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

kokosing commented Dec 12, 2022

huberty89 commented Dec 7, 2022 •

edited

Loading

huberty89 Dec 8, 2022 •

edited

Loading