Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require Trusting CA when securing sites #1488

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

adrum
Copy link
Contributor

@adrum adrum commented Jun 21, 2024

When trying to reproduce #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
Copy link
Contributor

drbyte commented Jun 21, 2024

Logic seems sound to me.

@driesvints driesvints requested a review from mattstauffer June 24, 2024 10:03
@driesvints
Copy link
Member

Thanks @adrum

@mattstauffer
Copy link
Collaborator

@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 mattstauffer merged commit 5d4821b into laravel:master Jun 24, 2024
4 checks passed
@mattstauffer
Copy link
Collaborator

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
Copy link

rabol commented Jun 25, 2024

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
Copy link
Contributor Author

adrum commented Jun 27, 2024

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

I recall when implementing #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.

@adrum adrum deleted the fix/trust-ca branch June 27, 2024 23:35
@mattstauffer
Copy link
Collaborator

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

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.

5 participants