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: remove basicConstraints extension from certificate #2330

Merged
merged 3 commits into from Dec 18, 2019

Conversation

akx
Copy link
Contributor

@akx akx commented Nov 26, 2019

This fixes TLS on Chromium Linux (see #2313).

Based on #2313 (comment) – thanks to @gigafied for the fix!

fixes #2313

  • 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?

No tests – I did give it a spin on Linux Chromium (via Docker and Xquartz, which is why it's shown with macOS window decoration here). As discussed in #2313, we now only get ERR_CERT_AUTHORITY_INVALID, which is skippable.

Screenshot 2019-11-26 at 10 20 30

Motivation / Use-Case

Restores TLS functionality on Linux Chromium.

Breaking Changes

This should not break anything, though I don't have macOS Catalina (see #2274) to test with.

@jsf-clabot
Copy link

jsf-clabot commented Nov 26, 2019

CLA assistant check
All committers have signed the CLA.

@akx akx mentioned this pull request Nov 26, 2019
2 tasks
@codecov
Copy link

codecov bot commented Nov 26, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2330   +/-   ##
=======================================
  Coverage   93.95%   93.95%           
=======================================
  Files          34       34           
  Lines        1291     1291           
  Branches      368      368           
=======================================
  Hits         1213     1213           
  Misses         77       77           
  Partials        1        1
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 8f89c01...d8161b9. Read the comment docs.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 26, 2019

I am afraid it is can break https in other browsers, we should test this on chrome/firefox/safari, anyway thanks for PR

@akx
Copy link
Contributor Author

akx commented Nov 26, 2019

I am afraid it is can break https in other browsers, we should test this on chrome/firefox/safari, anyway thanks for PR

Sorry! I forgot to mention I did test it on Chrome on macOS too, and it works:

Screenshot 2019-11-26 at 12 35 49

Let me just fire up (heh) Firefox real quick... yep, it works 😄

Screenshot 2019-11-26 at 12 34 57

Mojave Safari is also fine with it:

Screenshot 2019-11-26 at 12 36 32

@alexander-akait
Copy link
Member

alexander-akait commented Nov 26, 2019

@akx thanks for PR, i will look on this in near future and do more tests 👍

@alexander-akait
Copy link
Member

alexander-akait commented Nov 26, 2019

@akx good test 👍

Works fine with chrome and firefox on linux

Somebody can test this on windows?

/cc @Loonride @hiroppy

@hiroppy
Copy link
Member

hiroppy commented Nov 27, 2019

@evilebottnawi do you know the reason? I have only Mac...

@alexander-akait
Copy link
Member

alexander-akait commented Nov 27, 2019

@hiroppy can you try to remove extKeyUsage (maybe we should do revert) and check it

Copy link
Member

@alexander-akait alexander-akait left a comment

To be honestly i afraid removing basicConstraints because it can break certificates for old browsers

@hiroppy
Copy link
Member

hiroppy commented Nov 28, 2019

I think it might be better to change the targeted branch from master to next.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 28, 2019

@hiroppy right now https is broken for chrome on linux, it should be not next, it is bug and we need fix it

@hiroppy
Copy link
Member

hiroppy commented Dec 4, 2019

I'm not sure this change's inpact. Anyway someone can check this change on Windows?

@alexander-akait
Copy link
Member

alexander-akait commented Dec 5, 2019

@hiroppy i think we should open issue in chrome bug tracker, because some options works on macos chrome but doesn't on linux chrome and versa vice, for me it is very strange behavior and we need more information from developers why it is happens

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.

4 participants