feat: allow member agent use OS's root certificate authority #364
Conversation
jim-minter
left a comment
jim-minter
left a comment
There was a problem hiding this comment.
A couple of nits, but LGTM. I have checked through the refactor carefully. Thanks for adding the unit test.
I have also checked that our base Docker image does indeed include a CA cert bundle.
@ryanzhang-oss please merge?
Co-authored-by: Jim Minter <jim-minter@users.noreply.github.com>
| hubConfig.TLSClientConfig.Insecure = tlsClientInsecure | ||
| if !tlsClientInsecure { | ||
| hubConfig.TLSClientConfig.CAFile = os.Getenv("CA_BUNDLE") |
There was a problem hiding this comment.
this value empty should not be supported when the TLS is secure. Linkedin has this use case.
There was a problem hiding this comment.
IMO, the original logic is specific for Linkedin, that is vendor specified and should not be in upstream's codes.
If CA_BUNDLE is empty, that will fallback to OS's default CA bundle. Other open sources are using same logic, for example curl. The OS's CA bundle is always be used by default unless user specific in argument or in environment variable CURL_CA_BUNDLE
| var ( | ||
| scheme = runtime.NewScheme() | ||
| useCAAuth = flag.Bool("use-ca-auth", false, "Use identity and CA bundle to authenticate the member agent.") | ||
| useCertificateAuth = flag.Bool("use-ca-auth", false, "Use key and certificate to authenticate the member agent.") |
There was a problem hiding this comment.
why change this name when it's passed down to the function as useCAAuth? I agree that the original name is not good.
There was a problem hiding this comment.
This PR has been merged, so I fixed it in separate PR: #365
| if !tlsClientInsecure { | ||
| hubConfig.TLSClientConfig.CAFile = os.Getenv("CA_BUNDLE") | ||
| hubCA := os.Getenv("HUB_CERTIFICATE_AUTHORITY") | ||
| if hubCA != "" { |
There was a problem hiding this comment.
we are using "" to imply that we use OS's root certificate but there is no way for us to know if user leave them blank deliberately or by mistake. I think we better ask user to explicitly indicate that they want to use OS root
| klog.ErrorS(err, "cannot decode hub cluster certificate authority data") | ||
| return nil, err | ||
| } | ||
| hubConfig.TLSClientConfig.CAData = caData |
There was a problem hiding this comment.
This will override the CAFile so it's another place that we want to make sure that users know what they are doing. It's either or.
There was a problem hiding this comment.
yes, the HUB_CERTIFICATE_AUTHORITY will override the CA_BUNDLE. I do agree that will make confuse for user.
Do you want me to add a validation for that? the validation will check only one of them will be present, otherwise will return a error
Description of your changes
When member agent use secure TLS talk to hub, it use either a specified CA bundle or CA data in configuration. This changes to allow member agent use OS's root certificate.
The changes are:
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer