Add function to get system cert pool #21
Conversation
|
Code LGTM (not a maintainer). Not sure I like the different behavior depending on compiler version. Would there be any issues with requiring Go 1.7 to build |
|
I just realized this package was being used here. Originally added here (https://github.com/docker/docker/tree/master/pkg/tlsconfig) but realized that package is not used. Without the alternative Docker builds will fail on less than 1.7. The "_other" is intended to be more of a catch all rather than an alternate implementation and using it will be no better and no worse than what you get today. |
|
Yes but I think it's a bad idea to have different behavior depending which compiler version you used. |
|
Ech. This |
|
@aaronlehmann I tried to design it such that the behavior for pre-1.7 is the same as the behavior if the system certs fail to load. The go version build flags were added for exactly this reason, not sure why it is a bad idea to use them, I agree they should be avoided when possible. @stevvooe I didn't agree with it getting separated and wouldn't mind it going back into docker. I believe it was split out to avoid a repository cyclical dependency for |
|
@dmcgowan So, I've moved some other items over to |
I agree. Since the behaviour in pre-go1.7 is a fallback, how about printing a warning when it's called? |
| func SystemCertPool() *x509.CertPool { | ||
| certpool, err := x509.SystemCertPool() | ||
| if err != nil { | ||
| return x509.NewCertPool() |
There was a problem hiding this comment.
Wondering if this should error, otherwise it's essentially a silent failure, which could result in unexpected behaviour.
There was a problem hiding this comment.
We can check for the windows ourselves here to avoid calling this when using windows, see https://golang.org/src/crypto/x509/cert_pool.go?s=730:770#L22. In that case we would still end up calling NewCertPool for windows.
| // SystemCertPool returns an new empty cert pool, | ||
| // accessing system cert pool is supported in go 1.7 | ||
| func SystemCertPool() *x509.CertPool { | ||
| return x509.NewCertPool() |
There was a problem hiding this comment.
Should we instead be trying to manually read from the default system pool locations to mimic the 1.7 behaviour manually?
There was a problem hiding this comment.
Trying to avoid forking code from the standard library here. This implementation is just to keep it compilable on other systems, official builds should use 1.7.
There was a problem hiding this comment.
It's possible if we change the interface to return an error, we can just return an error here when not on windows let the caller decide whether to use x509.NewCertPool() instead. At least in that case we can have output in the debug logs that the pool was not merged with the system pool.
There was a problem hiding this comment.
@dmcgowan We can log a warning here.
dmcgowan
force-pushed
the
system-cert-pools
branch
from
af4b51f
to
c401142
Compare
Defaults to getting empty cert pool when less than go 1.7 or on fails to load on Windows. Logs a warning when an empty cert pool is returned. Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
dmcgowan
force-pushed
the
system-cert-pools
branch
from
c401142
to
55aadc3
Compare
|
@stevvooe @dnephin added a warning whenever an empty cert pool is returned. @endophage if the system certificate pool fails to load on a non-windows system then it will return the error. Windows will always return an error for |
|
Sounds good |
Defaults to getting empty cert pool when less than go 1.7 or fails to load the system cert pool.
Needed for moby/moby#12756