-
Notifications
You must be signed in to change notification settings - Fork 102
Revert change for windows cert pool access in 1.8 #29
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
Revert change for windows cert pool access in 1.8 #29
Conversation
LGTM |
tlsconfig/certpool_go17.go
Outdated
@@ -14,7 +14,7 @@ import ( | |||
func SystemCertPool() (*x509.CertPool, error) { | |||
certpool, err := x509.SystemCertPool() | |||
if err != nil && runtime.GOOS == "windows" { | |||
logrus.Infof("Unable to use system certificate pool: %v", err) | |||
logrus.Infof("Unable to use system certificate pool with custom certificate pool: %v", err) |
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.
Bit unsure about the error
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.
The wording or the assumption that is used for a custom certificate pool?
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.
The wording is a bit confusing "use system pool with custom pool". Is the warning that they cannot be combined? (sorry can't fully grasp what it's saying 😄)
The upstream change to allow access to the windows system cert pool was reverted, reverting and updating messaging. Maybe 1.9....golang/go#18609 Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
6f510e0
to
f652133
Compare
Removed updated info message, whatever messaging is needed can be done by Docker. Added an empty check on the server side before calling that function to prevent setting an empty pool instead of nil. |
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.
LGTM
The upstream change to allow access to the windows system cert pool was reverted, reverting and updating messaging. Maybe 1.9....golang/go#18609
Thanks @cyli for catching this!
ping @aaronlehmann