fix: Remove protection on CA file after upgrade #48
Conversation
|
Oh yeah, I forgot that happens 🤦♂ |
src/certificate-authority.ts
Outdated
|
|
||
| function certErrors(): string { | ||
| try { | ||
| openssl(`x509 -in ${ rootCACertPath } -noout`); |
There was a problem hiding this comment.
I think quotes might be a good idea here. Might generate ENOENT type errors (or whatever openssl throws) if there are spaces in the path.
src/platforms/linux.ts
Outdated
| @@ -75,6 +75,10 @@ export default class LinuxPlatform implements Platform { | |||
| } | |||
| } | |||
|
|
|||
| async deleteProtectedFile(filepath: string) { | |||
| await run(`sudo rm ${filepath}"`); | |||
There was a problem hiding this comment.
I think a double quote was missed here
There was a problem hiding this comment.
I super don't know how that happened. It passed all tests--I must have typoed it before commit.
| const remainingErrors = certErrors(); | ||
| if (remainingErrors) { | ||
| return installCertificateAuthority(options); | ||
| } |
There was a problem hiding this comment.
I may be misreading this
I did misread this, sorry.
Irrelevant comments
Should this condition be this?
- if (remainingErrors) {
+ if (!remainingErrors) {
return installCertificateAuthority(options);
}from line 119
return '' // Success returns a falsy value, yeah?There was a problem hiding this comment.
Nevermind, I know why you're doing this now. Just kinda popped into my head 😆 Need to recreate the certificate authority if it is invalid.
So, now my question becomes: shouldn't the old domain certificates, the old CA files, and the old CA from the trust stores be removed? If creating a new CA, then all of that stuff will be invalid.
I know that the installCertificateAuthority routine checks for older versions.... maybe a new function that can be run in various circumstances when it's desirable to wipe out the current devcert CA state?
There was a problem hiding this comment.
I know it would be easier to read if certErrors(): string was like certIsValid(): boolean, but I did it this way to lay the groundwork for more sophisticated error reporting in the future.
As for removing the old certificates, you're right--and some of those will also be protected. Time to do even more scrubbing!
There was a problem hiding this comment.
That was all me. I don't think it's necessarily hard to read; I just had it in my head for some reason that the next reasonable step to take upon successful conversion was to run the install routine. Somehow I forgot that the certificate would have already been installed previously if it got to this point 🤷♂. Doubting whether I was reading it right was my subconscious telling me I absolutely was not, I think. 😄
I did it this way to lay the groundwork for more sophisticated error reporting in the future.
Out of curiosity, how would you feel about extending this cert checking & scrubbing process to check for an expired CA + renewing it (and its trusts/signed certs)? I could write up a PR when I have some more free time.
There was a problem hiding this comment.
I think that sounds great, @Js-Brecht . But yeah, it can be a subsequent PR.
Because of the breaking change in #41, I've had to revamp the installation/uninstallation process, so I've just updated this PR a fair bit. Please take another look!
There was a problem hiding this comment.
👍 I like it. It works well, tests ran with no problems, and I didn't see anything out of place. This is something that's been missing for some time 👏
I just have one (non-blocking) question about being more verbal when rebuilding the CA. It's not a show-stopper, and I doubt it would throw people off too much, really. Just a personal preference 🤷♂
src/certificate-authority.ts
Outdated
| try { | ||
| const caFileContents = await currentPlatform.readProtectedFile(rootCACertPath); | ||
| currentPlatform.deleteProtectedFiles(rootCACertPath); | ||
| writeFile(rootCACertPath, caFileContents); | ||
| } catch (e) { | ||
| return installCertificateAuthority(options); | ||
| } | ||
|
|
||
| // double check that we have a live one | ||
| const remainingErrors = certErrors(); | ||
| if (remainingErrors) { | ||
| return installCertificateAuthority(options); | ||
| } | ||
| } |
There was a problem hiding this comment.
I like this; it works well. If the CA has to be rebuilt, would it be possible to drop a message in the console? The reason I ask is because I get popups in Windows when it wants to remove the trust. It's not a big deal, if you know what's happening, and why; might throw some people off a bit when it pops up asking them to delete stuff as they're trying to run a development server, is all.
There was a problem hiding this comment.
I added a console warning for this! Please do open a followup PR if the wording isn't great. I'm really more of a good speller than a good writer.
Bumps [@types/qunit](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/qunit) from 2.9.6 to 2.11.1. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/qunit) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Since #41 merged, the master branch bombs out when trying to use the new
getCaPathandgetCaBufferoptions on a CA installed by a previous version of devcert, because they are unreadable files! This addition makes them readable after upgrade.@Js-Brecht if you wanna take a look at this, I'd welcome it, but if you're busy, I'll regress and merge it myself so I can release 1.1.0 for you tomorrow.