laravel / valet

A more enjoyable local development experience for Mac.
https://laravel.com/docs/valet
MIT License
2.51k stars 695 forks source link

added the ability to renew certs and view their expiration dates #1461

Closed adrum closed 9 months ago

adrum commented 9 months ago

I was helping a teammate today resolve an issue with a certificate issue locally. After discovering the cert had expired, I thought it might be nice to expose certificate expiration dates when listing secured sites. It would also be nice to be able to renew those certs.

This PR adds both of those features.

drbyte commented 9 months ago

I like the idea.

And I follow your thinking in terms of the 60-days and 368-days.

That said, I suspect most devs don't care what the expiration date is on local certs, they just want them to never expire. So, if any are expired, they probably just want to renew everything for another year. (as opposed to the default of only renewing those expiring in next 2 months).

Thus, I'm wondering what your thoughts are on defaulting the renew() to just refresh everything?

drbyte commented 9 months ago

Would it also make sense to call renew() during valet install (which is recommended to run after upgrading), so that any previously-existing certs are refreshed, especially those that expired on an old install that's been restored, etc?

drbyte commented 9 months ago

In the past I've simply run valet tld test to re-issue certs on all secured domains. But renew is certainly more intuitive, so thanks!

adrum commented 9 months ago

I like the idea.

And I follow your thinking in terms of the 60-days and 368-days.

That said, I suspect most devs don't care what the expiration date is on local certs, they just want them to never expire. So, if any are expired, they probably just want to renew everything for another year. (as opposed to the default of only renewing those expiring in next 2 months).

Thus, I'm wondering what your thoughts are on defaulting the renew() to just refresh everything?

So I just made this change, and I had to authenticate (TouchID, thankfully) each site's new certificate. I can see how this would be annoying to do every time. However, this would likely only be run once a year.

Although, I did enjoy the simplicity of removing the days option on the renew command.

drbyte commented 9 months ago

Ya, I haven't found a way to "batch-authenticate" (jump into sudo once for the whole group) from the CLI yet, since MacOS 11.

adrum commented 9 months ago

I've not gone looking for the answer myself, but it feels like we shouldn't have to trust the individual cert if the CA is trusted. I'm fairly certain we could skip trusting the cert, but I would have to test and check.

Do you know the answer off the top of your head?

drbyte commented 9 months ago

I believe what's triggering the password prompts is the registering of the certs into the keychain. Not sure that can be skipped.

adrum commented 9 months ago

Yeah, that is what is triggering it. I think as long as the CA is trusted, that's unnecessary. I'll try to spin up a clean environment this weekend and test it.

If I confirm it's optional, I will come up with a way to make it skippable via a parameter so we can only skip it when it running renew. That way, existing behavior is unchanged.

What do you think?

drbyte commented 9 months ago

If it's optional, then let's bypass it by default ... cuz it's really unfriendly to be prompted a dozen times unexpectedly.

I'm noting that there are several places that handle certs: valet secure, tld, renew and I think proxy secured. Maybe the valet config.json file is the spot where someone could set a flag to always register_site_certs_in_keychain if they require it for a certain reason? I wonder if this "optional" bit warrants a separate PR? Granted, it'll clash with this one. Kinda depends what you come up with.

drbyte commented 9 months ago

@mattstauffer I think this PR is ready for review and merge if you're okay with it.
This latter discussion of trying to skip the password prompts, if possible, is canonically separate, so can be tackled separately.

mattstauffer commented 9 months ago

Yah, I'd love to look at that separate PR about how sites are registered. Looking forward to that.

For now, reviewing this.