DRIVERS-2836 OIDC: Clarify that TOKEN_RESOURCE must be url-encoded #1569

blink1073 · 2024-04-24T11:22:27Z

Please complete the following before merging:

Update changelog.
Make sure there are generated JSON files from the YAML test files.
Test changes in at least one language driver.
Test these changes against all server versions and topologies (including standalone, replica set, sharded
clusters, and serverless).

blink1073 · 2024-04-24T11:23:10Z

Tested with mongodb/mongo-python-driver#1616

source/auth/auth.md

source/auth/tests/legacy/connection-string.json

source/auth/auth.md

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

sanych-sun · 2024-04-24T20:42:06Z

source/auth/auth.md

@@ -1224,7 +1224,8 @@ in the MONGODB-OIDC specification, including sections or blocks that specificall
  - TOKEN_RESOURCE\
    The URI of the target resource. If `TOKEN_RESOURCE` is provided and `ENVIRONMENT` is not one of
    `["azure", "gcp"]` or `TOKEN_RESOURCE` is not provided and `ENVIRONMENT` is one of `["azure", "gcp"]`, the driver
-    MUST raise an error.
+    MUST raise an error. Drivers MUST ensure that `TOKEN_RESOURCE` is url-encoded, such as by using a regex for special


I would prefer to have plain value here, because if some driver decide to use Azure/GCP/other SDK to pass the value - most likely SDK will do encoding on it's end. The fact that the value is being used as a url query parameter - this is implementation details and might be vary (if some other provider will use POST and pass the value in request body).

But then in the Azure and GCP section we have to require driver to encode the value as it being used as a query parameter.

sanych-sun · 2024-04-24T21:11:48Z

source/auth/tests/legacy/connection-string.json

