OIDC Add remaining environments (azure, gcp), evergreen testing, API naming updates #1371

katcharov · 2024-04-19T20:41:48Z

JAVA-5353 API naming
JAVA-5395 Atlas testing
JAVA-4834 Azure
JAVA-4932 GCP

katcharov · 2024-04-19T20:43:43Z

driver-core/src/main/com/mongodb/ConnectionString.java

@@ -916,7 +916,7 @@ private MongoCredential createCredentials(final Map<String, List<String>> option

        if (credential != null && authMechanismProperties != null) {
            for (String part : authMechanismProperties.split(",")) {
-                String[] mechanismPropertyKeyValue = part.split(":");
+                String[] mechanismPropertyKeyValue = part.split(":", 2);


This ignores the second : (a test passes in a value that decodes to a string that includes :).

…g updates

katcharov · 2024-04-23T17:47:39Z

Rebased feature-oidc and this branch on latest master, no conflicts.

driver-core/src/main/com/mongodb/MongoCredential.java

katcharov · 2024-04-24T16:20:00Z

driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java

@@ -69,11 +73,20 @@
 */
 public final class OidcAuthenticator extends SaslAuthenticator {

-    private static final List<String> SUPPORTED_PROVIDERS = Arrays.asList("aws");
+    private static final String TEST_ENVIRONMENT = "test";


This is only used (enabled) in unified tests. The "Entities" class could instead check if this environment is set, and supply the test callback under OIDC_CALLBACK_KEY, which is a mild abuse of mechanism properties, but would avoid the (limited) code needed in the driver to support this.

katcharov · 2024-04-24T16:25:02Z

driver-core/src/test/unit/com/mongodb/ConnectionStringSpecification.groovy

@@ -601,7 +601,7 @@ class ConnectionStringSpecification extends Specification {
        new ConnectionString('mongodb://jeff@localhost/?' +
                             'authMechanism=GSSAPI' +
                             '&authMechanismProperties=' +
-                             'SERVICE_NAME:foo:bar')
+                             'SERVICE_NAMEbar') // missing =


No spec was associated with the Jira ticket where this check was added. OIDC tests include a value with a second : (when decoded). Rather than remove, I replaced with a different invalidity check.

.evergreen/.evg.yml

...r-sync/src/test/functional/com/mongodb/internal/connection/OidcAuthenticationProseTests.java

.evergreen/run-mongodb-oidc-test.sh

blink1073 · 2024-04-25T00:43:42Z

.evergreen/.evg.yml

@@ -2216,6 +2339,27 @@ buildvariants:
  tasks:
    - name: "test_atlas_task_group_search_indexes"

+- name: "oidc-auth-test"
+  display_name: "OIDC Auth"
+  run_on: ubuntu2204-small


We explicitly test on Windows and MacOS in PyMongo, but I'd consider that optional.

All of our tests use linux/ubuntu as the os, so I took those out.

blink1073 · 2024-04-25T00:46:04Z

driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java

+    public static OidcCallback getAzureCallback(final MongoCredential credential) {
+        return (context) -> {
+            String resource = assertNotNull(credential.getMechanismProperty(TOKEN_RESOURCE_KEY, null));
+            String objectId = credential.getUserName();


This is actually the clientId

Hmm, my ensuing fetchAzure... call is calling:

public static CredentialInfo fetchAzureCredentialInfo(final String resource, @Nullable final String objectId) { String endpoint = "http://169.254.169.254:80" + "/metadata/identity/oauth2/token?api-version=2018-02-01" + "&resource=" + resource + (objectId == null ? "" : "&object_id=" + objectId);

This must be incorrect, since the spec says to use:

http://169.254.169.254/metadata/identity/oauth2/token?api-version=2018-02-01&resource=<resource>&client_id=<client_id>

I'll update this, but why do the tests still pass, and was this changed from object_id?

It was changed from object_id to align with the Azure SDK that uses client_id.

driver-core/src/main/com/mongodb/internal/authentication/GcpCredentialHelper.java

driver-core/src/main/com/mongodb/internal/authentication/AzureCredentialHelper.java

driver-core/src/main/com/mongodb/MongoCredential.java

driver-core/src/main/com/mongodb/internal/authentication/CredentialInfo.java

driver-sync/src/test/functional/com/mongodb/client/unified/Entities.java

Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>

…ntialInfo.java Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>

driver-core/src/main/com/mongodb/internal/connection/OidcAuthenticator.java

...r-sync/src/test/functional/com/mongodb/internal/connection/OidcAuthenticationProseTests.java

blink1073

LGTM!

...r-sync/src/test/functional/com/mongodb/internal/connection/OidcAuthenticationProseTests.java

driver-core/src/main/com/mongodb/internal/authentication/AzureCredentialHelper.java

driver-sync/src/test/functional/com/mongodb/client/unified/Entities.java

driver-core/src/main/com/mongodb/internal/Locks.java

stIncMale · 2024-04-26T20:24:34Z

driver-core/src/test/resources/auth/legacy/connection-string.json

Commits d856d84 and cc1c7ec in this PR take changes from mongodb/specifications#1569, which hasn't been approved and merged. Are we sure this is the way to go? I realize that we are short of time, but taking spec changes that are still work in progress (if this were not the case, they would have been merged) is not the right process, and we may end up having tests that have never been merged to the spec.

I would also expect that if there is time pressure on the driver, the same (even more, actually) should be true for the spec changes, because they have to go before the driver changes.

Maxim, if you have already confirmed this with Jeff, then OK. If not, then could you please confirm? (alternatively, I can do that)

I agree that we should block merging of at least the OIDC feature PR on the approval and merging of that PR (and I have no issues with blocking this PR since nothing depends on it).

driver-core/src/main/com/mongodb/ConnectionString.java

driver-core/src/test/unit/com/mongodb/ConnectionStringUnitTest.java

driver-core/src/main/com/mongodb/ConnectionString.java

stIncMale · 2024-04-29T17:12:41Z

driver-core/src/test/unit/com/mongodb/ConnectionStringUnitTest.java

+        // encoded tags will fail parsing with an "invalid read preference tag"
+        // error if decoding is skipped.
+        String encodedTags = encode("dc:ny,rack:1");


stIncMale

Just a small thing

...r-sync/src/test/functional/com/mongodb/internal/connection/OidcAuthenticationProseTests.java

…n/OidcAuthenticationProseTests.java Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>

* Implement OIDC SASL mechanism in sync (#1107) JAVA-4980 * Implement OIDC auth for async (#1131) JAVA-4981 * Remove non-machine workflow (#1259) JAVA-5077 * Add Human OIDC Workflow (#1316) JAVA-5328 * OIDC Add remaining environments (azure, gcp), evergreen testing, API naming updates (#1371) JAVA-5353 JAVA-5395 JAVA-4834 JAVA-4932 Co-authored-by: Valentin Kovalenko <valentin.kovalenko@mongodb.com>

katcharov commented Apr 19, 2024

View reviewed changes

katcharov requested a review from stIncMale April 23, 2024 16:23

katcharov marked this pull request as ready for review April 23, 2024 16:24

katcharov added 4 commits April 23, 2024 11:41

Add remaining environments (azure, gcp), evergreen testing, API namin…

240af4c

…g updates

Add remaining tests, refactor, increase GCP test machine

b84ca99

Cleanup, update since annotations, updates to match spec API

df3ef8d

Test fixes

581ae2e

katcharov force-pushed the feature-oidc branch from a67791b to 8b80055 Compare April 23, 2024 17:45

katcharov force-pushed the JAVA-oidc-4395 branch from b277316 to 581ae2e Compare April 23, 2024 17:46

katcharov commented Apr 23, 2024

View reviewed changes

driver-core/src/main/com/mongodb/MongoCredential.java Outdated Show resolved Hide resolved

PR fixes

00e4c15

katcharov requested a review from blink1073 April 23, 2024 21:16

katcharov commented Apr 24, 2024

View reviewed changes

Remove admin credentials

6898b4f

katcharov mentioned this pull request Apr 24, 2024

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

Merged

4 tasks

stIncMale requested changes Apr 24, 2024

View reviewed changes

blink1073 reviewed Apr 25, 2024

View reviewed changes

PR fixes

2621ae8

stIncMale requested changes Apr 25, 2024

View reviewed changes

katcharov and others added 6 commits April 25, 2024 17:07

PR fixes

c842d22

Apply suggestions from code review

3cea409

Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>

Update driver-core/src/main/com/mongodb/MongoCredential.java

e883279

Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>

Update driver-core/src/main/com/mongodb/internal/authentication/Crede…

bc30a2f

…ntialInfo.java Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>

Implement OIDC map value splitting

d856d84

PR fixes, doc updates

479fcdd

katcharov requested review from stIncMale and blink1073 April 26, 2024 16:15

stIncMale requested changes Apr 26, 2024

View reviewed changes

blink1073 approved these changes Apr 26, 2024

View reviewed changes

katcharov added 2 commits April 26, 2024 12:54

PR fixes for OIDC feature branch

4a844b1

Update connection-string latest specifications/pull/1569

cc1c7ec

stIncMale requested changes Apr 26, 2024

View reviewed changes

katcharov added 3 commits April 26, 2024 14:41

PR fixes

678d7b7

PR fixes

fcb65dc

PR fixes

71b3846

stIncMale requested changes Apr 26, 2024

View reviewed changes

katcharov added 2 commits April 26, 2024 17:42

PR Fixes

f6cb3da

PR fixes

be63643

katcharov requested a review from stIncMale April 27, 2024 01:04

stIncMale reviewed Apr 29, 2024

View reviewed changes

katcharov added 3 commits April 29, 2024 11:25

PR fixes

0532a87

PR fixes: mustDecodeNonOidcAsWhole

761918e

PR fixes

7428fd1

stIncMale requested changes Apr 29, 2024

View reviewed changes

...r-sync/src/test/functional/com/mongodb/internal/connection/OidcAuthenticationProseTests.java Outdated Show resolved Hide resolved

Update driver-sync/src/test/functional/com/mongodb/internal/connectio…

fcdab29

…n/OidcAuthenticationProseTests.java Co-authored-by: Valentin Kovalenko <valentin.male.kovalenko@gmail.com>

stIncMale approved these changes Apr 29, 2024

View reviewed changes

Doc fix

8971a79

katcharov merged commit 106ee4d into mongodb:feature-oidc Apr 29, 2024
2 checks passed

katcharov deleted the JAVA-oidc-4395 branch April 29, 2024 21:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OIDC Add remaining environments (azure, gcp), evergreen testing, API naming updates #1371

OIDC Add remaining environments (azure, gcp), evergreen testing, API naming updates #1371

katcharov commented Apr 19, 2024 •

edited

katcharov Apr 19, 2024

katcharov commented Apr 23, 2024

katcharov Apr 24, 2024

katcharov Apr 24, 2024

blink1073 Apr 25, 2024

katcharov Apr 25, 2024

blink1073 Apr 25, 2024

katcharov Apr 25, 2024

blink1073 Apr 25, 2024

blink1073 left a comment

stIncMale Apr 26, 2024

katcharov Apr 26, 2024

stIncMale Apr 29, 2024

stIncMale left a comment

OIDC Add remaining environments (azure, gcp), evergreen testing, API naming updates #1371

OIDC Add remaining environments (azure, gcp), evergreen testing, API naming updates #1371

Conversation

katcharov commented Apr 19, 2024 • edited

Choose a reason for hiding this comment

katcharov commented Apr 23, 2024

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

blink1073 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

stIncMale left a comment

Choose a reason for hiding this comment

katcharov commented Apr 19, 2024 •

edited