Skip to content

Fix multi-certificate loading from certificate-authority-data - #1690

Merged
k8s-ci-robot merged 6 commits into
masterfrom
copilot/fix-certificate-loading-issue
Jan 6, 2026
Merged

k8s-ci-robot merged 6 commits into
masterfrom
copilot/fix-certificate-loading-issue

Conversation

Copilot AI commented Nov 25, 2025 •

Copy link
Copy Markdown
Contributor
  • Address @tg123's feedback about .NET Framework 4 compatibility
  • Add CertUtils.LoadFromPemText() for all frameworks
  • Use ImportFromPem for modern .NET (5.0+)
  • Use BouncyCastle for Classic (.NET Standard 2.0, .NET Framework 4.8)
  • Simplify code by using CertUtils methods
  • Fix build issues
  • All tests passing
  • Add dedicated unit tests for LoadFromPemText method

Summary

Added dedicated unit tests for CertUtils.LoadFromPemText() method:

  • LoadFromPemTextWithMultiCert: Tests loading multiple certificates from PEM text
  • LoadFromPemTextWithSingleCert: Tests loading a single certificate from PEM text

Tests follow the same pattern as the existing LoadPemWithMultiCert test and provide explicit validation of the method's behavior with various PEM text inputs.

Original prompt

This section details on the original issue you should resolve

<issue_title>Not all certificates from certificate-authority-data are loaded</issue_title>
<issue_description>Describe the bug
If i export a kubeconfig from Rancher there are three certificates present in the certificate-authority-data.

SetClusterDetails in KubernetesClientConfiguration.ConfigFile.cs uses X509CertificateLoader.LoadCertificate to load the certificate, but that does not support certificate collections. Therefore only the first certificate is loaded and the other two are lost and not added to SslCaCerts.

Kubernetes C# SDK Client Version
18.0.5

Dotnet Runtime Version
.NET 9.0+

To Reproduce
Load a kubeconfig file with more than one certificate. SslCaCerts will only contain the first one.

Expected behavior
All certificates are loaded.

Potential fix
X509Certificate2Collection.ImportFromPem imports a collection of PEM certificates and could be a potential fix. At least thats what we do as a workaround for now:

// Import all Certificates
var certData = kubernetesConfiguration.Clusters.FirstOrDefault()?.ClusterEndpoint?.CertificateAuthorityData;

if (!string.IsNullOrWhiteSpace(certData))
{
    var collection = new X509Certificate2Collection();
    var pemText = Encoding.UTF8.GetString(Convert.FromBase64String(certData));
    collection.ImportFromPem(pemText);

    kubernetesClientConfig.SslCaCerts = collection;
}
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
</comments>

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 25, 2025
Co-authored-by: tg123 <170430+tg123@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 25, 2025
Co-authored-by: tg123 <170430+tg123@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix loading of all certificates from certificate-authority-data Fix multi-certificate loading from certificate-authority-data Nov 25, 2025
Copilot AI requested a review from tg123 November 25, 2025 21:00
@tg123

tg123 commented Nov 25, 2025

Copy link
Copy Markdown
Member

- Add CertUtils.LoadFromPemText() method for both modern .NET and Classic
- Use ImportFromPem for .NET 5.0+ (net8.0, net9.0, net10.0)
- Use BouncyCastle for .NET Standard 2.0 and .NET Framework 4.8
- Simplify SetClusterDetails to use CertUtils methods for both certificate-authority-data and certificate-authority

Co-authored-by: tg123 <170430+tg123@users.noreply.github.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 25, 2025

Copilot AI commented Nov 25, 2025

Copy link
Copy Markdown
Contributor Author

@copilot

hi accroding to https://learn.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509certificate2collection.importfrompem?view=net-9.0
...

Good catch! I've addressed the .NET Framework compatibility issue in commit 2016d1f.

The fix now:

  • Uses ImportFromPem for modern .NET versions (5.0+, including net8.0/net9.0/net10.0)
  • Uses BouncyCastle's X509CertificateParser.ReadCertificates() for .NET Standard 2.0 and .NET Framework 4.8 in the Classic project
  • Adds CertUtils.LoadFromPemText() method with framework-specific implementations to handle multiple certificates from PEM text
  • Simplifies SetClusterDetails to use CertUtils methods for both code paths

Both certificate-authority-data and certificate-authority now support multiple certificates across all target frameworks.

@tg123

tg123 commented Nov 26, 2025

Copy link
Copy Markdown
Member

@copilot could you please fix build

- Remove unused System.Security.Cryptography.X509Certificates using from ConfigFile.cs
- Add System.Text using to Classic CertUtils.cs for Encoding class

Co-authored-by: tg123 <170430+tg123@users.noreply.github.com>

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

@copilot could you please fix build

Build fixed in commit a86c2e2. The issues were:

  • Removed unnecessary System.Security.Cryptography.X509Certificates using from ConfigFile.cs
  • Added missing System.Text using to Classic CertUtils.cs

Both KubernetesClient and KubernetesClient.Classic projects now build successfully, and all tests pass.

@tg123
tg123 marked this pull request as ready for review November 26, 2025 19:27
Copilot AI review requested due to automatic review settings November 26, 2025 19:27
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 26, 2025
@avin3sh

avin3sh commented Feb 12, 2026

Copy link
Copy Markdown
Contributor

We are also keen on using this fix and would appreciate if it can be accommodated in a release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Not all certificates from certificate-authority-data are loaded

7 participants