+        "mechanism": "MONGODB-OIDC",
+        "mechanism_properties": {
+          "ENVIRONMENT": "azure",
+          "TOKEN_RESOURCE": "a%24b"


Should we expect a$b here?

No, because it is azure and we would have encoded it.

source/auth/tests/legacy/connection-string.yml

source/auth/auth.md

source/auth/tests/legacy/connection-string.yml

source/auth/auth.md

source/auth/tests/legacy/connection-string.yml

blink1073 · 2024-04-26T00:56:42Z

@sanych-sun @katcharov I've updated the description and tests based on feedback

blink1073 · 2024-04-26T00:58:56Z

Updated tests in mongodb/mongo-python-driver#1620

sanych-sun

The latest revision looks good to me. Will implement the changes and verify if tests are working as expected for CSHARP Driver. If everything works well - will LGTM it later today.

sanych-sun · 2024-04-26T19:28:58Z

source/auth/tests/legacy/connection-string.yml

+      ENVIRONMENT: azure
+      TOKEN_RESOURCE: 'mongodb://test-cluster'
+- description: should accept an un-encoded TOKEN_RESOURCE (MONGODB-OIDC)
+  uri: mongodb://user@localhost/?authMechanism=MONGODB-OIDC&authMechanismProperties=ENVIRONMENT:azure,TOKEN_RESOURCE:mongodb://test-cluster


Is this Uri valid? Should we have : in the mongodb://test-cluster encoded as %3A? Having more then one value separator confuses our parser. And it's kind of ambiguous:
TOKEN_RESOURCE:mongodb://test-cluster
should it be parsed as TOKEN_RESOURCE=mongodb://test-cluster or TOKEN_RESOURCE:mongodb=//test-cluster

This usage is against the advice we give to users in the spec, but I think we should optimistically decode this (like this in Java), rather than failing. Although it is indeed ambiguous, I think we will never put a : in the name of any property (precisely because it is a separator).

Yep, can do the same, just want to double check if we see this as expected behavior.

alcaeus

One minor note, LGTM other than that.

source/auth/auth.md

Co-authored-by: Andreas Braun <git@alcaeus.org>

sanych-sun

LGTM

source/auth/auth.md

source/auth/tests/legacy/connection-string.json

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

katcharov

LGTM, with one comment

source/auth/auth.md

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

DRIVERS-2836 OIDC: Clarify that TOKEN_RESOURCE must be url-encoded

a947708

blink1073 requested a review from a team as a code owner April 24, 2024 11:22

blink1073 requested review from comandeo-mongo, sanych-sun and katcharov and removed request for a team and comandeo-mongo April 24, 2024 11:22

blink1073 closed this Apr 24, 2024

blink1073 reopened this Apr 24, 2024

katcharov requested changes Apr 24, 2024

View reviewed changes

source/auth/auth.md Outdated Show resolved Hide resolved

source/auth/tests/legacy/connection-string.json Outdated Show resolved Hide resolved

source/auth/auth.md Show resolved Hide resolved

blink1073 and others added 2 commits April 24, 2024 15:29

Update source/auth/tests/legacy/connection-string.json

c397684

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

update other urls

36c11c5

sanych-sun requested changes Apr 24, 2024

View reviewed changes

blink1073 added 3 commits April 24, 2024 15:43

make it more explicit

5a663a0

move logic to respective sections

d176681

remove dead links

518ef62

blink1073 requested a review from a team as a code owner April 24, 2024 20:50

blink1073 requested review from alcaeus and removed request for a team April 24, 2024 20:50

sanych-sun reviewed Apr 24, 2024

View reviewed changes

blink1073 requested review from sanych-sun and katcharov April 25, 2024 00:40

katcharov reviewed Apr 25, 2024

View reviewed changes

source/auth/tests/legacy/connection-string.yml Outdated Show resolved Hide resolved

katcharov reviewed Apr 25, 2024

View reviewed changes

source/auth/auth.md Outdated Show resolved Hide resolved

source/auth/tests/legacy/connection-string.yml Outdated Show resolved Hide resolved

source/auth/auth.md Show resolved Hide resolved

katcharov reviewed Apr 25, 2024

View reviewed changes

source/auth/tests/legacy/connection-string.yml Outdated Show resolved Hide resolved

katcharov mentioned this pull request Apr 25, 2024

DOCSP-34479 - OIDC mongodb/docs-java#537

Merged

5 tasks

stIncMale mentioned this pull request Apr 25, 2024

OIDC Add remaining environments (azure, gcp), evergreen testing, API naming updates mongodb/mongo-java-driver#1371

Merged

address review and add more tests

b5633b0

blink1073 requested a review from katcharov April 26, 2024 00:56

sanych-sun reviewed Apr 26, 2024

View reviewed changes

katcharov mentioned this pull request Apr 26, 2024

Implement OIDC SASL mechanism mongodb/mongo-java-driver#1134

Merged

sanych-sun reviewed Apr 26, 2024

View reviewed changes

alcaeus reviewed Apr 29, 2024

View reviewed changes

source/auth/auth.md Outdated Show resolved Hide resolved

Update source/auth/auth.md

64c9ea2

Co-authored-by: Andreas Braun <git@alcaeus.org>

blink1073 requested a review from sanych-sun April 29, 2024 12:35

sanych-sun approved these changes Apr 29, 2024

View reviewed changes

katcharov reviewed Apr 29, 2024

View reviewed changes

source/auth/auth.md Outdated Show resolved Hide resolved

source/auth/auth.md Outdated Show resolved Hide resolved

source/auth/tests/legacy/connection-string.json Show resolved Hide resolved

blink1073 and others added 2 commits April 29, 2024 11:42

Update source/auth/auth.md

3345a20

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

address review

a754728

blink1073 requested a review from katcharov April 29, 2024 16:48

sanych-sun mentioned this pull request Apr 29, 2024

CSHARP-4610: OIDC: Automatic token acquisition for GCP Identity Provider mongodb/mongo-csharp-driver#1315

Merged

katcharov approved these changes Apr 29, 2024

View reviewed changes

source/auth/auth.md Outdated Show resolved Hide resolved

Update source/auth/auth.md

a4d0725

Co-authored-by: Maxim Katcharov <maxim.katcharov@mongodb.com>

blink1073 merged commit c59c932 into mongodb:master Apr 29, 2024
3 checks passed

blink1073 deleted the DRIVERS-2836-2 branch April 29, 2024 20:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRIVERS-2836 OIDC: Clarify that TOKEN_RESOURCE must be url-encoded #1569

DRIVERS-2836 OIDC: Clarify that TOKEN_RESOURCE must be url-encoded #1569

blink1073 commented Apr 24, 2024

blink1073 commented Apr 24, 2024

sanych-sun Apr 24, 2024

sanych-sun Apr 24, 2024

blink1073 Apr 24, 2024

sanych-sun Apr 24, 2024

blink1073 Apr 24, 2024

blink1073 commented Apr 26, 2024

blink1073 commented Apr 26, 2024

sanych-sun left a comment

sanych-sun Apr 26, 2024 •

edited

katcharov Apr 26, 2024

sanych-sun Apr 26, 2024

alcaeus left a comment

sanych-sun left a comment

katcharov left a comment

DRIVERS-2836 OIDC: Clarify that TOKEN_RESOURCE must be url-encoded #1569

DRIVERS-2836 OIDC: Clarify that TOKEN_RESOURCE must be url-encoded #1569

Conversation

blink1073 commented Apr 24, 2024

blink1073 commented Apr 24, 2024

sanych-sun Apr 24, 2024

Choose a reason for hiding this comment

sanych-sun Apr 24, 2024

Choose a reason for hiding this comment

blink1073 Apr 24, 2024

Choose a reason for hiding this comment

sanych-sun Apr 24, 2024

Choose a reason for hiding this comment

blink1073 Apr 24, 2024

Choose a reason for hiding this comment

blink1073 commented Apr 26, 2024

blink1073 commented Apr 26, 2024

sanych-sun left a comment

Choose a reason for hiding this comment

sanych-sun Apr 26, 2024 • edited

Choose a reason for hiding this comment

katcharov Apr 26, 2024

Choose a reason for hiding this comment

sanych-sun Apr 26, 2024

Choose a reason for hiding this comment

alcaeus left a comment

Choose a reason for hiding this comment

sanych-sun left a comment

Choose a reason for hiding this comment

katcharov left a comment

Choose a reason for hiding this comment

sanych-sun Apr 26, 2024 •

edited