search

cert-manager / trust-manager

trust-manager is an operator for distributing trust bundles across a Kubernetes cluster.
https://cert-manager.io/docs/projects/trust-manager/
Apache License 2.0
233 stars 64 forks source link

Improve certificate deduplication operation #303

Closed arsenalzp closed 4 months ago

arsenalzp commented 4 months ago

This PR fixes issue #299

jetstack-bot commented 4 months ago

Hi @arsenalzp. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
erikgb commented 4 months ago

/ok-to-test

arsenalzp commented 4 months ago

/ok-to-test

erikgb commented 4 months ago

/lgtm /hold (to allow other to review) /cc @SgtCoDFish

arsenalzp commented 4 months ago

/lgtm /hold (to allow other to review) /cc @SgtCoDFish

I found the issue, deduplication doesn't work for certificates which are joined into one source, for example:

a.crt:

-----BEGIN CERTIFICATE----- MIICGTCCAZ+gAwIBAgIQCeCTZaz32ci5PhwLBCou8zAKBggqhkjOPQQDAzBOMQsw CQYDVQQGEwJVUzEXMBUGA1UEChMORGlnaUNlcnQsIEluYy4xJjAkBgNVBAMTHURp Z2lDZXJ0IFRMUyBFQ0MgUDM4NCBSb290IEc1MB4XDTIxMDExNTAwMDAwMFoXDTQ2 MDExNDIzNTk1OVowTjELMAkGA1UEBhMCVVMxFzAVBgNVBAoTDkRpZ2lDZXJ0LCBJ bmMuMSYwJAYDVQQDEx1EaWdpQ2VydCBUTFMgRUNDIFAzODQgUm9vdCBHNTB2MBAG ByqGSM49AgEGBSuBBAAiA2IABMFEoc8Rl1Ca3iOCNQfN0MsYndLxf3c1TzvdlHJS 7cI7+Oz6e2tYIOyZrsn8aLN1udsJ7MgT9U7GCh1mMEy7H0cKPGEQQil8pQgO4CLp 0zVozptjn4S1mU1YoI71VOeVyaNCMEAwHQYDVR0OBBYEFMFRRVBZqz7nLFr6ICIS B4CIfBFqMA4GA1UdDwEB/wQEAwIBhjAPBgNVHRMBAf8EBTADAQH/MAoGCCqGSM49 BAMDA2gAMGUCMQCJao1H5+z8blUD2WdsJk6Dxv3J+ysTvLd6jLRl0mlpYxNjOyZQ LgGheQaRnUi/wr4CMEfDFXuxoJGZSZOoPHzoRgaLLPIxAJSdYsiJvRmEFOml+wG4 DXZDjC5Ty3zfDBeWUA== -----END CERTIFICATE-----

b.crt:

