Add certificates deduplication feature #184

arsenalzp · 2023-09-23T17:42:36Z

This PR is related to the issue de-duplicate CA from the trust bundle #155
It add feature of certificates de-duplication, meaning to exclude certificates which were already added to a certificates bundle, thus save memory.

jetstack-bot · 2023-09-23T17:42:46Z

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

@arsenalzp, thanks for this! This is almost LGTM from me. I have just a few comments/questions.

erikgb · 2023-09-23T19:05:19Z

pkg/util/pem.go

@@ -120,3 +125,19 @@ func DecodeX509CertificateChainBytes(certBytes []byte) ([]*x509.Certificate, err

 	return certs, nil
 }
+
+// Check if the given certificate was added to certificates bundle already
+func IsCertificateDuplicate(certsHash map[[32]byte]struct{}, certData []byte) bool {


What do you think about creating a type for this function? Maybe it could be named certPool? 😉 With an appendCertFromPEM func, the type could also hold the certificates returned from the outer function.

I think I'd support this as a new type, ideally in internal if possible so nobody else relies on it!

I added CertPool type type with related methods.
It's not possible to put that structure into internal directory, due to limitation of importing packages which are located in internal sub-directories.

erikgb · 2023-09-23T19:08:02Z

pkg/util/pem.go

+// Check if the given certificate was added to certificates bundle already
+func IsCertificateDuplicate(certsHash map[[32]byte]struct{}, certData []byte) bool {
+	// calculate hash sum of the given certificate
+	hash := sha256.Sum256(certData)


I note that x509.CertPool uses sha256.Sum224 for deduplicating certs. Any particular reason you went for something else?

Interesting that CertPool uses sha224! I think sha256 is fine here though, it's what I'd have reached for as a "default"

Hello,
Thank you so much for your feedback, it is very appreciated!
There weren't any reasons to use sha224 against sha256, they have almost equal computation complexity, bus some differences have present:

https://www.rfc-editor.org/rfc/rfc3874.txt

1.1. Usage Considerations

Since SHA-224 is based on SHA-256, roughly the same amount of effort
is consumed to compute a SHA-224 or a SHA-256 digest message digest
value. Even though SHA-224 and SHA-256 have roughly equivalent
computational complexity, SHA-224 is an appropriate choice for a
one-way hash function that provides 112 bits of security. The use of
a different initial value ensures that a truncated SHA-256 message
digest value cannot be mistaken for a SHA-224 message digest value
computed on the same data.

Some usage environments are sensitive to every octet that is
transmitted. In these cases, the smaller (by 4 octets) message
digest value provided by SHA-224 is important.

These observations lead to the following guidance:

When selecting a suite of cryptographic algorithms that all offer
112 bits of security strength, SHA-224 is an appropriate choice
for one-way hash function.

When terseness is not a selection criteria, the use of SHA-256 is
a preferred alternative to SHA-224.

For our purposes we have to choice algorithm which proposes:

acceptable computation complexity and

minimize hash collisions

I'm fine with sha256. 😃

erikgb · 2023-09-25T20:51:56Z

pkg/util/cert_pool.go

+)
+
+// CertPool is a set of certificates.
+type CertPool struct {


Ref. #184 (comment) I think this type should be defined in an internal package. Or as an unexported type in the package it's used. We mustn't tempt anyone to use it. :-)

Sounds good!
Let's do it!

CertPool and its methods were made unexported, so certPool is used by util package only

erikgb · 2023-09-25T21:30:09Z

pkg/util/pem.go

@@ -87,7 +88,10 @@ func ValidateAndSplitPEMBundle(data []byte) ([][]byte, error) {
 			return nil, fmt.Errorf("invalid PEM block in bundle; invalid PEM certificate: %w", err)
 		}

-		certificates = append(certificates, pem.EncodeToMemory(b))
+		// check the duplicate certificates
+		if !certPool.IsCertificateDuplicate(b.Bytes) {


Some changes missing here? I think the idea was to use the new certPool type.

Ah, sure! I didn't recognize your requirement properly!
I'm going to fix the code.

Some changes missing here? I think the idea was to use the new certPool type.

If function ValidateAndSplitPEMBundle returns certPool type instead of [][]byte then some tests will fail, due to their dependencies on that function' result, so I leave it as is.
However, I decided to move operation with certPool to ValidateAndSanitizePEMBundle function.
Could you please review it?

Why can't you just return certPool.getCertsPEM() from ValidateAndSplitPEMBundle? Am I missing something? 🤔

Oh, in that case we can! I thought you wanted to return certPool

Why can't you just return certPool.getCertsPEM() from ValidateAndSplitPEMBundle? Am I missing something? 🤔

I fixed a code as you required, all tests were passed.
Could you please review?

erikgb

This looks good, thanks for addressing the review comments. I have just a question about some commented code.

/ok-to-test

erikgb · 2023-09-26T22:03:46Z

pkg/util/cert_pool.go

+			// // p is nil and the whole of the input is returned in rest.
+			// if len(PEMdata) != 0 {
+			// 	return fmt.Errorf("error of decoding PEM block")
+			// }


Is the out-commented code intentional? I am not very familiar with this PEM decoding, but if this code is not worth having, I think we should delete it.

Is the out-commented code intentional? I am not very familiar with this PEM decoding, but if this code is not worth having, I think we should delete it.

Yes of course, I will fix it.
Thank you so much for your help, hints and patience, it is very appreciated!

Is the out-commented code intentional? I am not very familiar with this PEM decoding, but if this code is not worth having, I think we should delete it.

Hello,
Comments were trimmed as you requested.

erikgb · 2023-09-26T22:16:55Z

/retest

erikgb · 2023-09-26T22:37:02Z

/retest

erikgb

/lgtm
/cc @SgtCoDFish

Thanks @arsenalzp! With this new certPool type I think this can be improved even further in a follow-up PR.

Maybe you can rebase and squash commits?

Signed-off-by: Oleksandr Krutko <[email protected]> add certPool structure and add related functions Signed-off-by: Oleksandr Krutko <[email protected]> add header to comply with code format Signed-off-by: Oleksandr Krutko <[email protected]> substitute IsCertificateDuplicate func to CertPool method Signed-off-by: Oleksandr Krutko <[email protected]> convert CertPool and its mothods to unexported ones Signed-off-by: Oleksandr Krutko <[email protected]> move certPool functionality to ValidateAndSplitPEMBundle func Signed-off-by: Oleksandr Krutko <[email protected]> Update cert_pool.go Trim comments in the certPool code. Signed-off-by: Oleksandr Krutko <[email protected]>

arsenalzp · 2023-09-28T13:15:54Z

/lgtm /cc @SgtCoDFish

Thanks @arsenalzp! With this new certPool type I think this can be improved even further in a follow-up PR.

Maybe you can rebase and squash commits?

It was done.

erikgb · 2023-09-28T13:43:42Z

/retest
/lgtm

SgtCoDFish

/lgtm
/approve

Thanks so much @arsenalzp (and @erikgb for reviewing)! 😁

jetstack-bot · 2023-09-28T17:35:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb, 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

arsenalzp · 2023-09-28T21:44:55Z

/lgtm /approve

Thanks so much @arsenalzp (and @erikgb for reviewing)! 😁

Thank you for your assistance!

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 Sep 23, 2023

jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 23, 2023

arsenalzp changed the title ~~add certificates deduplication feature~~ Add certificates deduplication feature Sep 23, 2023

erikgb reviewed Sep 23, 2023

View reviewed changes

erikgb reviewed Sep 25, 2023

View reviewed changes

erikgb reviewed Sep 26, 2023

View reviewed changes

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 Sep 26, 2023

erikgb mentioned this pull request Sep 27, 2023

Bump version to v0.7.0-alpha.1 #186

Merged

erikgb approved these changes Sep 28, 2023

View reviewed changes

jetstack-bot requested a review from SgtCoDFish September 28, 2023 12:22

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

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2023

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

SgtCoDFish approved these changes Sep 28, 2023

View reviewed changes

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

jetstack-bot merged commit 34b2c0f into cert-manager:main Sep 28, 2023

erikgb mentioned this pull request Sep 28, 2023

de-duplicate CA from the trust bundle #155

Closed

erikgb mentioned this pull request Oct 27, 2023

Add erikgb to project approvers #216

Merged

arsenalzp deleted the addCertsDedup branch November 6, 2023 18:00

erikgb mentioned this pull request Dec 5, 2023

New version of Bundle API #242

Open

5 tasks

Jiawei0227 mentioned this pull request Feb 20, 2024

trust-manager deduplication doesnt work #299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add certificates deduplication feature #184

Add certificates deduplication feature #184

arsenalzp commented Sep 23, 2023

jetstack-bot commented Sep 23, 2023

erikgb left a comment

erikgb Sep 23, 2023

SgtCoDFish Sep 25, 2023

arsenalzp Sep 25, 2023 •

edited

Loading

erikgb Sep 23, 2023 •

edited

Loading

SgtCoDFish Sep 25, 2023

arsenalzp Sep 25, 2023

arsenalzp Sep 25, 2023

erikgb Sep 25, 2023

erikgb Sep 25, 2023

arsenalzp Sep 25, 2023

arsenalzp Sep 25, 2023 •

edited

Loading

erikgb Sep 25, 2023

arsenalzp Sep 26, 2023

arsenalzp Sep 26, 2023

erikgb Sep 26, 2023 •

edited

Loading

arsenalzp Sep 26, 2023

arsenalzp Sep 26, 2023

erikgb left a comment

erikgb Sep 26, 2023

arsenalzp Sep 27, 2023

arsenalzp Sep 28, 2023

erikgb commented Sep 26, 2023

erikgb commented Sep 26, 2023

erikgb left a comment •

edited

Loading

arsenalzp commented Sep 28, 2023

erikgb commented Sep 28, 2023

SgtCoDFish left a comment

jetstack-bot commented Sep 28, 2023

arsenalzp commented Sep 28, 2023

Add certificates deduplication feature #184

Add certificates deduplication feature #184

Conversation

arsenalzp commented Sep 23, 2023

jetstack-bot commented Sep 23, 2023

erikgb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsenalzp Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

erikgb Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arsenalzp Sep 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgb Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgb commented Sep 26, 2023

erikgb commented Sep 26, 2023

erikgb left a comment • edited Loading

Choose a reason for hiding this comment

arsenalzp commented Sep 28, 2023

erikgb commented Sep 28, 2023

SgtCoDFish left a comment

Choose a reason for hiding this comment

jetstack-bot commented Sep 28, 2023

arsenalzp commented Sep 28, 2023

arsenalzp Sep 25, 2023 •

edited

Loading

erikgb Sep 23, 2023 •

edited

Loading

arsenalzp Sep 25, 2023 •

edited

Loading

erikgb Sep 26, 2023 •

edited

Loading

erikgb left a comment •

edited

Loading