Skip to content

Conversation

@icyrizard
Copy link

@icyrizard icyrizard commented Dec 13, 2021

This pr is meant to overcome an issue in concerning Chrome v58 and up.

The tests I've conducted are:

Test the default setting

  1. Clean linked directories, $ valet unpark
  2. Run without expireIn should go to default. $ valet secure <my_laravel_project>
  3. See if Chrome is happy - yes it is - validity is set default to 368 days now.

Test with a too high expireIn

  1. Clean linked directories, $ valet unpark
  2. Run without secure, $ valet secure --expireIn=730 <my_laravel_project>
  3. See if Chrome is not happy, throws same error net::ERR_CERT_VALIDITY_TOO_LONG

Test with a lower expireIn

  1. Clean linked directories, $ valet unpark
  2. Run with shorter expireIn, $ valet secure --expireIn=120 <my_laravel_project>
  3. See if Chrome is happy.

Extra test is done by checking Mac's Keychain Access for default option, 368 days counting from December 13 2021
image

The expireIn option that can be passed to $ valet secure <domain> --expireIn=<nDays> can be used to overcome the aforementioned issue.

@driesvints
Copy link
Member

Hey @icyrizard, can you please refrain from linking to an issue in commit messages? It messes up the history/conversation in GitHub. See the log in #1153

@icyrizard
Copy link
Author

@driesvints Ah I see, my bad, I honestly thought that was a useful feature before you mentioned this. I'll squash the commits into one.

@icyrizard icyrizard force-pushed the issue-1153-valet-lifetime-configureable branch from 4e0ad6d to 4283dae Compare December 13, 2021 14:17
@icyrizard icyrizard changed the title Add 'expireIn=' option and use in the openssl command for #1153 Add 'expireIn=' option and use in the openssl command Dec 13, 2021
@mattstauffer mattstauffer merged commit 55c29e2 into laravel:master Dec 13, 2021
@mattstauffer
Copy link
Collaborator

Thanks @icyrizard! This looks great. Per @driesvints's suggestion, I'd also like to see if we can use some of this same code to allow people to update expired/expiring certificates per #1103

@stancl
Copy link
Contributor

stancl commented Dec 13, 2021

Not sure if this was meant to fix it or if this caused it, but I just reinstalled my local setup due to a PHP version upgrade and started getting this after I tried to secure a site:

➜ samuel@MacBook-Pro  ~/Sites/helpdesk git:(master) ✗ valet secure
Enter PEM pass phrase:

In Nginx.php line 138:
                                                                                                                                                                                                                                                                                                                                                                                      
  Nginx cannot start; please check your nginx.conf [1: nginx: [emerg] cannot load certificate "/Users/samuel/.config/valet/Certificates/helpdesk.test.crt": BIO_new_file() failed (SSL: error:02001002:system library:fopen:No such file or directory:fopen('/Users/samuel/.config/valet/Certificates/helpdesk.test.crt','r') error:2006D080:BIO routines:BIO_new_file:no such file)  
  nginx: configuration file /usr/local/etc/nginx/nginx.conf test failed                                                                                                                                                                                                                                                                                                               
  ].                                                                                                                                                                                                                                                                                                                                                                                  
                                                                                                                                                                                                                                                                                                                                                                                      

secure [--expireIn EXPIREIN] [--] [<domain>]

Was pretty confusing to debug, but after adding some dd() calls to the vendor valet files, I found that $caExpireInDays is null inside secure(), despite the parameter having a default value of 368. Which was leading to openssl complaining about the number of days missing:

bad number of days: invalid\n
usage: x509 args\n

And therefore only creating the csr request and not the actual crt certificate that the nginx file tries to include.

Passing the number of days manually using --expireIn=368 fixes everything.

My guess is that the issue is caused by this line having null as a default:

$app->command('secure [domain] [--expireIn=]', function ($domain = null, $expireIn = null) {

So if you have time right now, please test if you can reproduce this and if setting $expireIn to 368 ^ here then fixes the issue. Alternatively, you can revert this and re-merge when it's fixed in the implementation.

I wanted to open an issue pointing this out and saying that passing the --expireIn flag manually fixes this so that others don't have to go through the same painful debugging, but I saw that a related PR (this one) was merged 3 hours ago which made finding the (likely) root cause much easier.

Either way, it would be great to patch this at least somehow and tag a new release, since right now running valet secure or just updating your PHP version (seems like that triggers a valet reinstall/reconfigure) sort of bricks the entire nginx configuration.

@stancl
Copy link
Contributor

stancl commented Dec 13, 2021

Figured that I can fix this faster than someone else having to reinstall valet to try reproducing it. Linked PR changes the default value which fully fixes the openssl issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants