Introduce certificate pool structure and remove multiple encode/decode process #375

arsenalzp · 2024-06-30T22:02:38Z

This PR introduce certificate pool structure which is mainly used to hold Certificates.
I want to decouple PEM operations from Certificates pool ones.
Deduplication option for certificate pool was also introduced - deduplication process can be optionally disabled.
It is related to #305

cert-manager-prow · 2024-06-30T22:02:48Z

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-sigs/prow repository.

erikgb · 2024-07-01T05:31:38Z

/ok-to-test

arsenalzp · 2024-07-01T07:18:47Z

Let me fix typos!

erikgb

Thanks for working on this @arsenalzp! I did a first review pass. Please take a look!

pkg/bundle/source.go

pkg/bundle/bundle.go

pkg/util/cert_pool.go

erikgb · 2024-07-02T10:43:22Z

pkg/util/pem.go

@@ -121,3 +104,29 @@ func DecodeX509CertificateChainBytes(certBytes []byte) ([]*x509.Certificate, err

 	return certs, nil
 }
+


I think the following functions should have CertPool as receiver. Why not?

Also, we could rename the functions as follows:

GetSplitPEMBundle -> AsSplitPEMBundle

GetSplitPEMBundleBytes -> AsPEMBundleBytes

GetSplitPEMBundleStrings-> AsPEMBundleStrings

GetCertsList -> AsCertificateList

Functions were renamed.
Regarding CertPool as argument for DecodeX509CertificateChainBytes().
Let me explain: an idea here is PEM is responsible for providing prepared data for consumers in form that is needed by consumers, consumers by themself shouldn't know how, where data was taken from, CertPool is responsible for store, manage, filter, de-duplicate, etc of Certificates, it provides data for PEM, so end consumers shouldn't know how CertPool works inside (encapsulation).
Imagine, we want to provide DER encoded data instead of PEM one.
Moreover, I'm thinking about making CertPool abstract structure (interface).

Regarding CertPool as argument for DecodeX509CertificateChainBytes()

I think Erik was referring to the functions below (AsSplitPEMBundle, AsPEMBundleBytes, AsCertificateList, GetCertificatesQuantity) rather than DecodeX509CertificateChainBytes.

An idea here is PEM is responsible for providing prepared data for consumers in form that is needed by consumers, consumers by themself shouldn't know how, where data was taken from, CertPool is responsible for store, manage, filter, de-duplicate, etc of Certificates, it provides data for PEM, so end consumers shouldn't know how CertPool works inside (encapsulation).

I'm not sure what PEM refers to. Is it a type that will be created in the future and that, for now, exists through the existence of the AsPEM functions?

Imagine, we want to provide DER encoded data instead of PEM one.

I imagine that CertPool would just need to have a couple of receiver functions:

func (*CertPool) AsDER() [][]byte func (*CertPool) AsPEMBundle() []byte func (*CertPool) AsPEM() [][]byte func (*CertPool) Count() int

Why is adding receiver functions a bad idea? I don't see the benefits of encapsulating the "export" (DER or PEM) logic.

Moreover, I'm thinking about making CertPool abstract structure (interface).

What would be the benefits of turning this into an interface? Would that be for testing purposes?

pkg/bundle/source.go

pkg/util/pem.go

pkg/util/cert_pool.go

erikgb

I think we are approaching the finish line here, but there are still some "style" comments I would like to see fixed. :-)

pkg/bundle/source.go

pkg/util/cert_pool.go

pkg/util/cert_pool_test.go

erikgb · 2024-07-04T18:55:14Z

pkg/util/pem.go

+
+func GetCertificatesQuantity(certPool *CertPool) int {
+	return certPool.size()
+}


Why can't we just export the size function and delete this function?

It would be easy to calculate a size for other kind of storages in the future. As was stated earlier it was done because I wanted to encapsulate the insides of Certificates pool.

Ok, I don't understand your argument, but this can stay - it's "just" a name. 😉 If you still want to alias the Size function I think GetCertificateCount could be a better name.

I wanted to encapsulate the insides of Certificates pool.

Thanks for the clarification.

Can you elaborate on the need for encapsulating CertPool's behavior, and why the count of certificate in the certificate pool shouldn't be exposed?

I don't think that the behaviors "counting certificates", "exporting certificates to a PEM bundle", and "exporting certificates to a slide of DER bytes" need to be encapsulated away. More generally, I'm not convinced that encapsulation will solve anything in this context. I'd love to know more, though!

Hello colleagues,
Behavior of CertPool should be hidden from its consumers. For example we are going to substitute current CertPool by one from x509 package or even third-party. As little possibilities to works with CertPool directly are given to consumers, as little components coupling we have.
From my opinion CertPool acts like io.Writer or io.Reader.

Behavior of CertPool should be hidden from its consumers. For example we are going to substitute current CertPool by one from x509 package or even third-party. As little possibilities to works with CertPool directly are given to consumers, as little components coupling we have. From my opinion CertPool acts like io.Writer or io.Reader.

This does not make sense to me. Creating util-wrapper functions does not change your stated concern. It just looks very strange to me to have dummy single-statement functions. Making the CertPool type internal probably makes sense - to allow ourselves more freedom to modify it over time.

test/env/data.go

erikgb · 2024-07-08T11:52:29Z

@arsenalzp I have a big refactoring incoming: #378. It would be nice to get this PR merged soon. 😉

arsenalzp · 2024-07-08T12:17:09Z

@arsenalzp I have a big refactoring incoming: #378. It would be nice to get this PR merged soon. 😉

Hello,
Can it wait until this evening?

erikgb · 2024-07-08T12:26:21Z

Hello, Can it wait until this evening?

Sure, there is no urgency. I just have some extra time for open source work this week.

arsenalzp · 2024-07-08T22:00:12Z

Hello, Can it wait until this evening?

Sure, there is no urgency. I just have some extra time for open source work this week.

Done

pkg/bundle/source_test.go

pkg/bundle/source.go

erikgb · 2024-07-08T23:52:52Z

pkg/bundle/source.go

@@ -99,27 +98,19 @@ func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.Bundl
 			return bundleData{}, fmt.Errorf("failed to retrieve bundle from source: %w", err)
 		}

-		opts := util.ValidateAndSanitizeOptions{FilterExpired: b.Options.FilterExpiredCerts}
-		sanitizedBundle, err := util.ValidateAndSanitizePEMBundleWithOptions([]byte(sourceData), opts)
+		err = util.ValidateAndSplitPEMBundle(certPool, []byte(sourceData))


I find the function name a bit strange when used in this context. It is not evident, at least not to me, that the certs from sourceData are appended to certPool.

Yes, I agree with you, we can improve naming. Which suggestions ?

Suggested change

err = util.ValidateAndSplitPEMBundle(certPool, []byte(sourceData))

err = certPool.AppendCertsFromPEM([]byte(sourceData))

Similar to https://pkg.go.dev/crypto/x509#CertPool.AppendCertsFromPEM.

In general, let us use functions with CertPool as receiver, and avoid the dummy "utils" wrapper functions. As noted many times already in reviews of this PR. 😸

pkg/fspkg/package.go

pkg/util/cert_pool.go

pkg/util/pem_test.go

test/env/data.go

erikgb · 2024-07-12T11:25:02Z

pkg/bundle/source.go

@@ -99,27 +98,19 @@ func (b *bundle) buildSourceBundle(ctx context.Context, sources []trustapi.Bundl
 			return bundleData{}, fmt.Errorf("failed to retrieve bundle from source: %w", err)
 		}

-		opts := util.ValidateAndSanitizeOptions{FilterExpired: b.Options.FilterExpiredCerts}
-		sanitizedBundle, err := util.ValidateAndSanitizePEMBundleWithOptions([]byte(sourceData), opts)
+		err = util.ValidateAndSplitPEMBundle(certPool, []byte(sourceData))


Suggested change

err = util.ValidateAndSplitPEMBundle(certPool, []byte(sourceData))

err = certPool.AppendCertsFromPEM([]byte(sourceData))

Similar to https://pkg.go.dev/crypto/x509#CertPool.AppendCertsFromPEM.

In general, let us use functions with CertPool as receiver, and avoid the dummy "utils" wrapper functions. As noted many times already in reviews of this PR. 😸

erikgb · 2024-07-12T11:28:49Z

pkg/util/pem.go

+
+func GetCertificatesQuantity(certPool *CertPool) int {
+	return certPool.size()
+}


Behavior of CertPool should be hidden from its consumers. For example we are going to substitute current CertPool by one from x509 package or even third-party. As little possibilities to works with CertPool directly are given to consumers, as little components coupling we have. From my opinion CertPool acts like io.Writer or io.Reader.

This does not make sense to me. Creating util-wrapper functions does not change your stated concern. It just looks very strange to me to have dummy single-statement functions. Making the CertPool type internal probably makes sense - to allow ourselves more freedom to modify it over time.

arsenalzp · 2024-07-12T12:48:12Z

I didn't want to change code drastically.
OK, I will re-write it with receivers :)

erikgb · 2024-07-16T18:47:16Z

@arsenalzp Do you have the energy to rebase and resolve the current conflicts? Sorry, but we had to get the serious issue merged and released. 😸

inteon · 2024-07-19T12:48:19Z

@erikgb @arsenalzp I rebased the PR and applied the code suggestions.

erikgb

Only minor nits/suggestions left. Almost GTM.

pkg/bundle/source.go

pkg/fspkg/package.go

pkg/util/pem_test.go

erikgb

/lgtm

Thanks for finishing this @inteon! 🚀

…ng done. Signed-off-by: Oleksandr Krutko <[email protected]>

inteon · 2024-07-19T14:55:57Z

/approve

cert-manager-prow · 2024-07-19T14:56:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb, inteon

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~~ [erikgb,inteon]

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

erikgb

/lgtm

cert-manager-prow bot added the dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. label Jun 30, 2024

cert-manager-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 30, 2024

cert-manager-prow 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 Jul 1, 2024

arsenalzp force-pushed the multipleDecode branch from 8f9488a to 1596064 Compare July 1, 2024 19:28

erikgb requested changes Jul 2, 2024

View reviewed changes

inteon changed the title ~~Introduce certitiface pool structure and remove multiple encode/decode process~~ Introduce certificate pool structure and remove multiple encode/decode process Jul 3, 2024

arsenalzp force-pushed the multipleDecode branch 4 times, most recently from aac3ec6 to a200310 Compare July 3, 2024 21:10

erikgb reviewed Jul 4, 2024

View reviewed changes

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

pkg/util/pem.go Outdated Show resolved Hide resolved

pkg/util/cert_pool.go Outdated Show resolved Hide resolved

arsenalzp force-pushed the multipleDecode branch from a200310 to 84f288e Compare July 4, 2024 18:42

erikgb requested changes Jul 4, 2024

View reviewed changes

cert-manager-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024

arsenalzp force-pushed the multipleDecode branch 2 times, most recently from e0e7df7 to 4cb0887 Compare July 8, 2024 21:59

cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024

erikgb reviewed Jul 9, 2024

View reviewed changes

arsenalzp force-pushed the multipleDecode branch 2 times, most recently from 06316db to 5cc56c4 Compare July 10, 2024 20:57

erikgb requested changes Jul 12, 2024

View reviewed changes

erikgb mentioned this pull request Jul 12, 2024

Sort certificates in bundles to ensure deterministic behaviour #380

Merged

cert-manager-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2024

inteon force-pushed the multipleDecode branch from 5cc56c4 to b9216d0 Compare July 19, 2024 12:41

cert-manager-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2024

inteon force-pushed the multipleDecode branch from b9216d0 to ad09933 Compare July 19, 2024 12:47

cert-manager-prow bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2024

inteon force-pushed the multipleDecode branch from ad09933 to 2a5a6a7 Compare July 19, 2024 12:54

erikgb requested changes Jul 19, 2024

View reviewed changes

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

pkg/fspkg/package.go Outdated Show resolved Hide resolved

pkg/util/pem_test.go Outdated Show resolved Hide resolved

pkg/util/pem_test.go Show resolved Hide resolved

inteon force-pushed the multipleDecode branch from 2a5a6a7 to 8bc2305 Compare July 19, 2024 14:35

erikgb approved these changes Jul 19, 2024

View reviewed changes

cert-manager-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 19, 2024

Refactor: use CertPool helper to limit the amount of encoding/ decodi…

1bbbe3b

…ng done. Signed-off-by: Oleksandr Krutko <[email protected]>

inteon force-pushed the multipleDecode branch from 8bc2305 to 1bbbe3b Compare July 19, 2024 14:50

cert-manager-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2024

erikgb reviewed Jul 19, 2024

View reviewed changes

cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2024

cert-manager-prow bot merged commit 51f1e51 into cert-manager:main Jul 19, 2024
6 checks passed

erikgb mentioned this pull request Jul 19, 2024

Avoid multiple decode/encode of certificates #305

Closed

erikgb mentioned this pull request Nov 23, 2024

Improve filtered certs error reporting #276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce certificate pool structure and remove multiple encode/decode process #375

Introduce certificate pool structure and remove multiple encode/decode process #375

arsenalzp commented Jun 30, 2024 •

edited

Loading

cert-manager-prow bot commented Jun 30, 2024

erikgb commented Jul 1, 2024

arsenalzp commented Jul 1, 2024

erikgb left a comment

erikgb Jul 2, 2024

inteon Jul 2, 2024

arsenalzp Jul 3, 2024 •

edited

Loading

maelvls Jul 9, 2024 •

edited

Loading

erikgb left a comment

erikgb Jul 4, 2024

arsenalzp Jul 8, 2024

erikgb Jul 9, 2024

maelvls Jul 9, 2024 •

edited

Loading

arsenalzp Jul 10, 2024 •

edited

Loading

erikgb Jul 12, 2024

erikgb commented Jul 8, 2024

arsenalzp commented Jul 8, 2024

erikgb commented Jul 8, 2024

arsenalzp commented Jul 8, 2024

erikgb Jul 8, 2024

arsenalzp Jul 10, 2024

erikgb Jul 12, 2024

erikgb Jul 12, 2024

erikgb Jul 12, 2024

arsenalzp commented Jul 12, 2024 •

edited

Loading

erikgb commented Jul 16, 2024

inteon commented Jul 19, 2024

erikgb left a comment

erikgb left a comment

inteon commented Jul 19, 2024

cert-manager-prow bot commented Jul 19, 2024

erikgb left a comment

		@@ -121,3 +104,29 @@ func DecodeX509CertificateChainBytes(certBytes []byte) ([]*x509.Certificate, err

		return certs, nil
		}

	err = util.ValidateAndSplitPEMBundle(certPool, []byte(sourceData))
	err = certPool.AppendCertsFromPEM([]byte(sourceData))

Introduce certificate pool structure and remove multiple encode/decode process #375

Introduce certificate pool structure and remove multiple encode/decode process #375

Conversation

arsenalzp commented Jun 30, 2024 • edited Loading

cert-manager-prow bot commented Jun 30, 2024

erikgb commented Jul 1, 2024

arsenalzp commented Jul 1, 2024

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 Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

maelvls Jul 9, 2024 • edited Loading

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

maelvls Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

arsenalzp Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgb commented Jul 8, 2024

arsenalzp commented Jul 8, 2024

erikgb commented Jul 8, 2024

arsenalzp commented Jul 8, 2024

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 commented Jul 12, 2024 • edited Loading

erikgb commented Jul 16, 2024

inteon commented Jul 19, 2024

erikgb left a comment

Choose a reason for hiding this comment

erikgb left a comment

Choose a reason for hiding this comment

inteon commented Jul 19, 2024

cert-manager-prow bot commented Jul 19, 2024

erikgb left a comment

Choose a reason for hiding this comment

arsenalzp commented Jun 30, 2024 •

edited

Loading

arsenalzp Jul 3, 2024 •

edited

Loading

maelvls Jul 9, 2024 •

edited

Loading

maelvls Jul 9, 2024 •

edited

Loading

arsenalzp Jul 10, 2024 •

edited

Loading

arsenalzp commented Jul 12, 2024 •

edited

Loading