Improve certificate deduplication operation #303

arsenalzp · 2024-02-22T18:40:30Z

This PR fixes issue #299

jetstack-bot · 2024-02-22T18:40:40Z

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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

erikgb · 2024-02-22T19:08:11Z

/ok-to-test

erikgb

Thanks for this, @arsenalzp! I've added some comments/questions, but I consider this to be a good change.

/approve

pkg/bundle/sync_test.go

pkg/bundle/sync.go

arsenalzp · 2024-02-25T14:23:03Z

/ok-to-test

erikgb · 2024-02-25T16:16:11Z

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

arsenalzp · 2024-02-25T18:31:17Z

/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.

pkg/bundle/sync_test.go

pkg/bundle/sync.go

erikgb · 2024-02-25T20:47:57Z

/retest

arsenalzp · 2024-02-25T21:26:54Z

@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 · 2024-02-25T21:55:34Z

@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 · 2024-02-25T21:59:24Z

@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.

SgtCoDFish

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

Just one comment from me!

pkg/bundle/sync.go

erikgb · 2024-02-26T09:49:15Z

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 #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 · 2024-02-26T15:32:30Z

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

arsenalzp · 2024-02-26T22:05:32Z

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

SgtCoDFish · 2024-02-27T09:55:26Z

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 · 2024-02-27T16:21:29Z

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 · 2024-02-27T16:21:52Z

/retest

pkg/bundle/sync.go

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> improvement for TestBundlesDeduplication and name convention Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> improve deduplication process by using pem features Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> add more test of deduplication process Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> remove unnecessary calls in sync.go Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> add error parameter in deduplicateBundles() Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

arsenalzp · 2024-02-27T17:26:14Z

/retest

SgtCoDFish · 2024-02-27T19:31:08Z

/test pull-trust-manager-smoke

pkg/bundle/sync.go

Jiawei0227 · 2024-02-28T07:13:12Z

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: ***@***.***>

erikgb · 2024-02-28T07:17:35Z

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 · 2024-02-28T07:27:28Z

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 · 2024-02-28T07:32:46Z

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 · 2024-02-28T07:39:26Z

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 · 2024-02-28T09:39:14Z

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!

SgtCoDFish

/lgtm
/approve

Thank you for this, I think it's good to merge personally. Really appreciate it!

jetstack-bot · 2024-02-28T09:40:09Z

[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~~ [SgtCoDFish]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

SgtCoDFish · 2024-02-28T10:44:49Z

/unhold

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2024

jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 22, 2024

jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 22, 2024

erikgb reviewed Feb 25, 2024

View reviewed changes

pkg/bundle/sync_test.go Outdated Show resolved Hide resolved

pkg/bundle/sync.go Outdated Show resolved Hide resolved

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2024

arsenalzp force-pushed the main branch from bc8d8c9 to 0dae48b Compare February 25, 2024 14:21

jetstack-bot requested a review from SgtCoDFish February 25, 2024 16:16

jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 25, 2024

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2024

arsenalzp force-pushed the main branch from 8e1a3ee to bc20fbb Compare February 25, 2024 18:43

erikgb reviewed Feb 25, 2024

View reviewed changes

pkg/bundle/sync_test.go Outdated Show resolved Hide resolved

erikgb reviewed Feb 25, 2024

View reviewed changes

pkg/bundle/sync.go Show resolved Hide resolved

arsenalzp force-pushed the main branch from bc20fbb to da6d820 Compare February 25, 2024 19:48

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2024

erikgb mentioned this pull request Feb 26, 2024

Avoid multiple decode/encode of certificates #305

Closed

SgtCoDFish requested changes Feb 26, 2024

View reviewed changes

pkg/bundle/sync.go Outdated Show resolved Hide resolved

jetstack-bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Feb 26, 2024

arsenalzp force-pushed the main branch from ccfde7e to 8d17320 Compare February 26, 2024 22:04

arsenalzp force-pushed the main branch from 8d17320 to e497e02 Compare February 27, 2024 16:20

arsenalzp requested a review from SgtCoDFish February 27, 2024 16:31

erikgb requested changes Feb 27, 2024

View reviewed changes

pkg/bundle/sync.go Outdated Show resolved Hide resolved

pkg/bundle/sync.go Show resolved Hide resolved

jetstack-bot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2024

arsenalzp force-pushed the main branch from cedb023 to 225d70d Compare February 27, 2024 17:25

Jiawei0227 reviewed Feb 27, 2024

View reviewed changes

pkg/bundle/sync.go Show resolved Hide resolved

SgtCoDFish approved these changes Feb 28, 2024

View reviewed changes

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2024

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2024

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2024

jetstack-bot merged commit 67d4784 into cert-manager:main Feb 28, 2024
3 of 4 checks passed

Jiawei0227 mentioned this pull request Feb 28, 2024

Provide deterministic bundle #310

Closed

arsenalzp mentioned this pull request Apr 25, 2024

trust-manager deduplication doesnt work #299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve certificate deduplication operation #303

Improve certificate deduplication operation #303

arsenalzp commented Feb 22, 2024

jetstack-bot commented Feb 22, 2024

erikgb commented Feb 22, 2024

erikgb left a comment

arsenalzp commented Feb 25, 2024

erikgb commented Feb 25, 2024

arsenalzp commented Feb 25, 2024 •

edited

Loading

erikgb commented Feb 25, 2024

arsenalzp commented Feb 25, 2024 •

edited

Loading

erikgb commented Feb 25, 2024

arsenalzp commented Feb 25, 2024 •

edited

Loading

SgtCoDFish left a comment

erikgb commented Feb 26, 2024

erikgb commented Feb 26, 2024 •

edited

Loading

arsenalzp commented Feb 26, 2024

SgtCoDFish commented Feb 27, 2024

arsenalzp commented Feb 27, 2024

arsenalzp commented Feb 27, 2024

arsenalzp commented Feb 27, 2024

SgtCoDFish commented Feb 27, 2024

Jiawei0227 commented Feb 28, 2024 via email

erikgb commented Feb 28, 2024

arsenalzp commented Feb 28, 2024 •

edited

Loading

erikgb commented Feb 28, 2024

arsenalzp commented Feb 28, 2024 •

edited

Loading

SgtCoDFish commented Feb 28, 2024

SgtCoDFish left a comment

jetstack-bot commented Feb 28, 2024

SgtCoDFish commented Feb 28, 2024

Improve certificate deduplication operation #303

Improve certificate deduplication operation #303

Conversation

arsenalzp commented Feb 22, 2024

jetstack-bot commented Feb 22, 2024

erikgb commented Feb 22, 2024

erikgb left a comment

Choose a reason for hiding this comment

arsenalzp commented Feb 25, 2024

erikgb commented Feb 25, 2024

arsenalzp commented Feb 25, 2024 • edited Loading

erikgb commented Feb 25, 2024

arsenalzp commented Feb 25, 2024 • edited Loading

erikgb commented Feb 25, 2024

arsenalzp commented Feb 25, 2024 • edited Loading

SgtCoDFish left a comment

Choose a reason for hiding this comment

erikgb commented Feb 26, 2024

erikgb commented Feb 26, 2024 • edited Loading

arsenalzp commented Feb 26, 2024

SgtCoDFish commented Feb 27, 2024

arsenalzp commented Feb 27, 2024

arsenalzp commented Feb 27, 2024

arsenalzp commented Feb 27, 2024

SgtCoDFish commented Feb 27, 2024

Jiawei0227 commented Feb 28, 2024 via email

erikgb commented Feb 28, 2024

arsenalzp commented Feb 28, 2024 • edited Loading

erikgb commented Feb 28, 2024

arsenalzp commented Feb 28, 2024 • edited Loading

SgtCoDFish commented Feb 28, 2024

SgtCoDFish left a comment

Choose a reason for hiding this comment

jetstack-bot commented Feb 28, 2024

SgtCoDFish commented Feb 28, 2024

arsenalzp commented Feb 25, 2024 •

edited

Loading

arsenalzp commented Feb 25, 2024 •

edited

Loading

arsenalzp commented Feb 25, 2024 •

edited

Loading

erikgb commented Feb 26, 2024 •

edited

Loading

arsenalzp commented Feb 28, 2024 •

edited

Loading

arsenalzp commented Feb 28, 2024 •

edited

Loading