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

Add HTTPS/HTTP2 server #422

Merged
merged 11 commits into from Jun 5, 2020
Merged

Conversation

stramel
Copy link
Contributor

@stramel stramel commented Jun 4, 2020

Fixes #399

This will generate an HTTP2 server when using the --secure flag.

It will check the directory for a snowpack.cert and snowpack.key file and if not found will generate a local set to use.

Also, swapped the ETag over to use the weak option since we're using compression now. Missed in my previous PR #419

@FredKSchott I'm a bit unsure of how to get the files to output to the built files and wasn't sure where we wanted to look for/generate files. I was also not entirely sure the best way to resolve paths to files in Node. Those are the main things left to make this work.

A lot of this work is utilizing the work from servor.

NOTE: On Chrome, once I switched to the HTTP2 server, it seems like it was randomly not sending the if-none-match header. However, Firefox, continued to work fine.

@stramel stramel requested a review from FredKSchott June 4, 2020 19:33
@stramel stramel self-assigned this Jun 4, 2020
Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

Looks great! I bet this has another noticeable perf impact for large apps

src/commands/dev.ts Show resolved Hide resolved
src/commands/certify.sh Outdated Show resolved Hide resolved
src/commands/dev.ts Outdated Show resolved Hide resolved
src/commands/dev.ts Outdated Show resolved Hide resolved
src/commands/dev.ts Outdated Show resolved Hide resolved
src/commands/utils/ca.conf Outdated Show resolved Hide resolved
@stramel stramel marked this pull request as ready for review June 5, 2020 16:07
@stramel
Copy link
Contributor Author

stramel commented Jun 5, 2020

I modified the code slightly. Running --secure without key/cert,

  1. Check the cwd for snowpack.crt and snowpack.key
  2. If not found, generate key/cert in the node_modules/snowpack/assets folder as to not dirty the user's repo.
  3. Check the node_modules/snowpack/assets for snowpack.crt and snowpack.key

I think this provides a bit more seamless solution. If we want to make the user more aware of the generated key/cert, I think we should like you said, require the user's consent before generating and generate into their cwd.

Personally, I prefer the more seamless approach but either is fine with me.

@stramel
Copy link
Contributor Author

stramel commented Jun 5, 2020

One concern that I have using the certify.sh file is that it is only tested on Linux and Mac OSX. I was able to run it on my Windows machine when utilizing the Linux sub-system, although, I'm unsure if it would work for users that are running standard cmd prompt.

Also, I didn't add the section of changing the process id for installing the rootCA automatically in the keychain. This felt a bit awkward so instead, I left the snowpackCA.crt in the assets dir for manual installation if a user wishes to do so.

For better cross-platform usage, we may want to look into using devcert instead.

@FredKSchott
Copy link
Owner

@stramel If we're worried about the stability of this (especially the fact that it can't run on Windows) lets pull that logic out for now and just give explicit commands that the user can run if they need to generate. IE:

$ snowpack dev --secure
! No HTTPS credentials found! Missing Files: snowpack.crt, snowpack.key 

You can automatically generate credentials for your project via either:
  - devcert (no install needed): npx devcert-cli generate localhost
    https://github.com/davewasmer/devcert-cli (no install required)
  - mkcert: mkcert -install && mkcert localhost
    https://github.com/FiloSottile/mkcert (install required)

# exit

I think this may even be the best long-term solution as well. I mentioned elsewhere, but based on what I've seen I don't think we have the resources to maintain a custom integration that handles automatic generation & connecting that to your internal key store all perfectly within Snowpack.

@stramel
Copy link
Contributor Author

stramel commented Jun 5, 2020

@FredKSchott I did a bit of formatting on the error message. Feel free to clean it up however you see fit.

image

Also, I opened an issue on devcert-cli to be able to specify the output file names. davewasmer/devcert-cli#5 This way we can just have a single command and be up and running for snowpack.

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM!

@FredKSchott
Copy link
Owner

Thanks for tackling, this is great!

Assuming you've been able to test this running locally, feel free to merge when ready

@stramel stramel changed the title Add HTTP2 server Add HTTPS/HTTP2 server Jun 5, 2020
@stramel stramel merged commit 3266d9b into FredKSchott:master Jun 5, 2020
@stramel stramel deleted the ms/enable-https-server branch June 5, 2020 20:17
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.

HTTPS/HTTP2 Dev Server Mode
2 participants