-
Notifications
You must be signed in to change notification settings - Fork 583
Forward SSL env variables to containerization #1021
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
base: main
Are you sure you want to change the base?
Conversation
80bb417 to
084eb22
Compare
|
@sebimarkgraf If there isn't one, could you make an issue for this describing with examples what this enhances or fixes? Also, please rebase onto the latest main as
|
|
I will rebase and update today. This fixes #305 by being able to provide custom certificates via the standard env variables. |
084eb22 to
be0b344
Compare
|
Thanks! I'll update the PR description. |
jglogan
left a comment
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.
Can you drop REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE? We shouldn't be evaluating variables that are specific to Python requests or curl.
We should only use SSL_CERT_FILE (Go honors that one), and make the corresponding revert in containerization as well.
|
I agree with the above, sorry for approving the other two over in containerization. Those two seem odd looking back now, but the ssl_* ones are fairly standard across the board. |
Actually I think Go might only use that env variable on Linux. @sebimarkgraf @dcantah let's use this issue to circle back on both this and apple/containerization#402. Has this fix been verified in an environment that uses both a registry with a self-signed cert and registries that need the system trust store? Will the utility code only make it such the either a self-signed cert will be used or the system trust store, but not both? public enum TLSUtils {
public static func makeEnvironmentAwareTLSConfiguration() -> TLSConfiguration {
var tlsConfig = TLSConfiguration.makeClientConfiguration()
// Check standard SSL environment variables in priority order
let customCAPath =
ProcessInfo.processInfo.environment["SSL_CERT_FILE"]
?? ProcessInfo.processInfo.environment["CURL_CA_BUNDLE"]
?? ProcessInfo.processInfo.environment["REQUESTS_CA_BUNDLE"]
if let caPath = customCAPath {
tlsConfig.trustRoots = .file(caPath)
}
// else: use .default
return tlsConfig
}
} |
|
Agree with the reasoning, included them as these are the ones I usually look for. Reduced the list here to Fix has only been tested in an environment where my proxy has a self-signed cert via |
|
Thanks! I don't have a problem with us getting this feature in place as the use case (I have a private/local registry with a self-signed certificate and want to use it without having to allow the cert to be used system-wide). I wonder about the naming. If we chose If we chose There are of course other more complicated possibilities but let's make sure we (including @dcantah and anyone else interested!) all agree on the use case and then deliver the simplest feature that satisfies it. |
Type of Change
Motivation and Context
This is a follow-up to apple/containerization#402 allowing to easily use
containerwith custom SSL proxies.Especially in corporate setups, using an HTTPS Proxy is quite common for varying reasons and by following common coventions of env variables,
containercan be used easily without additional setup.Testing
Tagging @dcantah as he did the upstream PR review and this here is only the plumbing not the core change.