-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add TLS and hostname support #49
Conversation
This adds both TLS and hostname support. TLS support is activated by specifying paths to a key and a cert. There are caveats about self-signed certs in the README. This also adds hostname support. This impacts both the URL sent to browsers and the IP the server socket listens to. There are caveats about valid names, valid IPs, and HSTS preload in the README. HSTS-type errors in both Chrome and Firefox are detected automatically and translated into friendlier errors with a short link to the documentation. Closes #42 Replaces PR #43 One part of solving shaka-project/shaka-player#5547
Checking the Windows test failure against our Windows lab machine. |
I get a different failure on our Windows machine:
On CircleCI, we get:
And similar on several tests. I think CircleCI's Windows box only has a localhost interface, and The failure I get on my own Windows box seems to be an issue with my |
Forcing localhost to IPv6 in /etc/hosts on Linux reproduces the failures I get in CircleCI on Windows:
|
Ah, the problem with IPv6 for localhost is my defaults for hostname. Using "0.0.0.0" for the listening address doesn't mean "all IPs from all interfaces", it means "all IPs from IPv4 interfaces only". Easy to fix. |
Tests passing on all platforms, with IPv4 and IPv6. Please take a look! |
This will simplify further changes I am planning for later
Thanks for the PR. This looks solid at first glance. I'll review it in more detail soon. |
Thanks! |
const ip = getIP(); | ||
|
||
if (ip == '127.0.0.1') { | ||
pending('Cannot test hostname without a non-localhost interface.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunate, but I don't see a realistic alternative.
// all IPs (no hostname) and point browsers to "localhost". We preserve | ||
// backward compatibility here by using different defaults for these two | ||
// things. | ||
const listenOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I think it'd be better to listen only on loopback by default but that's a change for the next major release.
This adds both TLS and hostname support.
TLS support is activated by specifying paths to a key and a cert. There are caveats about self-signed certs in the README.
This also adds hostname support. This impacts both the URL sent to browsers and the IP the server socket listens to. There are caveats about valid names, valid IPs, and HSTS preload in the README. HSTS-type errors in both Chrome and Firefox are detected automatically and translated into friendlier errors with a short link to the documentation.
Closes #42
Replaces PR #43
One part of solving shaka-project/shaka-player#5547