fix: pregenerate https certificate #6173

TrySound · 2021-12-18T13:06:30Z

Description

node-forge used inside selfsigned is a bit package https://packagephobia.com/result?p=node-forge
Sadly there is no js-only alternative (only openssl wrappers).

While testing this feature (server.https: true) I found chrome detects
the certificate is insecure though still allows to access the page with
https after proceeding warning wall.

So as there is not much value in using so big dependency to generate
insecure certificates @patak-dev got an idea to pregenerate single
certificate and publish it with vite.

This need to be tested on various machines though before releasing.

I made the certificate expire in 10 years. Let's cross our fingers this
will be enough.

du -ck dist/node

before: 12348
after: 9812

Additional context

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the Commit Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

@patak-dev

node-forge used inside selfsigned is a bit package https://packagephobia.com/result?p=node-forge Sadly there is no js-only alternative (only `openssl` wrappers). While testing this feature (server.https: true) I found chrome detects the certificate is insecure though still allows to access the page with https after proceeding warning wall. So as there is not much value in using so big dependency to generate insecure certificates @patak-dev got an idea to pregenerate single certificate and publish it with vite. This need to be tested on various machines though before releasing. I made the certificate expire in 10 years. Let's cross our fingers this will be enough. ``` du -ck dist/node ``` before: 12348 after: 9812

packages/vite/package.json

aleclarson · 2021-12-19T19:04:14Z

Looks good code-wise. I haven't tested the certificate

patak-dev · 2021-12-19T19:08:55Z

We will need help testing this one. I think the best is to merge it during the 2.8 beta period, we can revert it if there are reported issues before releasing 2.8

Shinigami92 · 2021-12-26T18:15:56Z

packages/vite/dev-cert.pem

@@ -0,0 +1,51 @@
+-----BEGIN RSA PRIVATE KEY-----


Just to clarify: is it safe to commit a private key to repo?

This is a key that will only be used during development, and to trick the browser, it isn't a secure HTTPS certificate. But what we currently do isn't secure either. I'm still not sure if this is possible, but at least with the people we have discussed so far, it looks like inlining the cert is the same as generating it locally with fixed params.

@TrySound should we add a comment in the .pem so people finding this private key in our repo would not be surprised? Looks like anything before the -----BEGIN should be ignored by parsers: https://stackoverflow.com/questions/19578812/comments-in-a-pem-file
Seems there are some parsers that have issues but it may be worth trying. Or any other idea to avoid getting security complaints once we merge this PR?

Is this really needed at all? i'd rather not put a private key into a public repo for whatever reason. Someone takes it and uses it to target developers using vite in a social engineering/phishing attack.
Even without a public private key, a centralized cert is a bad idea imho. If you want to do this, generate the whole thing on the client side and preferably throw away the key afterwards so no other certs can be created.

@dominikg the same answer I gave @patak-dev privately in discord

@dominikg aren't we doing the same already with selfsigned with static params? I was under the impression that this cert couldn't be exploited as it is just an insecure hack to get the browser to comply even if you get a warning during development.
If having a global cert is more insecure than generating one with selfsigned, we shouldn't merge this PR. Would you explain a bit how this works?

If we don't merge this one, another option to remove selfsigned would be to deprecate this insecure feature, and add a new plugin/docs to push users to create a proper certificate, and then we can remove it in 3.0, as others are proposing in other messages

userquin · 2021-12-26T19:43:44Z

you can check https-localhost repo, this library generates also the ssl/tls certificate for localhost and auto install the generated certificate on the local truststore: for windows and for macos on keychain, on linux I don't know (not tested), and so avoid warning messages on chrome/edge/safari, just some hints.

EDIT: I mean, the registration part on the OS trust store.

TrySound · 2021-12-26T21:21:04Z

https://bundlephobia.com/package/[email protected]

patak-dev · 2021-12-26T21:27:56Z

We discussed with @userquin, and he didn't mean that we should use https-localhost instead of this PR (I thought the same given the initial comment).
IIUC he thinks this PR is ok, but that we should push users to create their own secure certificates and provide a new option in Vite to let them specify the cert to use. This is not directly related to this PR where we are evaluating two insecure options. And can be considered a suggestion for future improvements.

TrySound · 2021-12-26T21:31:25Z

But you do let to specify own cert already. I use it daily. And agree we should provide some recipes

https: {
  key: fs.readFileSync('....pem'),
  cert: fs.readFileSync('....pem'),
}

TrySound · 2021-12-26T21:34:00Z

Btw this is what we do in our development (written by my teammate)
https://dev.to/istarkov/fast-and-easy-way-to-setup-web-developer-certificates-450e

patak-dev · 2021-12-26T21:40:28Z

Well, better like that, ha 🤦🏼
We should definitely add some best practices to the docs. HTTPS setup questions are one of the most common

dominikg · 2021-12-27T08:49:10Z

packages/vite/scripts/generateSelfsignedCertificate.ts

+import { generate } from 'selfsigned'
+
+/**
+ * https://github.com/webpack/webpack-dev-server/blob/master/lib/utils/createCertificate.js


this link does not work, createCertificate.js does not exist in the webpack-dev-server repo current master.

Skimming the repo it looks like they only offer configuration options for bringing your own cert, no auto- or pregenerated certs

We were talking with @userquin that it may also be better for us to do the same. Maybe in Vite 3.0 we can remove the autogenerated cert to push users to create secure certificates

dominikg · 2021-12-27T09:06:59Z

As mentioned on discord i don't think vite should be distributing pregenerated certificates as it could be a potential security issue and is extra maintenance overhead.

Instead add config options for ssl and a third party vite-plugin-local-ssl-setup could use these options to generate a proper cert on the developers machine.

something like

{
  name: 'vite-plugin-local-ssl-setup',
  config: () => {
    // check if local cert exists, generate one if not (use a node package or platform dependant scripts around openssl)
    const ssl = localSSL();
    return { server: { ssl } }
  }
}

as it is a plugin that can be installed independently of vite, the dependency won't affect users that don't need it.

Edit, more ideas:

Instead of a plugin it could also be a one-time script that modifies the users vite config
additional documentation on how to create and handle certs.

The documentation part is important i think. Proper handling of ssl certificates and related keys is something web developers need to be aware of. Vite users who lack the knowledge to do it themselves should be educated in it instead of given an insecure default handling that may lead them to believe it is ok to do that in other places too.

aleclarson · 2021-12-27T16:01:59Z

Agreed with @dominikg

I suggest we maintain a @vitejs/plugin-selfsigned-https package in the Vite monorepo.

patak-dev · 2021-12-27T16:10:26Z

Agree with both, but this is something we could do in Vite 3.0. We could deprecate now and start pushing user in the right direction though

> watching The Matrix while hacking these certificates Alternative to vitejs#6173 Since sharing same certificate for all users is not the best approach I suggest to try decompose node-forge which is used internally by selfsigned package. We can just use part of the package which already gives a good result. Eventually we will deprecate this feature but would be good to have less impact on current major too. ``` du -sk dist/node ``` before: 10760 after: 9944

> watching The Matrix while hacking these certificates Alternative to #6173 Since sharing same certificate for all users is not the best approach I suggest to try decompose node-forge which is used internally by selfsigned package. We can just use part of the package which already gives a good result. Eventually we will deprecate this feature but would be good to have less impact on current major too. ``` du -sk dist/node ``` before: 10760 after: 9944

patak-dev · 2021-12-30T23:17:46Z

Closing in favour of #6325

userquin · 2021-12-31T00:25:35Z

@patak-dev should be 6325 instead 3625?

Niputi · 2021-12-31T00:28:13Z

comment changed

TrySound force-pushed the pregenerate-cert branch from 5c727ac to acdab15 Compare December 18, 2021 13:08

patak-dev added this to the 2.8 milestone Dec 18, 2021

patak-dev added the p2-nice-to-have Not breaking anything but nice to have (priority) label Dec 18, 2021

ydcjeff reviewed Dec 18, 2021

View reviewed changes

packages/vite/package.json Outdated Show resolved Hide resolved

Fix script with ts

5f5209d

aleclarson approved these changes Dec 19, 2021

View reviewed changes

patak-dev approved these changes Dec 19, 2021

View reviewed changes

Shinigami92 reviewed Dec 26, 2021

View reviewed changes

dominikg reviewed Dec 27, 2021

View reviewed changes

patak-dev added the on hold label Dec 28, 2021

TrySound mentioned this pull request Dec 30, 2021

chore: use forge partially to generate certificates #6325

Merged

9 tasks

patak-dev closed this Dec 30, 2021

patak-dev mentioned this pull request Jun 10, 2022

Make node-forge an optional dependency #8532

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pregenerate https certificate #6173

fix: pregenerate https certificate #6173

TrySound commented Dec 18, 2021

aleclarson commented Dec 19, 2021

patak-dev commented Dec 19, 2021

Shinigami92 Dec 26, 2021

patak-dev Dec 26, 2021

patak-dev Dec 27, 2021

dominikg Dec 27, 2021

userquin Dec 27, 2021

patak-dev Dec 27, 2021

userquin commented Dec 26, 2021 •

edited

Loading

TrySound commented Dec 26, 2021

patak-dev commented Dec 26, 2021

TrySound commented Dec 26, 2021

TrySound commented Dec 26, 2021

patak-dev commented Dec 26, 2021 •

edited

Loading

dominikg Dec 27, 2021

patak-dev Dec 27, 2021

dominikg commented Dec 27, 2021 •

edited

Loading

aleclarson commented Dec 27, 2021

patak-dev commented Dec 27, 2021

patak-dev commented Dec 30, 2021 •

edited by Niputi

Loading

userquin commented Dec 31, 2021 •

edited

Loading

Niputi commented Dec 31, 2021

fix: pregenerate https certificate #6173

fix: pregenerate https certificate #6173

Conversation

TrySound commented Dec 18, 2021

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

aleclarson commented Dec 19, 2021

patak-dev commented Dec 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

userquin commented Dec 26, 2021 • edited Loading

TrySound commented Dec 26, 2021

patak-dev commented Dec 26, 2021

TrySound commented Dec 26, 2021

TrySound commented Dec 26, 2021

patak-dev commented Dec 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikg commented Dec 27, 2021 • edited Loading

aleclarson commented Dec 27, 2021

patak-dev commented Dec 27, 2021

patak-dev commented Dec 30, 2021 • edited by Niputi Loading

userquin commented Dec 31, 2021 • edited Loading

Niputi commented Dec 31, 2021

userquin commented Dec 26, 2021 •

edited

Loading

patak-dev commented Dec 26, 2021 •

edited

Loading

dominikg commented Dec 27, 2021 •

edited

Loading

patak-dev commented Dec 30, 2021 •

edited by Niputi

Loading

userquin commented Dec 31, 2021 •

edited

Loading