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

(feature) Return CA Certificate path/data #41

Merged
merged 10 commits into from
Nov 21, 2019

Conversation

Js-Brecht
Copy link
Contributor

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

This PR changes three things:

  • Updates typescript & eslint to fix runtime errors.
  • Relax excessive security of CA certificate
    • The certificate needs to be read by browsers and other applications in order to trust certificates that are issued by it. If it is in a trust store, in the browser or in the system, the certificate can be read. It doesn't need to be encrypted/locked down on the disk.
    • Only the key really needs to be locked down
    • Being able to read the certificate without requiring a password provides a better user experience when using this new feature. Means you don't have to enter a password every single time you start your dev server.
  • Allows user to define returnCa option to receive the CA certificate path or data (as a buffer)

This doesn't introduce any breaking API changes. If nobody uses the option, they will continue to get the same data they always have.

The reason for this new feature is because node likes to throw errors when trying to fetch from a site that is using a self-signed (or private CA signed) certificate, since it maintains its own trust store. There are a couple of methods to fix this, which I will detail below, but in order to use them, the CA certificate must be readable. It just made sense to perform the read/return the path internally, since devcert already knows where everything is, instead of making the user do it.

The error that is occurring is unable to verify the first certificate . In order to fix that, you have to add the CA to node's trusted store, or completely bypass by telling node not to reject unauthorized certificates (insecure).

You can:

  • Use an environment variable
NODE_EXTRA_CA_CERTS=/path/to/ca/cert.crt node ...
  • Set the environment variable in code
process.env.NODE_EXTRA_CA_CERTS = '/path/to/ca/cert.crt';
  • Or, you can manually add it to your https global ca options
const https = require('https');
https.globalAgent.options.ca || https.globalAgent.options.ca = [];
https.globalAgent.options.ca.push(caData);

The issue with the last method is it replaces all of the predefined certs as well. Preferred method would be number 2, because node then reads and appends that cert to its trust store when it starts the server.

This is why we need to be able to retrieve the path to the CA cert, or the CA cert data itself.
How I achieve this on the receiving end is like this:

const sslData = await certificateFor('project-name', { returnCa: true });
if (sslData.ca) process.env.NODE_EXTRA_CA_CERTS = sslData.ca;
const ssl = {
   cert: sslData.cert,
   key: sslData.key,
}
const server = require('https').createServer(ssl, app);

It's destructured like that so that the ca parameter isn't passed to https, because that indicates peer authentication.

Related

Issue gatsbyjs/gatsby#14990

@Js-Brecht
Copy link
Contributor Author

@zetlen hate to bother you, but would it be possible to get a review on this PR as well? It is also blocking a PR/issue @ gatsbyjs. It is a bit more involved, but the behavior is necessary; unless, of course, you have a better alternative.

@zetlen
Copy link
Collaborator

zetlen commented Nov 21, 2019

@Js-Brecht Thanks for your patience. I'm reviewing now.

@Js-Brecht
Copy link
Contributor Author

Thanks for taking the time 🙂

Copy link
Collaborator

@zetlen zetlen 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 for your patience and for this excellent addition--it should add some flexibility for our use cases as well!

One small change request to the API. I know you want to expedite this, but I'm trying to get public API changes right so we don't have release churn.

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
Js-Brecht and others added 4 commits November 21, 2019 07:54
Separates `returnCa` into `getCaPath` and `getCaBuffer`

Co-Authored-By: James Zetlen <[email protected]>
Comment on lines +34 to +47
interface ICaBuffer {
ca: Buffer;
}
interface ICaPath {
caPath: string;
}
interface IDomainData {
key: Buffer;
cert: Buffer;
}
type IReturnCa<O extends Options> = O['getCaBuffer'] extends true ? ICaBuffer : false;
type IReturnCaPath<O extends Options> = O['getCaPath'] extends true ? ICaPath : false;
type IReturnData<O extends Options = {}> = (IDomainData) & (IReturnCa<O>) & (IReturnCaPath<O>);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zetlen Because of the additional flexibility of the API, I wrote these types that will cause the return value to be more explicit, depending on what flags you set when you call certificateFor().

Let me know if you think this is too much/too complicated. The alternative is to limit it to two different return types, but then you wind up with Partial properties:

type ReturnData = {
  key: Buffer,
  cert: Buffer
} | {
  ca?: Buffer,
  caPath?: string,
  key: Buffer,
  cert: Buffer
}

This probably wouldn't be an issue, ultimately. I just wanted it to be more clear what you're getting for what you give, and also get the benefits of strong types.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the self-documenting types!

Copy link
Collaborator

Choose a reason for hiding this comment

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

I approve of the types; there are lot of ways to type this, but if it populates autosuggest correctly, then that's all that matters. Thanks for thinking about it carefully.

@Js-Brecht Js-Brecht requested a review from zetlen November 21, 2019 21:16
@Js-Brecht
Copy link
Contributor Author

Just wanted to get those updates out, so I could get your opinion. Running tests now

@Js-Brecht
Copy link
Contributor Author

Tests are 💯

@zetlen zetlen merged commit 0d3ce16 into davewasmer:master Nov 21, 2019
@Js-Brecht Js-Brecht deleted the return-ca-cert branch November 22, 2019 20:39
alias-mac pushed a commit to alias-mac/devcert that referenced this pull request Feb 8, 2024
Bumps [configstore](https://github.com/yeoman/configstore) from 3.1.5 to 5.0.1.
- [Release notes](https://github.com/yeoman/configstore/releases)
- [Commits](sindresorhus/configstore@v3.1.5...v5.0.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

2 participants