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

MONGOSH-1906 Add OIDC workforce auth requirements docs #1731

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

addaleax
Copy link
Contributor

Thank you @alexbevi for the support here 🙂

@addaleax addaleax requested a review from a team as a code owner November 14, 2024 14:41
@addaleax addaleax requested review from JamesKovacs and removed request for a team November 14, 2024 14:41
docs/workforce-human-oidc-auth.md Outdated Show resolved Hide resolved
docs/workforce-human-oidc-auth.md Outdated Show resolved Hide resolved
@addaleax addaleax requested a review from a team as a code owner November 14, 2024 15:37
@blink1073 blink1073 self-requested a review November 14, 2024 16:31
@addaleax addaleax changed the title Add OIDC workforce auth requirements docs MONGOSH-1906 MONGOSH-1906 Add OIDC workforce auth requirements docs Nov 19, 2024
Copy link
Contributor

@pmeredit pmeredit left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

.github/CODEOWNERS Show resolved Hide resolved
docs/workforce-human-oidc-auth.md Outdated Show resolved Hide resolved
docs/workforce-human-oidc-auth.md Outdated Show resolved Hide resolved
Copy link
Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

Overall fantastic. Requesting some minor changes.

docs/workforce-human-oidc-auth.md Outdated Show resolved Hide resolved
Currently, users who connect to a host other than localhost or an Atlas hostname need to explicitly opt-in into being
able to do so by setting the `ALLOWED_HOSTS` flag (specified in the drivers auth spec). In the future, MongoDB is hoping
to support Demonstrating Proof of Possession (DPoP, [RFC9449](https://datatracker.ietf.org/doc/html/rfc9449)) that will
allow lifting this restriction. The goal here is to prevent users from connecting to untrusted endpoints that will
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest omitting this forward-looking statement about implementing DPoP.

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've moved these into an appendix section at the end of the file and cleaned them up a bit. I do think that these are items that implementers should be aware of, especially considering that "accidentally" implementing these may be counterproductive – depending on the OIDC library used, it may be quite easy to do so, but there would be a risk of doing it in a way that is incompatible with what we end up standardizing on in the future.

It's a good reminder, though. In particular RFC8707 adoption is essentially just a client-side thing that we need to figure out and shouldn't require server support, so I've opened https://jira.mongodb.org/browse/MONGOSH-1937 to make sure we do end up investigating this option.

endpoints in general).

We would also like to generally adopt [RFC8707](https://datatracker.ietf.org/doc/html/rfc8707), but have not decided on
a specific format for expressing the MongoDB endpoints as resources.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here as we may never implement this feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above :)


- The scopes listed in the `requestScopes` field of the IdP response from the MongoDB endpoint, and
- The `openid` and `offline_access` scopes[^3]. If the IdP metadata document contains a `scopes_supported` field, and
this field does not list one of these scopes, then the respective scope SHOULD NOT be added to the `scope`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a SHOULD NOT or a MUST NOT? Does auth fail if an unsupported scope is provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, yeah, it's a good question, and I've gone a bit back and forth on this in response to you bringing it up 🙂

I think the RFC2119 definition of "SHOULD NOT" is ultimately the right one here. The implementer would need to accept that their application's authentication attempts could be rejected by an IdP that doesn't provide refresh tokens (as that is what offline_access asks for). That seems like a sensible behavior for an application that strictly requires ongoing access to the cluster while it is active – e.g. mongosync might want to make this choice (Not saying they do, but that it would be understandable if they did).

That being said, if you feel strongly here, I'm still happy to make this change.

Does auth fail if an unsupported scope is provided?

It's up to the individual IdP (this sentence referring to the IdP/server side of OIDC):

Scope values used that are not understood by an implementation SHOULD be ignored.

docs/workforce-human-oidc-auth.md Outdated Show resolved Hide resolved
@addaleax addaleax requested a review from JamesKovacs December 5, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants