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

feat(cli-repl): add tlsCertificateSelector support on Win32 MONGOSH-573 #717

Merged
merged 6 commits into from Mar 15, 2021

Conversation

addaleax
Copy link
Contributor

No tests so far (including no manual ones), and I think testing this automatically would have to involve stubbing out the win-export-certificate-and-key package.

@addaleax addaleax changed the title [PoC] feat(cli-repl): add tlsCertificateSelector support on Win32 MONGOSH-573 feat(cli-repl): add tlsCertificateSelector support on Win32 MONGOSH-573 Mar 11, 2021
Copy link
Contributor Author

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This should be ready for review -- I've manually verified that this works with actual certificate/private key combinations from the Windows CA store.

@@ -181,7 +186,8 @@ describe('e2e TLS', () => {
);
const certUser = 'emailAddress=tester@example.com,CN=Wonderwoman,OU=DevTools Testers,O=MongoDB';

it('can connect with cert to create user', async() => {
before(async() => {
/* connect with cert to create user */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... took me half an hour to figure out why the server couldn't find the user :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorrrryyy :D

Copy link
Contributor

@rose-m rose-m left a comment

Choose a reason for hiding this comment

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

Awesome :) I just think we should update the usage / help output and the README to document our newly supported flag :D

@@ -181,7 +186,8 @@ describe('e2e TLS', () => {
);
const certUser = 'emailAddress=tester@example.com,CN=Wonderwoman,OU=DevTools Testers,O=MongoDB';

it('can connect with cert to create user', async() => {
before(async() => {
/* connect with cert to create user */
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorrrryyy :D

@@ -106,9 +106,7 @@ const DEPRECATED_ARGS_WITH_REPLACEMENT: Record<string, string> = {
const UNSUPPORTED_ARGS: Readonly<string[]> = [
'eval',
'sslFIPSMode',
'tlsFIPSMode',
'sslCertificateSelector',
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably update the help text of the CLI to include the flags and also add a comment in there to say that it's currently only supported on Windows.

@addaleax
Copy link
Contributor Author

Awesome :) I just think we should update the usage / help output and the README to document our newly supported flag :D

@rose-m I’ve updated the README/help output, but the flag was already documented – it should not have been, right?

@rose-m
Copy link
Contributor

rose-m commented Mar 15, 2021

Yeah it should have been removed by my other PR where I cleaned those up... anyway - now looks good ✅

@addaleax addaleax merged commit 31e8a45 into master Mar 15, 2021
@addaleax addaleax deleted the 573-dev branch March 15, 2021 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants