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(server): Add extKeyUsage to self-signed cert #2274

Merged
merged 3 commits into from
Oct 15, 2019
Merged

fix(server): Add extKeyUsage to self-signed cert #2274

merged 3 commits into from
Oct 15, 2019

Conversation

tarrall
Copy link
Contributor

@tarrall tarrall commented Oct 9, 2019

For issue #2273

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Sorry, I did not. I am not sure if the codebase has tests to ensure that the generated cert works on MacOS?

I did manually test to confirm the new cert works on both MacOS 10.14 and MacOS 10.15; it should not impact users on other platforms.

Motivation / Use-Case

Self-signed cert does not work on MacOS 10.15 Catalina.

Breaking Changes

None.

Additional Info

MacOS 10.15 Catalina has additional requirements for self-signed
certs: https://support.apple.com/en-us/HT210176

Chrome (and, I assume, Safari) will not let you clickthrough the
"invalid cert" warning (error is NET::ERR_CERT_INVALID) if the
ExtendedKeyUsage extension is not present with at least the
id-kp-serverAuth OID.

Firefox does not seem to be affected; I think they may have their
own SSL stack.

MacOS 10.15 Catalina has additional requirements for self-signed
certs: https://support.apple.com/en-us/HT210176

Chrome (and, I assume, Safari) will not let you clickthrough the
"invalid cert" warning (error is NET::ERR_CERT_INVALID) if the
ExtendedKeyUsage extension is not present with at least the
id-kp-serverAuth OID.
@jsf-clabot
Copy link

jsf-clabot commented Oct 9, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

@codecov
Copy link

codecov bot commented Oct 9, 2019

Codecov Report

Merging #2274 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2274   +/-   ##
=======================================
  Coverage   93.92%   93.92%           
=======================================
  Files          34       34           
  Lines        1284     1284           
  Branches      369      369           
=======================================
  Hits         1206     1206           
  Misses         71       71           
  Partials        7        7
Impacted Files Coverage Δ
lib/utils/createCertificate.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8af2d9...5768d72. Read the comment docs.

@alexander-akait
Copy link
Member

@tarrall hm, so with this PR, it is allow to clickthrough the
"invalid cert" warning, right? Can you provide screen shots before and after? Anyway thanks for the PR

@tarrall
Copy link
Contributor Author

tarrall commented Oct 9, 2019

That is correct; apparently clickthrough not allowed with NET::ERR_CERT_INVALID but is allowed with NET::ERR_CERT_AUTHORITY_INVALID.

I should be able to get some screenshots tomorrow AM (USA time).

@alexander-akait
Copy link
Member

alexander-akait commented Oct 9, 2019

@tarrall Do you have NET::ERR_CERT_INVALID before? Maybe we should report to repo which we use for generating certs

@tarrall
Copy link
Contributor Author

tarrall commented Oct 9, 2019

Before and after screenshots attached -- courtesy of a teammate.

Re reporting upstream... I considered it, but I think the upstream modules are functioning as intended. You're using the selfsigned module, which in turn uses forge. They're just doing what the call tells them to do; since webpack-dev-server doesn't ask for the extended key usage extensions (before this PR) it's correct for them to not add those.

The cert isn't outright a "bad cert"; it just doesn't include all the stuff that MacOS 10.15 demands of a cert used for a website.

The forge example includes these extensions. (I skipped "emailProtection" in this PR because I can't think of any case where the self-signed cert we're creating would be used for that purpose.)

Before:

before_screenshot

After:

after_screenshot

@alexander-akait
Copy link
Member

@tarrall thanks for feedback, we can merge this PR for better development experience, but we should ivnestigate why cert doesn't work on MacOS 10.15 Catalina. Maybe you can do it and help us?

@tarrall
Copy link
Contributor Author

tarrall commented Oct 11, 2019

MacOS 10.15 Catalina has additional requirements for self-signed
certs. Please see: https://support.apple.com/en-us/HT210176 for documentation.

While Apple does not say in that doc exactly why they did it, I think "improved security" is the general goal; they are ensuring that only certificates which explicitly say they're intended to be used for server authentication.

Per https://tools.ietf.org/html/rfc5280#page-44 clients are allowed to require specific extensions:

[...] Certificate using
applications MAY require that the extended key usage extension be
present and that a particular purpose be indicated in order for the
certificate to be acceptable to that application.

Since the certificate you are generating is for server authentication, you should always include that extension.

@alexander-akait
Copy link
Member

/cc @hiroppy @Loonride

@andremescaline
Copy link

@evilebottnawi what about merging this hotfix? we cant run development with https on updated MACos ;(

@tarrall thanks for feedback, we can merge this PR for better development experience, but we should ivnestigate why cert doesn't work on MacOS 10.15 Catalina. Maybe you can do it and help us?

@olivandesign
Copy link

Same here. Really looking forward for this hotfix

@evilebottnawi what about merging this hotfix? we cant run development with https on updated MACos ;(

@tarrall thanks for feedback, we can merge this PR for better development experience, but we should ivnestigate why cert doesn't work on MacOS 10.15 Catalina. Maybe you can do it and help us?

@alexander-akait
Copy link
Member

@tarrall can you fix lint problem (need run prettier on file)

@tarrall tarrall changed the title Add extKeyUsage to auto-generated self-signed cert fix(server): Add extKeyUsage to self-signed cert Oct 14, 2019
@tarrall
Copy link
Contributor Author

tarrall commented Oct 14, 2019

OK I think the one failing lint that's left is from the commit message... in theory should pass if run again? Not positive, not sure how to test that locally and can't find anything in the repo that defines the required spec for commit messages. Please feel free to reword that commit message if necessary!

@alexander-akait
Copy link
Member

@tarrall don't worry we fix it before merge

@alexander-akait alexander-akait merged commit a4dbc3b into webpack:master Oct 15, 2019
@doc-l
Copy link

doc-l commented Oct 18, 2019

In the meantime I found out you can visit the local webpage in safari but not in chrome. So until this fix is released you can at least get the dev environment running in safari.

@k1sul1
Copy link

k1sul1 commented Nov 12, 2019

Yeah let me just fire up Safari on my linux laptop...

Thanks for fucking all chromium based browsers. Luckily 3.8.2 still works.

@alexander-akait
Copy link
Member

@k1sul1 what is problem?

@k1sul1
Copy link

k1sul1 commented Nov 12, 2019

#2313

@akx akx mentioned this pull request Nov 12, 2019
2 tasks
@akx
Copy link
Contributor

akx commented Nov 12, 2019

This does break Chrome on Linux, unfortunately. See my discussion and investigation in #2313 (comment).

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.

10 participants