-----BEGIN CERTIFICATE----- MIICGTCCAZ+gAwIBAgIQCeCTZaz32ci5PhwLBCou8zAKBggqhkjOPQQDAzBOMQsw CQYDVQQGEwJVUzEXMBUGA1UEChMORGlnaUNlcnQsIEluYy4xJjAkBgNVBAMTHURp Z2lDZXJ0IFRMUyBFQ0MgUDM4NCBSb290IEc1MB4XDTIxMDExNTAwMDAwMFoXDTQ2 MDExNDIzNTk1OVowTjELMAkGA1UEBhMCVVMxFzAVBgNVBAoTDkRpZ2lDZXJ0LCBJ bmMuMSYwJAYDVQQDEx1EaWdpQ2VydCBUTFMgRUNDIFAzODQgUm9vdCBHNTB2MBAG ByqGSM49AgEGBSuBBAAiA2IABMFEoc8Rl1Ca3iOCNQfN0MsYndLxf3c1TzvdlHJS 7cI7+Oz6e2tYIOyZrsn8aLN1udsJ7MgT9U7GCh1mMEy7H0cKPGEQQil8pQgO4CLp 0zVozptjn4S1mU1YoI71VOeVyaNCMEAwHQYDVR0OBBYEFMFRRVBZqz7nLFr6ICIS B4CIfBFqMA4GA1UdDwEB/wQEAwIBhjAPBgNVHRMBAf8EBTADAQH/MAoGCCqGSM49 BAMDA2gAMGUCMQCJao1H5+z8blUD2WdsJk6Dxv3J+ysTvLd6jLRl0mlpYxNjOyZQ LgGheQaRnUi/wr4CMEfDFXuxoJGZSZOoPHzoRgaLLPIxAJSdYsiJvRmEFOml+wG4 DXZDjC5Ty3zfDBeWUA== -----END CERTIFICATE----- -----BEGIN CERTIFICATE----- MIIEmTCCA4GgAwIBAgIQKfox3OU/IAcSUwfq9Oc3pjANBgkqhkiG9w0BAQsFADBG MQswCQYDVQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZpY2VzIExM QzETMBEGA1UEAxMKR1RTIENBIDFDMzAeFw0yNDAyMDUwODE5NTFaFw0yNDA0Mjkw ODE5NTBaMBoxGDAWBgNVBAMTD21haWwuZ29vZ2xlLmNvbTBZMBMGByqGSM49AgEG CCqGSM49AwEHA0IABBu++VA4p3sxRK4wYGWkD2HHvwjQKCHhbNr8d9SoQiSIzdzY mab2Xnhr81PgSkR7crQbpRM3kB4WOEMOz/FlkG6jggJ4MIICdDAOBgNVHQ8BAf8E BAMCB4AwEwYDVR0lBAwwCgYIKwYBBQUHAwEwDAYDVR0TAQH/BAIwADAdBgNVHQ4E FgQUi3kZPzAhKIuvoxUfX9DKZ6kdS9IwHwYDVR0jBBgwFoAUinR/r4XN7pXNPZzQ 4kYU83E1HScwagYIKwYBBQUHAQEEXjBcMCcGCCsGAQUFBzABhhtodHRwOi8vb2Nz cC5wa2kuZ29vZy9ndHMxYzMwMQYIKwYBBQUHMAKGJWh0dHA6Ly9wa2kuZ29vZy9y ZXBvL2NlcnRzL2d0czFjMy5kZXIwLAYDVR0RBCUwI4IPbWFpbC5nb29nbGUuY29t ghBpbmJveC5nb29nbGUuY29tMCEGA1UdIAQaMBgwCAYGZ4EMAQIBMAwGCisGAQQB 1nkCBQMwPAYDVR0fBDUwMzAxoC+gLYYraHR0cDovL2NybHMucGtpLmdvb2cvZ3Rz MWMzL2ZWSnhiVi1LdG1rLmNybDCCAQIGCisGAQQB1nkCBAIEgfMEgfAA7gB1AO7N 0GTV2xrOxVy3nbTNE6Iyh0Z8vOzew1FIWUZxH7WbAAABjXiRG0oAAAQDAEYwRAIg CkCRFKqRFIAL097RdzzrkZc+Mr8Y+9yTw1pklOwdV3cCICZAl6Km8/xBVpK+7wGj i/lZEx7jB/z8y1xB42n0rqzAAHUASLDja9qmRzQP5WoC+p0w6xxSActW3SyB2bu/ qznYhHMAAAGNeJEbbQAABAMARjBEAiASElVpHUrwUrbEyyipW14O0HlQFguECtF0 6wqE0fHMQQIgRPRcWNMteFnaOtONT53o1tVcwfi5E1DE1UJ4pqC2/ykwDQYJKoZI hvcNAQELBQADggEBADAsLdvk4zEDtkWxGLH+zTe6GiA/NJhEm0D30Uxqq41ZMFa7 QGVj58CFSyvRznEGBp/LZMGgFegLIe37hSHy72IMxgI4t+iSkOBLGQRxvrQaLqu+ /GPmgTPiLB3oNZeROMOTZinLsxM2MPV/ygPKH+ZDv6GACFPmF+2ujthYNJLUgFC3 fBAndiWvBUPQ0O96s3vOsSjCD8J19ZX55e1VF6DS/yY84sZ2iub4b4OeHDcGKQux 79Aw3bk/l+giFAKvI3XAZIrzD+lGLmWK4uc3vt8DhRh2xHCKy6pZUUJ6AWCS9YC5 E7YqVpuqZ0TeK+pdo1ciBT1c2qHEEjKi1T7P8Ew= -----END CERTIFICATE-----

However it works for a single certificate per one source, like this: a.crt:

-----BEGIN CERTIFICATE---- cert data_1 -----END CERTIFICATE-----

b.crt:

-----BEGIN CERTIFICATE---- cert data_1 -----END CERTIFICATE-----

So, I made some improvements for my code, as well as for test.

erikgb commented 4 months ago

/retest

arsenalzp commented 4 months ago

@erikgb I guess it would be better to maintain a single cert pool across all calls, up until the populate() call, where the final string content is produced. For example, the call ValidateAndSanitizePEMBundleWithOptions() should receive sourceData, opts; it should return cert pool which is accepted as arguement by other calls, hence every single call might manipulate with cert pool by their own way: deduplicate certs, remove expired, or even verify the validity of cers chain.

erikgb commented 4 months ago

@erikgb I guess it would be better to maintain a single cert pool across all calls, up until the populate() call, where the final string content is produced. For example, the call ValidateAndSanitizePEMBundleWithOptions() should receive sourceData, opts; it should return cert pool which is accepted as arguement by other calls, hence every single call might manipulate with cert pool by their own way: deduplicate certs, remove expired, or even verify the validity of cers chain.

Yes, that was what I was also thinking in my review comment. But I suggest to do it in a separate PR, as this PR is almost GTM IMO.

/lgtm

arsenalzp commented 4 months ago

@erikgb I guess it would be better to maintain a single cert pool across all calls, up until the populate() call, where the final string content is produced. For example, the call ValidateAndSanitizePEMBundleWithOptions() should receive sourceData, opts; it should return cert pool which is accepted as arguement by other calls, hence every single call might manipulate with cert pool by their own way: deduplicate certs, remove expired, or even verify the validity of cers chain.

Yes, that was what I was also thinking in my review comment. But I suggest to do it in a separate PR, as this PR is almost GTM IMO.

/lgtm

Thank you for your reviews, hints and work! Let me know if you need my help with that new PR, I kindly prepare it.

erikgb commented 4 months ago

This looks like it could maybe use more refactoring to use CertPool or something, but I'm OK with merging if it improves things.

I created https://github.com/cert-manager/trust-manager/issues/305 as a placeholder issue. I suspect it to affect multiple areas of the code, especially unit tests, so I think it makes sense to do it in a separate PR. @arsenalzp has gracefully volunteered to look into it.

erikgb commented 4 months ago

~@SgtCoDFish Are you ok with unholding this PR now?~ NVM, I thought the debug logging was fixed....

arsenalzp commented 4 months ago

~@SgtCoDFish Are you ok with unholding this PR now?~ NVM, I thought the debug logging was fixed....

l'll do it once get home this evening.

arsenalzp commented 4 months ago

It seems, I did something wrong during rebase...

SgtCoDFish commented 4 months ago

Yeah the rebase looks wrong for sure.

It looks like the commit hash we want is e497e02 looking at the history in this PR. You might be able to use something like this stackoverflow post to get back to that commit!

arsenalzp commented 4 months ago

Yeah the rebase looks wrong for sure.

It looks like the commit hash we want is e497e02 looking at the history in this PR. You might be able to use something like this stackoverflow post to get back to that commit!

It's done. Thank you for the hint.

arsenalzp commented 4 months ago

/retest

arsenalzp commented 4 months ago

/retest

SgtCoDFish commented 4 months ago

/test pull-trust-manager-smoke

Jiawei0227 commented 4 months ago

I think the point is it would be great if it can generate determinstic trust bundle. Regardless of ca order in the source.

On Tue, Feb 27, 2024, 14:41 Oleksandr Krutko @.***> wrote:

@.**** commented on this pull request.

In pkg/bundle/sync.go https://github.com/cert-manager/trust-manager/pull/303#discussion_r1505103167 :

@@ -130,7 +131,12 @@ func (b bundle) buildSourceBundle(ctx context.Context, bundle trustapi.Bundle) return bundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle") }

  • if err := resolvedBundle.populateData(bundles, bundle.Spec.Target); err != nil {
  • deduplicatedBundles, err := deduplicateBundles(bundles)

Which key should be used for sort?

— Reply to this email directly, view it on GitHub https://github.com/cert-manager/trust-manager/pull/303#discussion_r1505103167, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZKDPBDIDE4QQBWMSHY7HDYVZOKRAVCNFSM6AAAAABDVO3DACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBUHE2TIMJWG4 . You are receiving this because you commented.Message ID: @.***>

erikgb commented 4 months ago

I think the point is it would be great if it could generate a deterministic trust bundle. Regardless of CA order in the source.

I agree, but would put it even stronger: "We MUST.." 😉

arsenalzp commented 4 months ago

I think the point is it would be great if it can generate determinstic trust bundle. Regardless of ca order in the source. … On Tue, Feb 27, 2024, 14:41 Oleksandr Krutko @.> wrote: @*.*** commented on this pull request. ------------------------------ In pkg/bundle/sync.go <#303 (comment)> : > @@ -130,7 +131,12 @@ func (b bundle) buildSourceBundle(ctx context.Context, bundle trustapi.Bundle) return bundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle") } - if err := resolvedBundle.populateData(bundles, bundle.Spec.Target); err != nil { + deduplicatedBundles, err := deduplicateBundles(bundles) Which key should be used for sort? — Reply to this email directly, view it on GitHub <#303 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACZKDPBDIDE4QQBWMSHY7HDYVZOKRAVCNFSM6AAAAABDVO3DACVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMBUHE2TIMJWG4 . You are receiving this because you commented.Message ID: @.>

I guess it should be done in separate PR. During the deduplication process certificates order is not changed, it goes FIFO: the first certificate is first in the result bundle, if an another equal certificate is followed by the first one then it will be removed from the bundle. However, I guess an order of certificates is user's duty, only user is aware of the right order. Deduplication function is to remediate the case when careless user just do a copy-paste of the same certificates in many sources.

erikgb commented 4 months ago

it goes FIFO: the first certificate is first in the result bundle, if an another equal certificate is followed by the first one then it will be removed from the bundle.

I consider this to be deterministic enough.

arsenalzp commented 4 months ago

it goes FIFO: the first certificate is first in the result bundle, if an another equal certificate is followed by the first one then it will be removed from the bundle.

I consider this to be deterministic enough.

Moreover, dedup function doesn't shuffle certs, and only a user knows the right certs order, if we would do a sort of its certs - it might cause a bother...

SgtCoDFish commented 4 months ago

I'd be interested to explore fully deterministic ordering further in a separate issue & PR. I'd do it alphanumerically based on the hex-encoded sha256 certificate fingerprint personally!

jetstack-bot commented 4 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SgtCoDFish

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: - ~~[OWNERS](https://github.com/cert-manager/trust-manager/blob/main/OWNERS)~~ [SgtCoDFish] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
SgtCoDFish commented 4 months ago

/unhold