Add option to provide custom ssl certificates during development

[alexbrazier]

Closes: #4050 #3603 #3441 #1411

This PR adds the option to provide a custom SSL certificate and key using the SSL_CRT_FILE and SSL_KEY_FILE env vars.

The webpack dev server already accepts certificates if you provide them, so this just reads the files and passes them in.

As an extra check so there is no confusion when an error occurs, it will verify the files exist and that they are valid before passing them into webpack.

You can pass in relative paths ../cert.crt, cert.crt etc, or full paths /Users/test/cert.crt

Testing

• Tested using yarn start and yarn create-react-app my-app
• Tested with .env file
• Tested with exported envs and inline envs
• Tested without HTTPS flag - (same behaviour as before)
• Tested with HTTPS flag and without new flags - (same behaviour as before)
• Tested with invalid path (see error below)
• Tested with invalid cert and key (see error below)
• Tested with valid custom cert and verified the dev server was using it

Invalid config errors

Doesn't exist

Invalid cert file

[joggienl]

This would be a very nice addition to CRA!
Adding custom SSL is currently the only reason why I would eject my CRA app..

[joggienl]

I was wondering, why do you do this, I mean the encrypted value is not used anywhere?

[alexbrazier]

@joggienl Thanks for reviewing. The reason for this is to try and stay consistent with create react app being easy to use and debug. This check, and the one below is to validate the certs passed in. If they are invalid it logs an easy to understand error. Without this the standard error message is some random text related to the crypto package, and it isn't clear what went wrong.

[joggienl]

@alexbrazier thanks for the quick reply!
You're correct about the more easy to understand error! That is actually much appreciated :-)

You did not answer what I was trying to get to know though, why is the call to crypto.publicEncrypt made? Apologies for the unclear question!

Based on your reply I actually think the assumption I had beforehand was correct: by encrypting the Buffer with the public key (and below, the private one) you actually "validate" the certificate or key.

I do have a suggestion, and that is to put a comment to explain a bit what happens, not only from user point of view. Unless you are using the encrypted value this code actually does something else then it reads, it validates the certificate. This can easily be unclear when looking to at a later moment.

[alexbrazier]

@joggienl yeah, if the encrypt throws then it means that the key/cert was invalid. I will update the PR to add a comment as you suggest

[alexbrazier]

I've updated it with comments, and moved the https config into a separate file

[joggienl]

Nice!
Thanks @alexbrazier , nice work.

[joggienl]

Same comment as on line 52, I was wondering why do this encryption?

[Fordi]

Tested locally and it works exactly as described.

[csalvato]

Looking forward to this! :)

[alaindeurveilher]

hooo.. everything checked OK, I really hope this to be released in the next version. Great job everyone thank you! 👍

[alaindeurveilher]

...! What just happened??!! 🤔😲

[Fordi]

I assume he's replied to the wrong thread in his email?

[Fordi]

Yeah. See: #5606

It appears this person has no idea what "github" is, thinks licenses can be used to steal private data or clone phones (or something... it's not really clear), and has, I suppose, had their ex visiting some mobile cancer websites / apps. Probably pretty safe to ignore.

In like a year, this would be an SNL skit, if SNL did skits about geeky stuff.

[guylepage3]

Any luck with this? Would love to know when this is being pulled into CRA. Thx

[csalvato]

Any update here? Any info on why this isn't merged in yet?

[akrantz]

@iansu I would love to see this HTTPS certificate support merged in.

[ssolidus]

Merge this please!

[mikebeaton]

This does work as advertised and it would be great to have this officially included.

But, if I may, I've found one possible problem:

• The validateKeyAndCerts code is obviously entirely well-intentioned, but it has the side effect of just hiding useful exceptions, with additional information, which are otherwise shown anyway if the cert or key is invalid
  • In fact, I had to simply comment out the call to it in order to see what the initial issues where with my key and cert(!); and for this reason, if I may, I'd suggest that the PR would be better without it
• Also (but it may not be possible to get round this?) the user needs to convert their private key to a password-free version, since there is no way to specify the key password
  • However, maybe there is (already?) a way I don't know of to do this (e.g. with another webpack env var?)
  • Even if there is a way to do this, I would agree with anyone who said that having the password right there with the key file makes keeping the password on the key file kind of redundant anyway

EDIT: Apologies, on reflection I think the second item is a non-issue, and better as it is, but I'll stick by the first one.

[albertorestifo]

@gaearon could you have a look at this?

It really seems like a good, non-breaking feature to have in CRA. With the growing adoption of HTTPS and thanks to one-command tools like mkcert, I think it's beneficial to give the possibility to specify a custom certificate.

[mdulin2]

When will this be merged? I'd love to have this soon!

[Volune]

• The validateKeyAndCerts code is obviously entirely well-intentioned, but it has the side effect of just hiding useful exceptions, with additional information, which are otherwise shown anyway if the cert or key is invalid

  • In fact, I had to simply comment out the call to it in order to see what the initial issues where with my key and cert(!); and for this reason, if I may, I'd suggest that the PR would be better without it

Is this the last thing blocking this PR?

Would that be a good solution (adding the original error to the message):

  } catch (err) {
    throw new Error(`The certificate "${chalk.yellow(crtFile)}" is invalid.` + os.EOL + String(err));
  }

If yes, can the PR be updated, or can it be merged and we open another PR to improve the error messages?

[mikebeaton]

@Volune I made that comment, but I'm nothing to do with the project. If I was, I'd say that the PR would be best just removing validateKeyAndCerts and the call to it, because it hides errors which you would see anyway, behind other yes 'more readable' but unfortunately less useful errors! (I'm not sure if there is any real gain in wrapping the original error in another error.) Despite suggesting removing it, I'd like to say again that I can see that some work and thought went into that part of the code, and I can see why it was done. On the other hand @joggienl from the project in the code review above liked this feature. 'Ping' if I may? I don't know why this PR is languishing. Lack of available time and other higher priorities would be a guess. (But it works very well, is nicely designed straight forward code, and would be a very useful feature!)

[joggienl]

On the other hand @joggienl from the project in the code review above liked this feature.

I am not a maintainer on the project, in think everyone can create review comments on code. I saw this PR and I liked what it was about, so I took a look and made some comments.

Anyway I think your concern about not seeing the useful error details are valid enough to change this PR.

@alexbrazier can you take another look?

[stale]

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs.

[alexbrazier]

I'm still happy to make any required changes to this PR, but it would be good to hear from the team if this will be merged so I'm not wasting my time.

[mikebeaton]

Is there any chance at all the anyone from the project could at least comment on this?

It is a simple, clean pull request that adds a useful, non-breaking feature.

[joulse]

Please merge this 🙏

[getrembl]

Hi @ianschmitz and @iansu,
is it possible to merge it or have any feedback on why it is not ?

[alexbrazier]

I've updated the docs version to say 3.2.0 assuming it will make it in to that release, and merged in master, so the PR is ready to be merged.

@mrmckeb @ianschmitz @iansu is there anything else left to do, or wait for before this PR gets merged?

[Ivkaa]

Why was this not released in 3.3 version?

[jonahallibone]

Any update on this?

[alexbrazier]

I've merged master and removed the need to add a new dependency. Also bumped the version in the docs to 3.4.0 🤞

[AaronLT]

I would also be interested in this

[mikebeaton]

Apologies, I've deleted my previous post (asking for feedback from the project team) having realised that the links above from @iansu and @ianschmitz mean this is now in milestones for an upcoming release. Yay! 🎉

[andriijas]

@alexbrazier can you remove the version from the documentation? Thanks for your patience!

[alexbrazier]

@alexbrazier can you remove the version from the documentation? Thanks for your patience!

@andriijas I've removed the line about needing a certain version b029ef3 - does that look ok?

[andriijas]

Thanks for your patience everyone!

Please note that env var configuration might be changed to config file in the future.

[dbk91]

Glad to see this MR made it in! Good job @alexbrazier

@andriijas I agree about migrating to a config file as opposed to an env variable. The implementation I had made basically copied the same paradigm as how a user configures the proxy manually.

My PR was closed due to staleness and this one had more traction anyhow. Does that approach make sense as a foundation for moving it to a configuration file? Mostly just curious to see how the CRA team would approach this.

[andriijas]

@dbk91 There are multiple discussions going on for configuration and extendability. Nothing is set in stone yet unfortunatly.