-
Notifications
You must be signed in to change notification settings - Fork 42
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
remove duplicated code and simplify CA certs loading #82
base: main
Are you sure you want to change the base?
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.
Is it okay if this only loads certs under specific a folder but not the system default directory ? if yes, LGTM
it will load system and the SSL_CERT_DIR @vadmeste - we are relying on the Go's internal behavior to load System certs. Go stdlib loads certs via Systemload() from standard list of directories + SSL_CERT_DIR environment value. |
https://pkg.go.dev/crypto/x509 It doesn't seem it loads the default certs, what are you trying to do in the first place? |
https://cs.opensource.google/go/go/+/refs/tags/go1.20.7:src/crypto/x509/root_unix.go;l=41 That documentation is wrong and incomplete from how the code. The intention is to add new folders into system cert list via the environment instead of writing the entire code on our end. |
func loadSystemRoots() (*x509.CertPool, error) { | ||
// Add additional ENV to load k8s CA certs. | ||
os.Setenv("SSL_CERT_DIR", strings.Join(certDirectories, ":")) |
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.
Maybe you can append instead of overwriting this env variable. It is good to have this env variable configurable by the user.
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.
Yeah can do that @vadmeste
No description provided.