-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add environment variable to set root CA for TLS verification #65
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
My thoughts
- Free CAs like LetsEncrypt are already pretty easy to configure, most people who are conscientious enough to take care of TLS would just get a regular trusted certificate.
- Go already has a built-in mechanism to override the CA store: crypto/x509#SystemCertPool:
On Unix systems other than macOS the environment variables SSL_CERT_FILE and SSL_CERT_DIR can be used to override the system default locations for the SSL certificate file and SSL certificate files directory, respectively. The latter can be a colon-separated list.
- On people who are really ultra security aware and has their own CA across services, they probably would have already added their CA to the system pool instead of trying to override it in every service they host.
Suggestions
With all being said, Go does lack built-in override mechanism on Windows and MacOS which can be addressed by your fix. So I am open to merging the custom CA logic.
I think it would be also useful to implement a certificate pinning mechanism, which boils down to two features, like the SSH known_hosts file:
- An option to "pin" the expected certificate signature.
- An option to trust the server certificate on first use and remember the signature for further use.
The code structure would be about the same except provide a custom TLS verify function.
Thanks for your comment. Here are my thoughts on this:
As for your suggestions, I also agree it's useful to allow pinning a certificate. I'll add the option later. |
That makes sense. I agree this PR is useful in these cases. I think after the above concerns are addressed I agree this can be merged. My logic was most people in this situation would just want to pin a specific certificate instead of having their own custom CA and signing their own local domains so we should make pinning work as well aside from only allowing custom CAs. (like, I never heard anyone use a CA SSH key and sign their own machines, they just compare the fingerprint and trust)
It depends on your use case, I was talking about those people who have like centralized SSL-intercepting firewalls, like a company or something. |
9382ae7
to
9a0ffa5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good on my end. @jmattheis Any comments on your end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution and the review!
This PR adds an environment variable to set custom root CA for TLS verification. This option is safer than skipping TLS verification when using a self-signed certificate.
Furthermore, I believe it's more convenient to add a field in the config file (like
TLSRootCA
) that can be created duringgotify init
. I can continue implementing it if we agree on this idea.