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(gatsby): Add self-signed cert to node trust store (https) #18703

Merged
merged 33 commits into from
Apr 17, 2020

Conversation

Js-Brecht
Copy link
Contributor

@Js-Brecht Js-Brecht commented Oct 15, 2019

Description

This PR does a few things.

  1. It upgrades devcert to v1.1.0 to fix https issues.
    • The version is currently pointing to the release branch of my devcert fork, as a proof of concept
    • If/when devcert gets patched and published, I will update the version spec to point to the new version
  2. It adds a little functionality to trust self-signed, and privately signed, certs
    • If only flags --cert-file and --key-file are specified, it assumes it's self-signed, and tells node to trust it.
    • I've included a new CLI flag, --ca-file. If the certificate is signed by a private ca, then include that ca's certificate using this flag in order for node to trust the cert/key pair
  3. It adds functionality to collect the ca certificate path from devcert during the automatic setup, and tells node to trust it.
  4. It updates the documentation to reflect the new changes/process.

Tracking related external PRs

Prerequisites

  • devcert v1.1.0

Related Issues

Fixes #16212
Fixes #14990

@Js-Brecht Js-Brecht added type: bug An issue or pull request relating to a bug in Gatsby type: upstream Issues outside of Gatsby's control, caused by dependencies topic: cli Related to the Gatsby CLI labels Oct 15, 2019
@Js-Brecht Js-Brecht requested review from a team as code owners October 15, 2019 22:53
yarn.lock Show resolved Hide resolved
packages/gatsby/package.json Outdated Show resolved Hide resolved
@Js-Brecht
Copy link
Contributor Author

@pieh The work over at devcert has been finished. I've updated this PR to use devcert@^1.1.0. Please do run some tests, if you don't mind. I haven't run into any issues in my testing, but I would like to make sure.

Several things have changed from devcert-san, to [email protected], and then to [email protected].

  • The structure of the apps data changes from devcert-san, so it will wipe out the old files and remove from trust stores, and rebuild/reinstall using the new.
  • If [email protected] was being run, the format of the certificate authority changes; it will run an update process to fix that. If it is unable to fix, it will run a process to remove the CA (from filesystem & trust stores), then reinstall everything.

I do have a couple of questions

  1. Somehow, I thought that the following lines were set up to not install certutil, when, in fact, the reverse is true. It will attempt to install the program if it isn't already. In this PR, I disabled that functionality, and updated the docs to reflect that. Since it was originally configured to install certutil, would you like me to update my changes to reflect that, or keep it the way it is now?:

report.info(`setting up automatic SSL certificate (may require sudo)\n`)
try {
return await getDevelopmentCertificate(name, {
installCertutil: true,
})

  1. Would you like me to revert the commit with the resolution for vue-template-compiler? Perhaps that should be a different PR?

@Js-Brecht
Copy link
Contributor Author

CI checks are passing. Needed to update the jest tests, and the code fences in the doc. There's one still failing, but it doesn't seem related to this PR.

@Js-Brecht Js-Brecht removed the type: upstream Issues outside of Gatsby's control, caused by dependencies label Nov 27, 2019
@Js-Brecht Js-Brecht requested a review from pieh December 3, 2019 18:32
@Js-Brecht Js-Brecht added the status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response label Dec 11, 2019
Copy link
Contributor

@DanielRuf DanielRuf left a comment

Choose a reason for hiding this comment

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

There are currently conflicts. Can you check and resolve them?

@Js-Brecht
Copy link
Contributor Author

There are currently conflicts. Can you check and resolve them?

Okay, I've merged in the commits from the master branch, and resolved the conflicts. I think that should do it.

I see develop.js has become develop.ts. Is there a migration to Typescript happening? If so, I'm all for it 😄

@Js-Brecht
Copy link
Contributor Author

@pieh would it be possible to get some action on this PR? I know you guys are probably busy, but I keep having to come back to it and resolve conflicts, which is a pain sometimes. Not to mention, there's still problems because of the current state. New issue opened this morning: #20991.

I just updated my branch, to bring it even with the master branch, and resolve all of the conflicts. I also ran through some manual tests, upgrading from the current Gatsby version to this updated version, and it all ran smoothly. I don't, however, have access to a Mac machine to test on, or my Linux machine at the moment.

/cc @wardpeet @KyleAMathews

@wardpeet wardpeet self-assigned this Jan 30, 2020
@wardpeet
Copy link
Contributor

wardpeet commented Mar 2, 2020

@Js-Brecht what would be the best way to test this PR?

@Js-Brecht
Copy link
Contributor Author

@wardpeet since devcert Is now v1, the best way to test it is to run gatsby develop with https using that version, then again with this one. We made sure that devcert could handle the upgrade gracefully

@Js-Brecht
Copy link
Contributor Author

When running the upgrade, the CA cert should get updated, both on disk and in your certificate trust store. After, it should be readable, and it should be picked up by Node

@wardpeet
Copy link
Contributor

wardpeet commented Mar 2, 2020

I've ran it and it works but I'm mostly interested in the CA-file part. How do I test that code path?

@Js-Brecht
Copy link
Contributor Author

That's for when somebody is using their own custom certificate via cert-file and key-file.

If only cert-file and key-file are defined, then it is assumed that cert-file is self-signed, and it is used as the certificate authority. If cert-file is signed by a "private" certificate authority, then just include ca-file pointing to that CA certificate.

You could use the certificates generated by devcert, if you wanted, or use a custom certificate set.
Devcert certificates should be stored at:

  • Mac
    • ~/Library/Application Support/devcert
  • Linux
    • If the variable exists: $XDG_CONFIG_HOME/devcert,
    • otherwise: ~/.config/devcert
  • Windows
    • cmd: %LOCALAPPDATA%\devcert
    • Powershell: $ENV:LOCALAPPDATA\devcert

In subdirectories under that path:

  • CA: certificate-authority/certificate.cert
  • Cert: domains/localhost/certificate.crt
  • Key: domains/localhost/private-key.key

@wardpeet wardpeet removed their assignment Mar 10, 2020
@wardpeet wardpeet requested a review from a team March 10, 2020 10:28
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Thanks for adding so much to the docs for this! I did an initial pass with some suggestions. I'd also ask that you use HTTPS with capital letters consistently, so if you could review for that as well that'd be great.

docs/docs/local-https.md Outdated Show resolved Hide resolved
docs/docs/local-https.md Outdated Show resolved Hide resolved
docs/docs/local-https.md Outdated Show resolved Hide resolved
@wardpeet wardpeet added status: awaiting author response Additional information has been requested from the author and removed status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response labels Mar 10, 2020
Co-Authored-By: Ward Peeters <[email protected]>
Js-Brecht and others added 2 commits March 10, 2020 06:13
@Js-Brecht
Copy link
Contributor Author

@laurieontech @wardpeet thanks for the comments! Left a couple of questions for clarification

I'd also ask that you use HTTPS with capital letters consistently, so if you could review for that as well that'd be great.

Done!

@laurieontech
Copy link
Contributor

Looks great from a content perspective! I pulled it down and did some re-organization to make use of headings since there are so many different forks based on OS. If you want to take a look and confirm that I didn't obscure your meaning somehow that'd be great! But this should be good to go.

@Js-Brecht
Copy link
Contributor Author

Looks great from a content perspective! I pulled it down and did some re-organization to make use of headings since there are so many different forks based on OS. If you want to take a look and confirm that I didn't obscure your meaning somehow that'd be great! But this should be good to go.

Looks good to me. Thanks for that. Just waiting to get the last review completed 🙂

I did miss one "HTTPS" somehow, though 🤔.

@Js-Brecht Js-Brecht requested a review from wardpeet March 12, 2020 19:11
@Js-Brecht Js-Brecht added status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response and removed status: awaiting author response Additional information has been requested from the author labels Mar 14, 2020
@Js-Brecht
Copy link
Contributor Author

Hey @wardpeet, did you have a moment to come back to this? I think the only thing blocking this PR is that last review, which is pretty minor, and should be a really quick fix when the questions are answered.

@wardpeet wardpeet self-assigned this Apr 16, 2020
@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Apr 17, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Thank you @Js-Brecht, Finally getting this in.

Sorry for the wait, thank you for the great write-up and bug fixing. Finally, https works great again on develop. Our users will be pleased 🙏

@Js-Brecht
Copy link
Contributor Author

@wardpeet Thanks for helping me get this over the finish line!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: awaiting reviewer response A pull request that is currently awaiting a reviewer's response topic: cli Related to the Gatsby CLI type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--https fails if the path of the project contains spaces Regression: self-signed certificates no longer work
5 participants