Let it be possible to specify optional CAcert for trust of remote cfssl #51
Conversation
|
@cbroglie Don't know if you work with certmgr as well? But since you merged my latest PR on cfssl, perhaps you'd care to review this as well? :) thanks in any case. |
cert/cert.go
Outdated
| if len(ca.RootCACert) > 0 { | ||
| rootCABytes, err := ioutil.ReadFile(ca.RootCACert) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
why panic here? why not simply return nil, err?
cert/cert.go
Outdated
| rootCaCertPool := x509.NewCertPool() | ||
| ok := rootCaCertPool.AppendCertsFromPEM(rootCABytes) | ||
| if !ok { | ||
| panic("failed to parse rootCA certs") |
There was a problem hiding this comment.
you should probably return nil, errors.New("failed to parse rootCA certs")
cert/cert.go
Outdated
| panic(err) | ||
| } | ||
|
|
||
| if err == nil { |
There was a problem hiding this comment.
Go devs are not fond of if..else blocks, can you change this entire block to:
if err != nil {
return nil, err
}
rootCaCertPool := x509.NewCertPool() ...
also don't panic :)
cert/cert.go
Outdated
| pem []byte | ||
| } | ||
|
|
||
| func (ca *CA) getRemoteCert() ([]byte, error) { | ||
| remote := client.NewServer(ca.Remote) | ||
|
|
…sl tls cert. This makes it possible to use self-signed certificate on the cfssl apiserver.
johanot
force-pushed
the
remote-tls-rootca
branch
from
3fe00d5
to
bfbe3b4
Compare
|
@johanot no worries :), it looks great now, hopwfully someone can merge this soon. Today, I will take a look at the k8s pr over on nixpkgs. |
|
@kisom Can I kindly ask for your comment/opinion on this change? |
johanot
force-pushed
the
remote-tls-rootca
branch
from
2a7743a
to
bfbe3b4
Compare
|
@terinjokes Poking blindly here, but I would be very grateful if you or one of your colleagues could take a look at this PR and tell me whether it makes sense? :) We would like to include certmgr 1.6.1 (with this patch) in the upstream NixOS Kubernetes module. |
|
@kisom @terinjokes Re-ping -> one month after PR creation. |
cert/cert.go
Outdated
| pem []byte | ||
| } | ||
|
|
||
| func (ca *CA) getRemoteCert() ([]byte, error) { | ||
| remote := client.NewServer(ca.Remote) | ||
| var tlsConfig tls.Config | ||
| if len(ca.RootCACert) > 0 { |
There was a problem hiding this comment.
For consistency with other checks in this file (eg, AuthKeyFile), we might want to compare against an empty string, rather than a length check.
There was a problem hiding this comment.
Thanks @terinjokes.
Fixed in 519f8db .. Will squash if you approve. Any other comments to the functionality in general? It does make sense to implement support for self-signed certs on cfssl-apiserver this way right?
|
@terinjokes @kisom Is this something you will consider merging at some point, or is it just too far from the the core principals of certmgr? We've discussed in the NixOS kubernetes community whether to fork certmgr to be able to use self-signed certs with cfssl, or to build a new cfssl client; but I genuinely think that would be sad. I very much prefer to hear some sort of opinion from you on this, so that we can decide on whether to stick with upstream certmgr as cfssl client or not. Thanks a lot. :) |
|
@terinjokes @kisom Any updates on this? |
|
@johanot @adamtulinius I'm no longer at Cloudflare and don't have access to this repo anymore :). |
cert/cert.go
Outdated
| } | ||
| } | ||
|
|
||
| remote := client.NewServerTLS(ca.Remote, &tlsConfig) |
There was a problem hiding this comment.
Unless I'm missing something, this code change will result in an empty/zero initialized struct being passed for tlsConfig- rather than nil; does the consuming code make a difference in that regard?
More specifically, I want to confirm that these changes won't break the existing pathway, and tests don't exist so it's hard to evaluate this as a drive by reviewer.
There was a problem hiding this comment.
@ferringb You're right. Changed the tlsConfig var to be a pointer instead: aa36ac4 ... nil should be supported by the cfssl client lib. (https://github.com/cloudflare/cfssl/blob/master/api/client/client.go#L138)
|
@johanot it is- pardon the delay in follow up. |
If you run cfssl apiserver tls-enabled, it is currently not possible to use self-signed certificate, because certmgr will reject it.
This PR makes it possible to use self-signed certificate on the cfssl apiserver, by providing an optional cert-spec option
root_ca.