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

Trust CA Certificate only #1463

Merged
merged 2 commits into from
May 31, 2024
Merged

Conversation

adrum
Copy link
Contributor

@adrum adrum commented Dec 22, 2023

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

drbyte commented Dec 22, 2023

Woot! I like the simplicity of this!

@drbyte
Copy link
Contributor

drbyte commented Dec 22, 2023

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

adrum commented Dec 22, 2023

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

adrum commented Dec 22, 2023

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

drbyte commented Dec 22, 2023

thought about adding this logic to the valet trust command

Nah, that command has a different intention anyway.

@drbyte
Copy link
Contributor

drbyte commented Dec 22, 2023

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

adrum commented Dec 22, 2023

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

drbyte commented Dec 22, 2023

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

drbyte commented Dec 26, 2023

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

adrum commented Jan 3, 2024

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

drbyte commented Jan 3, 2024

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

adrum commented Jan 20, 2024

@drbyte Anything else needed from me for this one?

@drbyte
Copy link
Contributor

drbyte commented Jan 20, 2024

@drbyte Anything else needed from me for this one?

No. Thanks for that.

@drbyte
Copy link
Contributor

drbyte commented Jan 20, 2024

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

@mattstauffer
Copy link
Collaborator

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

@mattstauffer mattstauffer merged commit 2da59f9 into laravel:master May 31, 2024
5 checks passed
@adrum adrum deleted the feature/trust-ca-cert branch May 31, 2024 23:14
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.

3 participants