Fix dev certs for mac #245
Conversation
| return true; | ||
| // Ceritificate exists. Now check and see if it's expired and remove it if is. | ||
| try { | ||
| const command = `security find-certificate -c '${defaults.certificateName}' -p | openssl x509 -checkend 86400 -noout`; |
There was a problem hiding this comment.
can you move this try/catch into a function isCaCertificateExpired() which returns a boolean, and then have the code here be:
// on Mac, remove certificate if expired
if (process.platform === darwin) {
if (isCaCertificateExpired()) {
uninstallExpiredCertificate();
}
}
There was a problem hiding this comment.
My biggest concern with this is that the isCaCertificateInstalled() function is adding a side-effect which removes a cert. That really shouldn't be done inside this function as a side-effect. Is there a way to move this out of this function and call it for mac from another place?
| } | ||
| } | ||
| } | ||
|
|
||
| // Currently this method is only intended to be used for Mac due to problems on Mac that occur when CA certificates expire | ||
| export async function uninstallExpiredCaCertificate() { |
There was a problem hiding this comment.
actually I don't think you need this function at all. The uninstall command is the same and the message doesn't need to say whether it is expired. can't you just use the regular uninstallCaCertificate() function?
There was a problem hiding this comment.
I can't use the regular uninstallCaCertificate because it calls isCaCertificateInstalled, which will then get us in an endless loop.
There was a problem hiding this comment.
That's why I initially had uninstallCaCertificate take a Boolean expiredCert param so we could bypass the call to isCaCertificateInstalled for the removing of an expired cert
There was a problem hiding this comment.
Would it help to add an optional param "expiredOnly = false" to uninstallCaCertificate() which could be called with expiredOnly as true for Mac?
|
What's new about this pr? This issue is annoyed in Mac. |
|
Did a different fix in more up to date repo. |
With these changes, on Mac we will check to see if the dev-cert exists first. If it does, we will subsequently see if it's expired and if that's the case uninstall the certs.
I did local testing and this seems to work well, but it might be good to get some other eyes on it