Support GET method on user info endpoint (oauth) #11014

mdesmet · 2022-02-10T23:54:28Z

Description

General information

Is this change a fix, improvement, new feature, refactoring, or other?

This is new feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

It's a change to the core query engine

How would you describe this change to a non-technical end user or system administrator?

This feature allows to configure the requests method of the user info endpoint for oauth2 authentication for web ui and jdbc/odbc drivers.

Related issues, pull requests, and links

FIxes OAuth2 userInfoUrl request method cannot be configured #11013

Documentation

( ) No documentation is needed.
( *) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( *) No release notes entries required.
( ) Release notes entries required with the following suggested text:

kokosing · 2022-02-11T10:13:08Z

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

@@ -332,7 +333,7 @@ else if (nonce.isPresent() != oauth2Response.getIdToken().isPresent()) {
        if (userinfoUri.isPresent()) {
            // validate access token is trusted by remote userinfo endpoint
            Request request = Request.builder()
-                    .setMethod(POST)
+                    .setMethod(userInfoRequestMethod)


Can we simply change it to GET?

Both methods are allowed according to the spec.

According to https://openid.net/specs/openid-connect-core-1_0.html#UserInfoRequest GET is recommended. The thing that we can use both methods does not mean we need to. I would prefer to have less toggles and use GET if is more widely. The issue is that we need to make sure we don't introduce regression.

lukasz-walkiewicz · 2022-02-11T10:15:43Z

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

@@ -332,7 +333,7 @@ else if (nonce.isPresent() != oauth2Response.getIdToken().isPresent()) {
        if (userinfoUri.isPresent()) {
            // validate access token is trusted by remote userinfo endpoint
            Request request = Request.builder()
-                    .setMethod(POST)
+                    .setMethod(userInfoRequestMethod)


Both methods are allowed according to the spec.

lukasz-walkiewicz · 2022-02-11T10:21:44Z

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

+    @ConfigDescription("userinfo request method")
+    public OAuth2Config setUserInfoRequestMethod(String userInfoRequestMethod)
+    {
+        this.userInfoRequestMethod = userInfoRequestMethod;


Please add

@PostConstruct public void validate() { }

ensuring that a valid HTTP method name was provided and a test for it.

I added the method and verified the valid states using checkState

lukasz-walkiewicz · 2022-02-11T10:22:27Z

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

@@ -45,6 +46,7 @@
    private String tokenUrl;
    private String jwksUrl;
    private Optional<String> userinfoUrl = Optional.empty();
+    private String userInfoRequestMethod = POST;


userinfoUrl uses a different casing

Since userinforUrl is optional, so userinforRequestMethod should be optional as well. Configuring:

http-server.authentication.oauth2.userinfo-request-method=GET

shouldn't be allowed without http-server.authentication.oauth2.userinfo-url specified.

All remarks have been processed. I've changed the casing, made the property optional and added state checks.

martint · 2022-02-13T02:54:31Z

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

@@ -45,6 +50,7 @@
    private String tokenUrl;
    private String jwksUrl;
    private Optional<String> userinfoUrl = Optional.empty();
+    private Optional<String> userinfoRequestMethod = Optional.empty();


Why is this configurable? Does oauth2 specify both methods for this type of API?

https://openid.net/specs/openid-connect-core-1_0.html#UserInfoRequest

#11013

11xor6

LGTM

kokosing · 2022-03-09T10:49:05Z

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

@@ -332,7 +334,7 @@ else if (nonce.isPresent() != oauth2Response.getIdToken().isPresent()) {
        if (userinfoUri.isPresent()) {
            // validate access token is trusted by remote userinfo endpoint
            Request request = Request.builder()
-                    .setMethod(POST)
+                    .setMethod(userInfoRequestMethod.orElse(POST))


Let's simply use GET

What about the risk of regression?

I'm good to change to GET, just a little worried it might break for some existing users: I've seen many custom OAuth enterprise implementations, not always strictly according spec.

I think @lukasz-walkiewicz have tested all the IdP we think we support and it was working just fine.

No doubt the commercial idp will work with both POST or GET. I see rather the risk within custom built oidc implementations. if their custom oidc implementation only has the POST endpoint and suddenly it's not working anymore after a Trino upgrade. As I said already, for me it's ok to make the change to GET. Will adapt the code later today.

We are not aware of any use case like this. Also it sounds very exotic, I would prefer not to support such use cases without hard evidence it is actually needed.

kokosing · 2022-03-11T10:46:51Z

No release notes are needed here.

kokosing · 2022-03-11T10:47:11Z

Merged, thanks!

cla-bot bot added the cla-signed label Feb 10, 2022

kokosing requested review from lukasz-walkiewicz and 11xor6 February 11, 2022 10:11

kokosing reviewed Feb 11, 2022

View reviewed changes

lukasz-walkiewicz reviewed Feb 11, 2022

View reviewed changes

mdesmet force-pushed the oidc_support_get_user_endpoint branch 4 times, most recently from 5ce060e to 5c0f901 Compare February 12, 2022 21:48

martint reviewed Feb 13, 2022

View reviewed changes

11xor6 approved these changes Mar 9, 2022

View reviewed changes

kokosing reviewed Mar 9, 2022

View reviewed changes

Use GET request method for calling OAuth2 userInfoUri

49972fb

mdesmet force-pushed the oidc_support_get_user_endpoint branch from 5c0f901 to 49972fb Compare March 10, 2022 21:48

kokosing approved these changes Mar 11, 2022

View reviewed changes

kokosing merged commit 74661d6 into trinodb:master Mar 11, 2022

github-actions bot added this to the 374 milestone Mar 11, 2022

mosabua mentioned this pull request Mar 11, 2022

Add Trino 374 release notes #11417

Merged

mdesmet deleted the oidc_support_get_user_endpoint branch March 15, 2022 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support GET method on user info endpoint (oauth) #11014

Support GET method on user info endpoint (oauth) #11014

mdesmet commented Feb 10, 2022 •

edited by hashhar

Loading

kokosing Feb 11, 2022

lukasz-walkiewicz Feb 11, 2022

kokosing Feb 13, 2022

lukasz-walkiewicz Feb 11, 2022

lukasz-walkiewicz Feb 11, 2022

mdesmet Feb 12, 2022

lukasz-walkiewicz Feb 11, 2022

lukasz-walkiewicz Feb 11, 2022

mdesmet Feb 12, 2022

martint Feb 13, 2022

kokosing Feb 13, 2022

11xor6 left a comment

kokosing Mar 9, 2022

mdesmet Mar 9, 2022

kokosing Mar 10, 2022

mdesmet Mar 10, 2022

kokosing Mar 10, 2022

kokosing commented Mar 11, 2022

kokosing commented Mar 11, 2022

Support GET method on user info endpoint (oauth) #11014

Support GET method on user info endpoint (oauth) #11014

Conversation

mdesmet commented Feb 10, 2022 • edited by hashhar Loading

Description

General information

Related issues, pull requests, and links

Documentation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

11xor6 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

kokosing commented Mar 11, 2022

kokosing commented Mar 11, 2022

mdesmet commented Feb 10, 2022 •

edited by hashhar

Loading