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

Revert change for windows cert pool access in 1.8 #29

Merged
merged 1 commit into from
Feb 3, 2017

Conversation

dmcgowan
Copy link
Contributor

@dmcgowan dmcgowan commented Feb 3, 2017

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

@aaronlehmann
Copy link

LGTM

@@ -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)
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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)
@dmcgowan dmcgowan force-pushed the revert-windows-go-18-system-pool branch from 6f510e0 to f652133 Compare February 3, 2017 23:51
@dmcgowan
Copy link
Contributor Author

dmcgowan commented Feb 3, 2017

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.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 7da10c8 into docker:master Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants