Add bundle certificate support #253
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
k8s-ci-robot
added
cncf-cla: yes
src/KubernetesClient/CertUtils.cs
Outdated
| /// <returns>x509 instance.</returns> | ||
| public static X509Certificate2 LoadPemFileCert(string file) | ||
| /// <returns>List of x509 instances.</returns> | ||
| public static IList<X509Certificate2> LoadPemFileCert(string file) |
There was a problem hiding this comment.
should be X509Certificate2Collection
src/KubernetesClient/CertUtils.cs
Outdated
| // | ||
| var r = new Regex("-----BEGIN CERTIFICATE-----(.*?)-----END CERTIFICATE-----"); | ||
|
|
||
| var matches = r.Matches(certdata); |
There was a problem hiding this comment.
suggest to be new X509CertificateParser().ReadCertificates instead of building it ourselves.
There was a problem hiding this comment.
Definitely better. Only caveat is that we need to do some additional steps to convert the bouncy x509 certificates to .Net crypt x509 certs to store them in an X509Certificate2Collection, but that is trivial.
| // | ||
| foreach (var caCert in caCerts) | ||
| { | ||
| chain.ChainPolicy.ExtraStore.Add(caCert); |
| /// Checks | ||
| /// </summary> | ||
| [Fact] | ||
| public void LoadPemWithMultiCert() |
There was a problem hiding this comment.
this test is to ensure no regression when loading bundle.
There was a problem hiding this comment.
I've updated the test to make sure the certificates we've loaded match the original certificates we meant to load. Since the implementation of LoadPemFileCert has changed this test is a bit different than what you originally wrote. Let me know if there are any other preferred way to test this as the CertificateValidationTests now also include tests that verify that we can properly build certificates using the bundle.
| @@ -152,7 +154,7 @@ public Kubernetes(KubernetesClientConfiguration config, params DelegatingHandler | |||
| } | |||
| #endif | |||
|
|
|||
| private X509Certificate2 CaCert { get; } | |||
| private X509Certificate2Collection CaCert { get; } | |||
There was a problem hiding this comment.
| private X509Certificate2Collection CaCert { get; } | |
| private X509Certificate2Collection CaCerts { get; } |
There was a problem hiding this comment.
Updated this across the project
| @@ -58,7 +58,9 @@ public Kubernetes(KubernetesClientConfiguration config, params DelegatingHandler | |||
| throw new KubeConfigException("a CA must be set when SkipTlsVerify === false"); | |||
| } | |||
|
|
|||
| using (System.IO.MemoryStream certStream = new System.IO.MemoryStream(config.SslCaCert.RawData)) | |||
| var caCert = CaCert[CaCert.Count - 1]; | |||
There was a problem hiding this comment.
I'm not familiar with the Java libraries, but I changed it to at least add all the certs in the bundle to the trusted certificate list.
|
@tg123 I defer to your review, LGTM it when you think it's ready. |
|
/LGTM |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brendandburns, etchang The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Add support for cluster CA bundle certificates.
This changes the client to load potentially multiple certificates from the cluster's ca.crt file. When the client goes to validate an SSL connection, it will add these certs to the certificate chain provided by the server's response and validate that it can rebuild the chain. It will then make sure that the last certificate in the chain matches one of the certificates loaded from ca.crt to ensure the chain can be #trusted.
Fixes #234