Skip to content

DOCSP-13829 authentication mechanism #18

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

Merged
merged 10 commits into from
Aug 16, 2021

Conversation

kyuan-mongodb
Copy link
Contributor

@kyuan-mongodb kyuan-mongodb commented Aug 9, 2021

Pull Request Info

Added Fundamentals > Authentication Mechanisms page

Utilized snippets from Go driver repo: https://github.com/mongodb/mongo-go-driver/blob/master/mongo/client_examples_test.go

Based the format off of the Node and Java Auth Mechanism pages.

Issue JIRA link:

https://jira.mongodb.org/browse/DOCSP-13829

Docs staging link (requires sign-in on MongoDB Corp SSO):

https://docs-mongodbcom-staging.corp.mongodb.com/golang/docsworker-xlarge/DOCSP-13829-AuthMechanisms/fundamentals/auth/

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Does it render on staging correctly?
  • Are all the links working?
  • Are the staging links in the PR description updated?

Copy link
Member

@terakilobyte terakilobyte left a comment

Choose a reason for hiding this comment

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

It looks like this is nearly identical to the Java page (minus the go specifics), but removes one of the headings. I think it worked well.

Copy link
Member

@terakilobyte terakilobyte left a comment

Choose a reason for hiding this comment

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

LGTM with one correction needed.

- MongoDB 3.0, 3.2, 3.4, and 3.6

* - ``MONGODB-CR``
- MongoDB 3.6 and earlier
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be 2.6 and earlier.

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, I found the mention in the server manual release notes and it says it was depreciated as of 3.6 -- maybe the edit is "earlier than MongoDB 3.6"?

Choose a reason for hiding this comment

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

I'm not super familiar with default authentication mechanisms on the server, but if MONGODB-CR is 3.6 and earlier, is the default on 3.0 through 3.6 SCRAM-SHA-1 or MONGODB-CR?

Choose a reason for hiding this comment

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

Based on chooseAuthMechanism it looks like we pick SCRAM-SHA-256 if it's supported, falling back to SCRAM-SHA-1 if not, and finally to MONGODB-CR if neither is supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The link you provided contains /x, which I was told is experimental and not for public use.

Choose a reason for hiding this comment

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

As discussed offline: most of our authentication logic is "unstable" API and is therefore under /x. I wouldn't point to code under /x in documentation, but I was just linking to that code here to explain how the Go driver actually picks the "default" auth mechanism.

@kyuan-mongodb
Copy link
Contributor Author

Hi @kevinAlbs, This PR is ready for review.

@kevinAlbs kevinAlbs requested review from benjirewis and removed request for kevinAlbs August 11, 2021 21:24
Copy link

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Looking good; have some suggestions.

The Go driver also offers GSSAPI and PLAIN authentication mechanisms that might be worth mentioning.

- MongoDB 3.0, 3.2, 3.4, and 3.6

* - ``MONGODB-CR``
- MongoDB 3.6 and earlier

Choose a reason for hiding this comment

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

I'm not super familiar with default authentication mechanisms on the server, but if MONGODB-CR is 3.6 and earlier, is the default on 3.0 through 3.6 SCRAM-SHA-1 or MONGODB-CR?

- MongoDB 3.0, 3.2, 3.4, and 3.6

* - ``MONGODB-CR``
- MongoDB 3.6 and earlier

Choose a reason for hiding this comment

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

Based on chooseAuthMechanism it looks like we pick SCRAM-SHA-256 if it's supported, falling back to SCRAM-SHA-1 if not, and finally to MONGODB-CR if neither is supported.

@@ -72,7 +73,7 @@ mechanisms depending on what MongoDB versions your server supports:
- MongoDB 3.0, 3.2, 3.4, and 3.6

* - ``MONGODB-CR``
- MongoDB 3.6 and earlier
- MongoDB 3.6 and earlier if the server doesn't support``SCRAM-SHA-1``
Copy link
Member

Choose a reason for hiding this comment

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

This still isn't correct. This table is for the default it looks like. MongoDB 3.0 changed the default from MongoDB-CR to SCRAM-*.

@benjirewis
Copy link

As discussed offline: while I do believe you can explicitly specify MONGODB-CR as an auth mechanism, our documentation about its deprecation and eventual removal was slightly incorrect. A fix is in review now.

Copy link

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work.

Copy link
Member

@terakilobyte terakilobyte left a comment

Choose a reason for hiding this comment

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

lgtm

@kyuan-mongodb kyuan-mongodb merged commit a595baa into mongodb:master Aug 16, 2021
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.

3 participants