search

laravel / valet

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

Require Trusting CA when securing sites #1488

Closed adrum closed 3 months ago

adrum commented 3 months ago

When trying to reproduce https://github.com/laravel/valet/issues/1487, I was able to get a Site in this state by following these steps:

  1. Fresh install of brew and Valet on macOS 14.5 (via VirtualBuddy)
  2. Install valet v4.6.0
  3. secure a site
  4. Manually remove the CA from Keychain Access.app
  5. Site should still be valid, since its own cert is trusted.
  6. Upgrade Valet to v4.7.0
  7. Run valet install
  8. This calls renew, which also calls secure on every site. Since the CA is not trusted, the user will be prompted for their password to trust the CA in the System Keychain. Instead of approving, Click "Cancel"
  9. This will yield the NET::ERR_CERT_AUTHORITY_INVALID error, since the old site cert was removed from the System Keychain.

This PR aims to prevent this state by doing the following:

  1. Prevent users from proceeding with the secure command if they cancel the Trust CA operation.
  2. Within the secure command, moves the createCa invocation above the unsecure command to prevent the old site's certificates from being removed if the Trust CA command is canceled. This prevents the site from being in an unpredictable state. If the command fails, it should leave the site alone. Otherwise, the site will be unsecured unexpectedly.

cc @driesvints

drbyte commented 3 months ago

Logic seems sound to me.

driesvints commented 3 months ago

Thanks @adrum

mattstauffer commented 3 months ago

@adrum This is a pretty edge-case-y thing. I'll likely merge it anyway because the code isn't that complex, but is this really something we expect users to have to deal with? What's a practical situation in which someone is manually deleting the CA, and then also choosing not to trust the CA?

mattstauffer commented 3 months ago

I should've said: Thanks so much for the fix!

Because it is an improvement to the flow, and doesn't overly complicate the code, I'm gonna merge it even though I don't know whether I expect this to really be a common experience. Thanks so much!

rabol commented 3 months ago

Just a small note:

when one use the 'trust' option of Valet, one is not asked to confirm the creation of certificates

trust              Add sudoers files for Brew and Valet to make Valet commands run without passwords

In my case I'm not asked for any password when securing a local site

adrum commented 3 months ago

@mattstauffer I agree with you that this is an edge case.

I recall when implementing https://github.com/laravel/valet/pull/1463 my CA was not trusted in my machine, so I'm guessing it was due to an older version of Valet when I first set up my machine that never trusted it? Or I manually removed it, but I cannot verify that. (Nor does it matter, aside from there might be others out there without a trusted CA).

At the very least, it will help ensure those without a trusted CA will at least experience an error with a description of how to recover if they skip the Keychain Access GUI prompt.

mattstauffer commented 3 months ago

@adrum Totally makes sense. Thanks again for the PR!!