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

Jump to bottom

feat: add securityDetails.subjectAlternativeNames() #5628

Closed
wants to merge 5 commits into from
Closed

feat: add securityDetails.subjectAlternativeNames() #5628

wants to merge 5 commits into from

Conversation

mtoma
Copy link
Contributor

@mtoma mtoma commented Apr 11, 2020

Fixes issue #5625 - Get the sanList array of the SSL Certificate

mathiasbynens
mathiasbynens reviewed Apr 20, 2020
View reviewed changes
test/ignorehttpserrors.spec.js Outdated
@@ -63,6 +63,7 @@ describeFailsFirefox('ignoreHTTPSErrors', function() {
expect(securityDetails.subjectName()).toBe('puppeteer-tests');
expect(securityDetails.validFrom()).toBe(1550084863);
expect(securityDetails.validTo()).toBe(33086084863);
expect(securityDetails.sanList()).toEqual([]);
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test with an actual SAN list?

You can use https://1000-sans.badssl.com/ for the test, and check the length of the resulting array, as well as one particular element (so that the tests show the expected format of a single SAN object).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't this be included in the local test webserver certificate? Seems risky to me to have the whole test chain depend on network presence & internet presence.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, adding some SANs to the local cert would be ideal.

Copy link
Member

Choose a reason for hiding this comment

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

@mtoma Do you want to give this a go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mathiasbynens Yes, it's the plan. I'm just catastrophically busy these days. Any help or guidance with generating the local certificate wit a san list would be greatly appreciated.
Maybe someone knows how the certificate was generated in the first place? openssl command line probably?

Copy link
Member

Choose a reason for hiding this comment

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

I have no idea how the existing cert was generated, but it seems we could use something like https://github.com/FiloSottile/mkcert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. For future the command line I used to generate the certificate is:

openssl req -x509 -nodes -subj "/CN=puppeteer-tests" -newkey rsa:2048 -keyout key.pem -out cert.pem -addext "subjectAltName=DNS:www.puppeteer-tests.tld,DNS:www.puppeteer-tests-1.tld" -days 3650

mathiasbynens
mathiasbynens reviewed Apr 20, 2020
View reviewed changes
docs/api.md Outdated Show resolved Hide resolved
mathiasbynens
mathiasbynens requested changes Apr 20, 2020
View reviewed changes
Copy link
Member

@mathiasbynens mathiasbynens left a comment

Choose a reason for hiding this comment

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

Left some suggestions. I'm open to this change but would like to double-check with @whimboo and @mjzffr that this is something Firefox can expose as well.

@mathiasbynens mathiasbynens changed the title Fixes issue #5625 - Get the sanList array of the SSL Certificate feat: add securityDetails.subjectAlternativeNames() Apr 20, 2020
@mjzffr
Copy link
Contributor

mjzffr commented Apr 20, 2020

Yes, Firefox can expose this.

mathiasbynens
mathiasbynens reviewed May 12, 2020
View reviewed changes
test/ignorehttpserrors.spec.js Outdated
expect(securityDetails.validTo()).toBe(33086084863);
expect(securityDetails.validFrom()).toBe(1589283259);
expect(securityDetails.validTo()).toBe(1904643259);
expect(securityDetails.subjectAlternativeNames()).toEqual(['www.puppeteer-tests.tld', 'www.puppeteer-tests-1.tld']);
Copy link
Member

Choose a reason for hiding this comment

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

Let’s use .test as the TLD per https://tools.ietf.org/html/rfc2606#section-2.

mathiasbynens
mathiasbynens approved these changes May 12, 2020
View reviewed changes
Copy link
Member

@mathiasbynens mathiasbynens 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! One last request w.r.t. the test domain names.

@mathiasbynens
Copy link
Member

Would you mind rebasing? src/NetworkManager.js has moved here: https://github.com/puppeteer/puppeteer/blob/master/src/NetworkManager.ts

After the rebase this looks good to go. Thanks!

@OrKoN OrKoN mentioned this pull request May 18, 2020
Merged
mathiasbynens pushed a commit that referenced this pull request May 18, 2020
@mtoma @mathiasbynens
feat: add securityDetails.subjectAlternativeNames() #5628 (#5881)
a17bd89 
Co-authored-by: Michal TOMA <michaltoma2205@gmail.com>
Co-authored-by: Alex Rudenko <alexrudenko@chromium.org>

Fixes #5625. Closes #5628.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mathiasbynens mathiasbynens mathiasbynens approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mtoma @mjzffr @mathiasbynens @googlebot