include subjectAltName field in self-signed cert
#987
Conversation
|
Could you please fix that indentation error? The first line you added is indented with both spaces and tabs. Waiting for this so the page can stop refreshing for no reason. |
|
Now it's even worse. It's all supposed to be indented with tabs 😄. |
Codecov Report
@@ Coverage Diff @@
## master #987 +/- ##
=======================================
Coverage 73.17% 73.17%
=======================================
Files 5 5
Lines 451 451
Branches 143 143
=======================================
Hits 330 330
Misses 121 121
Continue to review full report at Codecov.
|
|
Doh .. that's what I get for trying to use github's online editor. As an aside, I'm not seeing the issue w/ Chrome disconnecting anymore, even without this change. Maybe something changed in Chrome that makes this change unnecessary. The "fix" was always a little dubious ... |
|
I've pinged a few people, that have a vested interest in the SSL Cert portion of this module, for feedback. Wanted to let you know so you didn't think your PR went unnoticed :D |
|
In general, there should be some effort to move away from creating new self-signed root CAs on developer machines. Each one of these is a liability because user-signed trust roots can be used to circumvent some important browser defenses (like public key pinning). To be clear, the certificate currently generated by webpack-dev-server is a ROOT certificate whose name happens to be "localhost". It can be used to sign other non-localhost certificates that users' machines will trust. It would be great if we could all rally around davewasmer/devcert and figure out a solution that involves a single user-signed cert authority (which a user could keep on a thumb drive or something), and then non-root certificates for things like webpack-dev-server. This way, we have one project to watch carefully, and many others that are creating less-dangerous (non-root, very short expiration time) single-purpose certs. |
lib/Server.js
Outdated
| value: "localhost" | ||
| }, | ||
| { | ||
| type: 6, // URI |
There was a problem hiding this comment.
Could we have some explanation as to which browsers look to each of these altName fields in order to function w/o warnings?
There was a problem hiding this comment.
Reason I ask: I'm not sure which browser specifically wants a URI entry type here (and is not satisfied by the DNS localhost entry above)
There was a problem hiding this comment.
I don't know of any browser that requires it; when I tested w/ Chrome, it wasn't required. I only included it based on this comment.
lib/Server.js
Outdated
| { | ||
| type: 7, // IP | ||
| ip: "127.0.0.1" | ||
| } |
There was a problem hiding this comment.
Another popular domain to use (especially when developing an app that makes use of subdomains) is http://lvh.me. Maybe we can add support for a wildcard domain *.lvh.me here?
lib/Server.js
Outdated
| type: 7, // IP | ||
| ip: "127.0.0.1" | ||
| } | ||
| ] |
There was a problem hiding this comment.
While we're making changes to this, consider adding IPv6 loopback IPs
::1 Most browsers are ok with this being a DNS entry type, but some versions of chrome want it as an IP
fe80::1
lib/Server.js
Outdated
| }, | ||
| { | ||
| type: 6, // URI | ||
| value: "localhost" |
There was a problem hiding this comment.
We also need localhost.localdomain for completeness
|
@cwalv please see comments by mike-north. those are excellent observations that should be addressed before we can merge. |
|
@mike-north if I merge this, would you be willing to fill in the blanks with a subsequent PR? I feat that the author has abandoned the PR, but I'd like to get the changes in there as they're worthwhile, but at the same time I'd like this to end up correct. I'm afraid I don't have the knowledge necessary as yet to implement the changes you'd requested to this one. |
|
I don't mind making the changes @mike-north mentions, if they're desired, but if we'd rather just use davewasmer/devcert it seems like wasted effort. Switching to use that project looks like it'd be straight forward, and having it install the root cert for you seems like it'd make it worth it (even ignoring the security benefit of just having the one project to watch). |
|
@cwalv I'd prefer to maintain ours in-house for the time being. keeping granular control over that for the time being is preferable over a dependency. others are of course free to weigh in on this. |
|
@shellscape I went ahead and made the changes @mike-north suggested. With these changes, after adding the generated cert to the trusted list I'm able to get green cert in Chrome at the following addresses: I haven't figured out a way to get Firefox to trust the cert, being self-signed, even if it's added as a trusted root authority. The only way I could get it to work is to create a csr and a non-CA cert that's signed by this root certificate (like what davewasmer/devcert does). |
|
I forgot to mention that I wasn't able to get a green cert at the ipv6 loopback (https://[::1]:3001/) in chrome, but Edge was okay with it. Interestingly, I was able to get a green cert at https://localhost:3001/ in Chrome, and since the Also, even with this change you still have to trust the cert manually. I like how davewasmer/devcert does this step for you, and also how it creates the server certs (and so works with firefox). The only real downside that I see to using that project (you already have a dependency on |
|
@cwalv yeah there's some debate about whether or not a cert should be automatically trusted. at the moment we're favoring the manual route, as it forces a user to at least be aware of what they're doing. looks like there's some conflicts there that need resolving, and then we can get this merged. we're going to be releasing 2.8.0 here soon and I'd love to get this one in. |
fixes issue with reconnect on Chrome 58+
|
@shellscape The way |
|
@cwalv ok, that doesn't sound as bad. we've got a 3.0 branch starting soon, and moving to devcert would be a good candidate for the new major version. I'll have to spend some time with it to see how the interaction is, and if we'd like to get any improvements to prompts/messages. for the meantime, and the 2.x branch, we'll merge this one in. |
fixes issue with reconnect on Chrome 58+
#941
What kind of change does this PR introduce?
#941