search

laravel / valet

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

Trust CA Certificate only #1463

Closed adrum closed 1 month ago

adrum commented 6 months ago

As a follow-up to #1461, this PR changes the certificate trust logic only to need to trust the LaravelValetCASelfSigned.pem certificate. Since all site certificates are generated from this CA, trusting the CA makes trusting the site certificate redundant.

I noticed my LaravelValetCASelfSigned.pem was not trusted on my system, so I added a check for that anytime secure is called to ensure the root CA is trusted.

This will make sure the sudo call to add an entry to the system keychain is called at most once when calling the renew command. Once the CA is trusted, there will be no need to authenticate when calling secure or renew commands.

I left the unsecure command alone since it won't hurt to clean up old certificates if needed.

drbyte commented 6 months ago

Woot! I like the simplicity of this!

drbyte commented 6 months ago

What are the chances that someone might need to explicitly trust a certificate managed by Valet? Should we offer an optional parameter to trigger that? (ie: backward compatibility)

Or am I overthinking? 😂

adrum commented 6 months ago

I actually thought about adding this logic to the valet trust command, but ultimately decided calling it whenever secure is called seemed like the most natural flow.

For anyone with existing installs, their certs will be already trusted. The next time they call the secure or renew command, their CA will now be trusted if it's not. The new generated cert won't be explicitly trusted in the keychain, but it would be trusted via the CA.

adrum commented 6 months ago

I re-read your comment, and I don't think it would be necessary since the CA is trusted. We could pass a --trust option to the secure command, but I don't think it would be necessary.

drbyte commented 6 months ago

thought about adding this logic to the valet trust command

Nah, that command has a different intention anyway.

drbyte commented 6 months ago

I don't think it would be necessary since the CA is trusted. We could pass a --trust option to the secure command, but I don't think it would be necessary.

Agreed. I'll give it a test later today myself.

adrum commented 6 months ago

thought about adding this logic to the valet trust command

Nah, that command has a different intention anyway.

Yeah, that's why I decided it didn't make any sense.

Let me know how your testing goes! I eventually removed all my certs from the keychain to validate, but feel free to try both existing and fresh installs. It seemed to work just fine for me, including subdomains.

drbyte commented 6 months ago

Yup. Looks good to me. I tested with a handful of existing secured sites, and new. And proxy. It's super-nice to no longer have to re-authenticate for every secured domain.

/cc @mattstauffer

drbyte commented 6 months ago

I've been using this update for several days. Works fine.

Firefox did (as expected, as it always has) initially flag a warning because Valet's CA is self-signed, but accepting that certificate was a one-time prompt. No issues since.

adrum commented 6 months ago

@drbyte I don't use Firefox, but I was curious enough to try to understand why it doesn't use the System Keychain. Upon my first attempt at reproducing the FF issue, it worked for me the first time without accepting anything.

Do you know what setting your about:config > security.enterprise_roots.enabled is set to? Mine was true, which means it should import CAs from the OS. I also wonder if Firefox needs restarted anytime new CAs are added to the System Keychain.

https://support.mozilla.org/en-US/kb/setting-certificate-authorities-firefox

drbyte commented 6 months ago

(My default browser is also not Firefox) My Firefox is quite "vanilla", and I have profiles that I often "reset to defaults" for testing things. The default for my Firefox for that about:config > security.enterprise_roots.enabled is false.

Screen Shot 2024-01-02 at 10 45 26 PM

But, setting it to true does indeed avoid Firefox's certificate warning. 🙌

adrum commented 5 months ago

@drbyte Anything else needed from me for this one?

drbyte commented 5 months ago

@drbyte Anything else needed from me for this one?

No. Thanks for that.

drbyte commented 5 months ago

I'm satisfied that this is ready for merge and release. /cc @mattstauffer

mattstauffer commented 1 month ago

Apologies for the delays y'all. We've been talking about this change for a while; it's great to see it working!