Add an option to filter expired certificates from bundle #273

Hoega · 2024-01-13T11:20:32Z

Closes #272

Add an option to skip expired certificates and exclude them from the bundle.

The option is defined as a cli parameter to be passed by the helm charts values:

filterExpiredCertificates:
  # -- Whether to filter expired certificates from the trust bundle.
  enabled: false

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

erikgb · 2024-01-16T10:40:10Z

/ok-to-test

erikgb · 2024-01-16T10:46:15Z

/test pull-trust-manager-verify

erikgb · 2024-01-16T11:17:59Z

@Hoega Thanks a lot for this PR. I will try to review it soon!

Please run 'make -s update-helm-docs'

to fix the CI.

SgtCoDFish

This looks really great - thank you for raising it! I've got a couple of comments - what do you think?

SgtCoDFish · 2024-01-16T12:05:43Z

pkg/util/cert_pool.go

 // Append certificate to a pool
-func (cp *certPool) appendCertFromPEM(PEMdata []byte) error {
+func (cp *certPool) appendCertFromPEM(PEMdata []byte, filterExpiredCerts bool) error {


suggestion: I can't imagine a situation where we'd need to configure filterExpiredCerts on a per-certificate basis like this.

Do you think it makes sense to add filterExpiredCerts as a boolean option on certPool, so we'd have newCertPool(filterExpiredCerts bool) *certPool instead and appendCertFromPEM would keep its current signature?

You are right, that would be better, I will make the change later. Thanks!

I have created new functions with a certPool receiver to preserve the signature of the previous function. I am not an expert in Go, so please let me know if you think of a better approach.

SgtCoDFish · 2024-01-16T12:24:43Z

pkg/util/pem.go

+func ValidateAndSanitizePEMBundle(data []byte, filterExpiredCerts bool) ([]byte, error) {
+	certificates, err := ValidateAndSplitPEMBundle(data, filterExpiredCerts)


suggestion: I'm uneasy about making a breaking change to an exported function here (and below with ValidateAndSplitPEMBundle) - I don't think we need to.

Can we change this so it's not breaking?

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

erikgb · 2024-01-16T14:07:25Z

/test pull-trust-manager-verify

deploy/charts/trust-manager/values.yaml

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

Co-authored-by: Erik Godding Boye <egboye@gmail.com> Signed-off-by: Hoega <45895770+Hoega@users.noreply.github.com>

pkg/util/cert_pool.go

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

SgtCoDFish

/lgtm
/approve

This looks great - really appreciate you raising this and getting involved in the community ❤️

I'll try to get around to releasing a new trust-manager version soon which will include this PR 😁

SgtCoDFish · 2024-01-17T16:27:50Z

pkg/util/pem_test.go

+		"valid bundle with valid certs and an expired cert": {
+			parts:          []string{dummy.TestCertificate1, dummy.TestExpiredCertificate, dummy.TestCertificate3},
+			expectedOutput: []string{dummy.TestCertificate1, dummy.TestCertificate3},
+			expectErr:      false,
+		},


comment: I think it'd be good to have a test that the expired cert is not removed if FilterExpired is false, but I don't think it should block this PR: I'll raise a followup PR to add that after this merges :)

Oh wait, the cases above test that - you already have it tested 😁 Thank you!

jetstack-bot · 2024-01-17T16:29:11Z

[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

Test tweaks following #273

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2024

Hoega force-pushed the feature/filter-expired-certificate branch from 876a408 to 8304f88 Compare January 15, 2024 10:04

Hoega force-pushed the feature/filter-expired-certificate branch from 45a5080 to 64c8cfb Compare January 15, 2024 11:49

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jan 15, 2024

Hoega marked this pull request as ready for review January 15, 2024 11:49

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 15, 2024

Add an option to filter expired certificates from bundle

847ed9c

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

Hoega force-pushed the feature/filter-expired-certificate branch from 64c8cfb to 847ed9c Compare January 15, 2024 11:52

jetstack-bot added the ok-to-test label Jan 16, 2024

SgtCoDFish reviewed Jan 16, 2024

View reviewed changes

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. and removed dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Jan 16, 2024

Hoega added 2 commits January 16, 2024 15:04

Update helm docs

5401436

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

Preserve util functions signature

a22f23f

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

Hoega force-pushed the feature/filter-expired-certificate branch from 9e2eac4 to a22f23f Compare January 16, 2024 14:04

jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Jan 16, 2024

erikgb reviewed Jan 16, 2024

View reviewed changes

deploy/charts/trust-manager/values.yaml Outdated Show resolved Hide resolved

Reduce duplicated code in util pkg

f103e8e

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

Remove newline at the end of values.yaml

2951db3

Co-authored-by: Erik Godding Boye <egboye@gmail.com> Signed-off-by: Hoega <45895770+Hoega@users.noreply.github.com>

Hoega force-pushed the feature/filter-expired-certificate branch from d04ab92 to 2951db3 Compare January 16, 2024 14:27

SgtCoDFish reviewed Jan 16, 2024

View reviewed changes

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

Implement options for pem functions

768009d

Signed-off-by: Hoega <rouxelflorent@yahoo.fr>

SgtCoDFish approved these changes Jan 17, 2024

View reviewed changes

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

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

jetstack-bot merged commit 83f1a23 into cert-manager:main Jan 17, 2024
3 of 4 checks passed

Hoega deleted the feature/filter-expired-certificate branch January 17, 2024 17:30

SgtCoDFish added a commit that referenced this pull request Jan 18, 2024

Merge pull request #275 from SgtCoDFish/validate-pem-tests

2180c78

Test tweaks following #273

erikgb mentioned this pull request Jan 21, 2024

Improve filtered certs error reporting #276

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to filter expired certificates from bundle #273

Add an option to filter expired certificates from bundle #273

Hoega commented Jan 13, 2024 •

edited

erikgb commented Jan 16, 2024

erikgb commented Jan 16, 2024

erikgb commented Jan 16, 2024

SgtCoDFish left a comment

SgtCoDFish Jan 16, 2024

Hoega Jan 16, 2024

Hoega Jan 16, 2024

SgtCoDFish Jan 16, 2024

erikgb commented Jan 16, 2024

SgtCoDFish left a comment

SgtCoDFish Jan 17, 2024

SgtCoDFish Jan 17, 2024

jetstack-bot commented Jan 17, 2024

		func ValidateAndSanitizePEMBundle(data []byte, filterExpiredCerts bool) ([]byte, error) {
		certificates, err := ValidateAndSplitPEMBundle(data, filterExpiredCerts)

Add an option to filter expired certificates from bundle #273

Add an option to filter expired certificates from bundle #273

Conversation

Hoega commented Jan 13, 2024 • edited

erikgb commented Jan 16, 2024

erikgb commented Jan 16, 2024

erikgb commented Jan 16, 2024

SgtCoDFish left a comment

Choose a reason for hiding this comment

SgtCoDFish Jan 16, 2024

Choose a reason for hiding this comment

Hoega Jan 16, 2024

Choose a reason for hiding this comment

Hoega Jan 16, 2024

Choose a reason for hiding this comment

SgtCoDFish Jan 16, 2024

Choose a reason for hiding this comment

erikgb commented Jan 16, 2024

SgtCoDFish left a comment

Choose a reason for hiding this comment

SgtCoDFish Jan 17, 2024

Choose a reason for hiding this comment

SgtCoDFish Jan 17, 2024

Choose a reason for hiding this comment

jetstack-bot commented Jan 17, 2024

Hoega commented Jan 13, 2024 •

edited