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

adrum · 2023-12-20T04:27:21Z

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 · 2023-12-20T04:55:53Z

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 · 2023-12-20T04:59:08Z

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 · 2023-12-20T04:59:55Z

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 · 2023-12-20T05:24:54Z

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.

This makes me a little sad, as it doesn't really test/document the behavior of the renew command.

drbyte · 2023-12-20T06:27:04Z

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 · 2023-12-20T13:30:57Z

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 · 2023-12-20T21:31:28Z

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

adrum · 2023-12-20T21:59:46Z

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 · 2023-12-20T22:20:35Z

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 · 2023-12-20T22:22:00Z

@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 · 2023-12-21T03:42:10Z

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

For now, reviewing this.

mattstauffer · 2023-12-21T03:44:13Z

cli/app.php

-        $sites = collect(Site::secured())->map(function ($url) {
-            return ['Site' => $url];
-        });
+    $app->command('secured [--expiring] [--days=]', function (OutputInterface $output, $expiring = null, $days = 60) {


This whole refactored method is very clean and readable. Nice work!

A reasonable default is set here, consistent with the default set in other places. Fixes laravel#1464 Updates laravel#1461

A reasonable default is set here, consistent with the default set in other places. Fixes #1464 Updates #1461

added the ability to renew certs and view their expiration dates

eec8d5f

adrum added 4 commits December 20, 2023 00:07

restored original secured command

86c40dd

update secured command test

7dfc739

remove unnessary unsecure

dc24a8d

added renew test

dce796a

adrum added 4 commits December 20, 2023 00:38

moved renew command to site

cd198cb

remove date logic on renewal

a3a483e

added renew to to install command

39ee79d

update tests to match renew command

42f4f39

This makes me a little sad, as it doesn't really test/document the behavior of the renew command.

mattstauffer approved these changes Dec 21, 2023

View reviewed changes

mattstauffer merged commit 81a2b3b into laravel:master Dec 21, 2023
4 checks passed

mattstauffer added a commit that referenced this pull request Dec 21, 2023

Clean up syntax from #1461

59b19a2

adrum deleted the feature/renew-certs branch December 22, 2023 17:42

adrum mentioned this pull request Dec 22, 2023

Trust CA Certificate only #1463

Merged

drbyte added a commit to drbyte/valet that referenced this pull request Dec 27, 2023

Allow renew() to be called without parameters

759b7a7

A reasonable default is set here, consistent with the default set in other places. Fixes laravel#1464 Updates laravel#1461

drbyte mentioned this pull request Dec 27, 2023

Allow renew() to be called without parameters #1465

Merged

driesvints pushed a commit that referenced this pull request Jan 1, 2024

Allow renew() to be called without parameters (#1465)

e3e5bea

A reasonable default is set here, consistent with the default set in other places. Fixes #1464 Updates #1461

adrum mentioned this pull request Jun 27, 2024

NET::ERR_CERT_AUTHORITY_INVALID #1487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adrum commented Dec 20, 2023 •

edited

Loading

drbyte commented Dec 20, 2023

drbyte commented Dec 20, 2023

drbyte commented Dec 20, 2023

adrum commented Dec 20, 2023 •

edited

Loading

drbyte commented Dec 20, 2023

adrum commented Dec 20, 2023

drbyte commented Dec 20, 2023

adrum commented Dec 20, 2023

drbyte commented Dec 20, 2023

drbyte commented Dec 20, 2023

mattstauffer commented Dec 21, 2023

mattstauffer Dec 21, 2023

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

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

Conversation

adrum commented Dec 20, 2023 • edited Loading

drbyte commented Dec 20, 2023

drbyte commented Dec 20, 2023

drbyte commented Dec 20, 2023

adrum commented Dec 20, 2023 • edited Loading

drbyte commented Dec 20, 2023

adrum commented Dec 20, 2023

drbyte commented Dec 20, 2023

adrum commented Dec 20, 2023

drbyte commented Dec 20, 2023

drbyte commented Dec 20, 2023

mattstauffer commented Dec 21, 2023

mattstauffer Dec 21, 2023

Choose a reason for hiding this comment

adrum commented Dec 20, 2023 •

edited

Loading

adrum commented Dec 20, 2023 •

edited

Loading