Rewrite OIDC implementation #8641

11xor6 · 2021-07-22T22:40:24Z

No description provided.

dain · 2021-07-23T19:47:40Z

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

    {
-        return service.parseClaimsJws(jws);
+        Map<String, Object> claims = service.convertTokenToClaims(token);
+        if (claims.containsKey(principalField)) {


I think you can drop the wrapping if statement here. The Optional.ofNullable(claims.get(principalField)) will be an empty optional if the field is missing.

Also the String.class::cast can fail if you have the wrong type... you'll get a class cast exception. If that is not what you want, consider filter, using map(Object::toString) or check the type and throw a more specific exception. My guess the cast is fine.

I think the runtime exception is fine, the only time that will happen is if they misconfigure the principal field to point at an array or object which are not common.

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

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

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

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

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

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

kokosing · 2021-07-28T10:11:18Z

...no-main/src/main/java/io/trino/server/security/oauth2/OAuth2AuthenticationSupportModule.java

@@ -26,6 +26,7 @@
    protected void setup(Binder binder)


Fix OIDC implementation to resolve correctness issues

Can you please explain what was the issue about?

Changed the commit message to be clearer. Specifically the commit fixes several structural issues where there were inappropriate dependencies and duplicate/confusing code. Additionally there are several security fixes including correctly validating ID and access tokens.

I don't see a new commit message yet.

Specifically the commit fixes several structural issues where there were inappropriate dependencies and duplicate/confusing code.

Let's have each of the issue handled in separate commit, currently it is hard to follow what is fixed where.

I worked with Nik on this PR and agreed to the current commit structure.

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

kokosing · 2021-07-28T10:13:41Z

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

    }

    @Override
    protected AuthenticationException needAuthentication(ContainerRequestContext request, String message)
    {
        UUID authId = UUID.randomUUID();
-        URI redirectUri = service.startRestChallenge(request.getUriInfo().getBaseUri().resolve(CALLBACK_ENDPOINT), authId);


Is changing the protocol needed to solve the "correctness issues"?

This does not change the protocol. It simply changes the x_redirect_server URL returned in the authenticate header, but the protocol is completely unchanged.

Protocol is not changed as there is no needed change in the client.

However there is a new round trip. And there is new endpoint method created. Commit message does not explain the motivation behind it. Previously we were starting the challenge at at the first call, now it is postponed and it is started once the new endpoint method is called.

Correct, and we should have always done that. It lets us use a nonce to protect the authentication code. Also in the future, we can use this to timeout clients faster... basically if a client doesn't hit the initiate in a few seconds, we kill the auth because likely the browser did not launch.

@11xor6 in the commit message mention the new use of nonce cookies for query clients.

kokosing · 2021-07-28T10:14:34Z

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

@@ -43,6 +43,7 @@
    private String authUrl;
    private String tokenUrl;
    private String jwksUrl;
+    private String userinfoUrl;


Should we log a warning when this field is not set?

This field is not required, if it's specified we use it to validate access tokens, otherwise we treat access tokens as JWTs.

kokosing

I would like @lukasz-walkiewicz to do a review of this before merging.

kokosing · 2021-08-03T05:40:48Z

Please notice #8756, oauth2 product tests started to fail deterministically. We won't be able to merge this pull request without fixing product tests first. @lukasz-walkiewicz Did you have a chance to understand why it started to fail?

11xor6 · 2021-08-05T05:53:31Z

#8756 has now been resolved via #8779; the OAuth2 product tests are now fully functional again.

cc: @kokosing

lukasz-walkiewicz

Looks good although I'm not sure I would agree with:

I'll add a config item for the access token issuer. However that said Authentication by definition needs to be as strict as possible.

I would say that it should be as strict as possible by default but in this case there should be a way to relax this check in order to make some unusual providers work. AD FS is just an example of IdP which would not work with the issuer check you implemented. I wouldn't be surprised if there are cases like this. Especially since access token format is not really defined in OAuth 2.0 spec and JWT spec even states that iss is optional.
That said, I'm fine with the current solution. Thanks for bringing support for opaque tokens to Trino 👍

lukasz-walkiewicz · 2021-08-05T17:15:10Z

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

        this.scopes = oauth2Config.getScopes();
        this.challengeTimeout = Duration.ofMillis(oauth2Config.getChallengeTimeout().toMillis());
        this.stateHmac = oauth2Config.getStateKey()
                .map(key -> sha256().hashString(key, UTF_8).asBytes())
                .orElseGet(() -> secureRandomBytes(32));
+
+        this.issuer = oauth2Config.getIssuer();
+        this.accessTokenIssuer = oauth2Config.getAccessTokenIssuer().orElse(issuer);


nit: I think it would be better to move this fallback to the config class.

I originally had it written that way, however I was asked to write it this way instead.

lukasz-walkiewicz · 2021-08-05T17:52:08Z

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

+
+public interface OAuth2TokenHandler
+{
+    void setAccessToken(String hashedState, String accessToken);


So I would say it's a hashed authId.

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

This includes: * Properly validating ID and access tokens * A single point of entry for initiating OAuth2 flows * Add an OAuth2 initiation endpoint for query clients which allows use of a nonce cookie in token exchange. * Breaking a hard dependency between OAuth2TokenExchange and OAuth2Service * Use a hashed UUID/UUID pair in token exchange to prevent a comprimised browser from obtaining the access token * Support trusting additional audiences in access tokens

hashhar · 2021-08-25T06:17:24Z

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

-    @Config("http-server.authentication.oauth2.audience")
-    @ConfigDescription("The required audience of a token")
-    public OAuth2Config setAudience(String audience)
+    @LegacyConfig("http-server.authentication.oauth2.audience")


I missed this during review. this should be something like below (if the configs are indeed equivalent).

@LegacyConfig(value = "http-server.authentication.oauth2.audience", replacedBy = "http-server.authentication.oauth2.additional-audiences")

Otherwise old config files will cause Trino startup to fail with an error like:

io.airlift.bootstrap.ApplicationConfigurationException: Configuration errors: 1) Error: Invalid configuration property http-server.authentication.oauth2.issuer: may not be null (for class OAuth2Config.issuer) 2) Error: Configuration property 'http-server.authentication.oauth2.audience' has been replaced. Use 'http-server.authentication.oauth2.additional-audiences' instead.

This should also be mentioned in the release notes and documentation. cc: @11xor6 @kokosing

If the configs are not interchangeable then the existing code looks good to me.

Opaque tokens are supported since 361: trinodb#8641

Opaque tokens are supported since 361: #8641

Opaque tokens are supported since 361: trinodb#8641

cla-bot bot added the cla-signed label Jul 22, 2021

11xor6 requested review from electrum and dain July 22, 2021 22:40

11xor6 force-pushed the rewrite-oidc-implementation branch 7 times, most recently from 7432889 to 624fc4d Compare July 23, 2021 02:50

hashhar requested a review from lukasz-walkiewicz July 23, 2021 05:43

11xor6 force-pushed the rewrite-oidc-implementation branch 13 times, most recently from 251b945 to f48bd09 Compare July 28, 2021 00:16

dain reviewed Jul 28, 2021

View reviewed changes

11xor6 force-pushed the rewrite-oidc-implementation branch from 77e4a39 to b46f60b Compare July 28, 2021 06:27

kokosing reviewed Jul 28, 2021

View reviewed changes

kokosing requested changes Jul 28, 2021

View reviewed changes

11xor6 force-pushed the rewrite-oidc-implementation branch from b46f60b to c581e04 Compare July 28, 2021 19:37

11xor6 force-pushed the rewrite-oidc-implementation branch from feef632 to 27ebe5a Compare August 3, 2021 02:09

11xor6 mentioned this pull request Aug 3, 2021

Correct assumption that OIDC access token is always a JWT #8250

Closed

11xor6 force-pushed the rewrite-oidc-implementation branch 2 times, most recently from 799ac9b to 17e0817 Compare August 3, 2021 04:20

11xor6 force-pushed the rewrite-oidc-implementation branch from 17e0817 to 3c7392d Compare August 3, 2021 21:23

Correct assumption that OIDC access token is always a JWT

5a938d2

11xor6 force-pushed the rewrite-oidc-implementation branch 6 times, most recently from 3b30853 to 34f07a2 Compare August 5, 2021 05:45

lukasz-walkiewicz approved these changes Aug 9, 2021

View reviewed changes

kokosing approved these changes Aug 10, 2021

View reviewed changes

11xor6 mentioned this pull request Aug 18, 2021

Release notes for 361 #8732

Closed

10 tasks

hashhar reviewed Aug 19, 2021

View reviewed changes

core/trino-main/src/main/java/io/trino/server/security/oauth2/OAuth2Config.java Show resolved Hide resolved

Nik Hodgkinson added 3 commits August 19, 2021 13:08

Add support for using the OIDC /userinfo endpoint

4ea742a

Rename AccessToken to OAuth2Response

1384f52

11xor6 force-pushed the rewrite-oidc-implementation branch from 34f07a2 to 1384f52 Compare August 19, 2021 20:08

dain merged commit 7cdd133 into trinodb:master Aug 19, 2021

hashhar reviewed Aug 25, 2021

View reviewed changes

11xor6 deleted the rewrite-oidc-implementation branch September 8, 2021 23:32

lukasz-walkiewicz added a commit to lukasz-walkiewicz/presto that referenced this pull request Dec 7, 2021

Remove outdated note from OAuth2 docs

e721e52

Opaque tokens are supported since 361: trinodb#8641

lukasz-walkiewicz mentioned this pull request Dec 7, 2021

Remove outdated note from OAuth2 docs #10219

Merged

kokosing pushed a commit that referenced this pull request Dec 7, 2021

Remove outdated note from OAuth2 docs

ccdbb5b

Opaque tokens are supported since 361: #8641

sumannewton pushed a commit to sumannewton/trino that referenced this pull request Jan 17, 2022

Remove outdated note from OAuth2 docs

3399ffa

Opaque tokens are supported since 361: trinodb#8641

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite OIDC implementation #8641

Rewrite OIDC implementation #8641

11xor6 commented Jul 22, 2021

dain Jul 23, 2021

11xor6 Jul 28, 2021 •

edited

Loading

kokosing Jul 28, 2021

11xor6 Jul 28, 2021

kokosing Jul 29, 2021

dain Jul 29, 2021 •

edited

Loading

kokosing Jul 28, 2021

dain Jul 28, 2021 •

edited

Loading

kokosing Jul 29, 2021

dain Jul 29, 2021

kokosing Jul 28, 2021

11xor6 Jul 28, 2021

kokosing left a comment

kokosing commented Aug 3, 2021

11xor6 commented Aug 5, 2021 •

edited

Loading

lukasz-walkiewicz left a comment

lukasz-walkiewicz Aug 5, 2021

11xor6 Aug 12, 2021

lukasz-walkiewicz Aug 5, 2021

hashhar Aug 25, 2021

hashhar Aug 25, 2021

Rewrite OIDC implementation #8641

Rewrite OIDC implementation #8641

Conversation

11xor6 commented Jul 22, 2021

Choose a reason for hiding this comment

11xor6 Jul 28, 2021 • edited Loading

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

dain Jul 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dain Jul 28, 2021 • edited Loading

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 left a comment

Choose a reason for hiding this comment

kokosing commented Aug 3, 2021

11xor6 commented Aug 5, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

11xor6 Jul 28, 2021 •

edited

Loading

dain Jul 29, 2021 •

edited

Loading

dain Jul 28, 2021 •

edited

Loading

11xor6 commented Aug 5, 2021 •

edited

Loading