valet icon indicating copy to clipboard operation
valet copied to clipboard

Trust CA Certificate only

Open adrum opened this issue 2 years ago • 14 comments

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.

adrum avatar Dec 22 '23 17:12 adrum

Woot! I like the simplicity of this!

drbyte avatar Dec 22 '23 18:12 drbyte

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? 😂

drbyte avatar Dec 22 '23 18:12 drbyte

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 avatar Dec 22 '23 19:12 adrum

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.

adrum avatar Dec 22 '23 19:12 adrum

thought about adding this logic to the valet trust command

Nah, that command has a different intention anyway.

drbyte avatar Dec 22 '23 19:12 drbyte

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.

drbyte avatar Dec 22 '23 19:12 drbyte

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.

adrum avatar Dec 22 '23 19:12 adrum

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 avatar Dec 22 '23 22:12 drbyte

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.

drbyte avatar Dec 26 '23 18:12 drbyte

@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

adrum avatar Jan 03 '24 01:01 adrum

(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. 🙌

drbyte avatar Jan 03 '24 03:01 drbyte

@drbyte Anything else needed from me for this one?

adrum avatar Jan 20 '24 20:01 adrum

@drbyte Anything else needed from me for this one?

No. Thanks for that.

drbyte avatar Jan 20 '24 22:01 drbyte

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

drbyte avatar Jan 20 '24 22:01 drbyte

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

mattstauffer avatar May 31 '24 22:05 mattstauffer