Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(NODE-2026): support SERVICE_REALM authentication mechanism property #2865

Merged

Conversation

rose-m
Copy link
Contributor

@rose-m rose-m commented Jun 28, 2021

Introduces support for the SERVICE_REALM authentication mechanism property. It will be work as expected in Windows environments due to the underlying nature of the GSSAPI implementation on *nix.

The logic to compute the Service Principal Name (SPN) is as follows:

  1. Combine service name and hostname (with a / in between for Windows and an @ otherwise)
  2. If present, attach the SERVICE_REALM with an @ to the result of step 1.

The logic is taken from the C driver where you can find the steps mangled as follows:

  1. "Join" the array of [serviceName, hostName, serviceRealm] with @ (see https://github.com/mongodb/mongo-c-driver/blob/f1093ba1686b8bf876eed67e0c7f59918649ef15/src/libmongoc/src/mongoc/mongoc-cluster-sspi.c#L61-L71)
  2. If there is no / in the SPN (which is not the case after step 1), replace the first @ with a / (see https://github.com/mongodb/mongo-c-driver/blob/5b91fb983fc5badf38e2debeb4ce6c231b73605b/src/libmongoc/src/mongoc/mongoc-sspi.c#L176-L182)

I was able to successfully test this behavior in a Windows environment with Kerberos enabled.

@rose-m rose-m force-pushed the NODE-2026-support-service-realm-auth-property branch from 5aa9efd to a075681 Compare June 29, 2021 07:14
@durran durran self-assigned this Jun 29, 2021
@durran durran added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Jun 29, 2021
@nbbeeken nbbeeken requested review from emadum and nbbeeken June 30, 2021 16:11
@behackett behackett assigned nbbeeken and unassigned durran Jun 30, 2021
Copy link
Contributor

@nbbeeken nbbeeken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for another example here's the logic in python I believe the only issue we have here is the limiting of SERVICE_REALM only on windows.

if ('SERVICE_REALM' in mechanismProperties) {
spn = `${spn}@${mechanismProperties.SERVICE_REALM}`;
}
);
} else {
spn = `${serviceName}@${host}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to keep the logic that uses slash instead of @ on windows, but move attaching the service realm to outside so it gets attached regardless of platform.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test: validate that SERVICE_REALM and CANONICALIZE_HOST_NAME can be passed in
in: test/manual/kerberos.test.js:54
is now going to fail after making this change, you can safely skip it, we have a ticket (NODE-3060) to track unskipping / removing later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discussed that with @mcasimir, too... So we want to just attach that regardless of the platform knowing it will not work on any *nix systems? I've no hard feelings on that - I'll add it :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea I'm just looking at python and mirroring their logic, we will revisit this when we have the right testing environment. You were able to confirm it works on windows so I'm happy with what we have here, thanks for this!

@rose-m rose-m force-pushed the NODE-2026-support-service-realm-auth-property branch from a075681 to c0544fa Compare July 1, 2021 07:32
@rose-m
Copy link
Contributor Author

rose-m commented Jul 1, 2021

Note: I was also able to successfully test the code in a Windows Kerberos environment (manually).

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nbbeeken nbbeeken merged commit 5caa354 into mongodb:4.0 Jul 1, 2021
@rose-m rose-m deleted the NODE-2026-support-service-realm-auth-property branch July 2, 2021 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Primary Review In Review with primary reviewer, not yet ready for team's eyes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants