-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
chore: use forge partially to generate certificates #6325
Conversation
> 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
LGTM, @dominikg @userquin @aleclarson what do you think? |
} | ||
|
||
export function createCertificate(): string { | ||
const days = 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add more days, the limit is too short, I configure the ssl/tls certificates with 2 years
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we have with existing logic. I think it's ok to keep as is if we are gonna deprecate and remove this feature eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TrySound can you provide a certificate generated with the code here?, my email is userquin@gmail.com
const attrs = [ | ||
{ | ||
name: 'commonName', | ||
value: 'example.org' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the CN should match the subject alternative name, browser will complain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should do another PR afterwards to improve these things? Lets review only the change away from selfsigned in this one and you can send the next one on top of it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is selfsigned default. Do we need to change anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is selfsigned default. Do we need to change anything?
I know this will be deprecated but if included add something functional, 30 days is too short, and from the logic perspective the work to be done to include 2 years doesn't matter (we are not changing the key lengths from 2048 to 4096 bits). The SSL/TLS certificates on browsers are quite sensitive...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@userquin could you check the PR against the current implementation? I think we have the same values. Lets try to merge this one and I think we can better discuss changes to the params in a future PR (hopefully from you :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patak-dev ok, no problem, I'm creating the p12 to import on windows and preparing some test..
I need to review a few things for browser support: I think we need to create the ssl/tls certificate for localhost only... |
@patak-dev @TrySound working (screenshots on windows + Edge, also working on windows + Chrome), adding the public cert to the trusted ca certificates on windows |
This is great @userquin, so no further changes are required? |
Yes, no changes required |
Description
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.
before: 10760
after: 9944
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).