Fix issue with updating of JKS/PKCS targets when password changes #449

arsenalzp · 2024-10-06T21:05:52Z

This draft PR is trying to fix #433

As was advised be @erikgb I added additional field named passwordHash.
However, as user shouldn't care of calculating password hash, I added mutation webhook to set passwordHash value during Bundle creation.

Let's consider this PR as draft one, therefore I would like to discuss my solution with you, then I'm gonna modify tests, prepare documentation for functions which were added.

cert-manager-prow · 2024-10-06T21:06:02Z

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-10-06T21:15:10Z

But the password hash needs to be added to the targets, and not the bundle resource. 😊

arsenalzp · 2024-10-06T21:37:07Z

But the password hash needs to be added to the targets, and not the bundle resource. 😊

In our case it is configMap which holds additional binary data for jks/pkcs12 formats.
Do you want to add annotations with hashes for each target formats?
I guess it is possible.

arsenalzp · 2024-10-07T21:19:28Z

I made changes as was discussed. However, it was made for ConfigMap target only. Once all changes agreed then I will make changes for Secret target and tests as well.

erikgb

Thanks for looking into this @arsenalzp! First review pass done. Before this can be merged we need tests for this and a similar implementation for target secrets.

pkg/bundle/internal/target/target.go

arsenalzp · 2024-10-12T13:02:00Z

All remarks have been removed.

erikgb

IMO, this is moving in the right direction now. I would still love to see this new stuff better aligned/integrated with the existing data hash. I also think it's time to write some tests - before we finally implement this for all target resource types.

pkg/bundle/internal/target/target.go

pkg/apis/trust/v1alpha1/types_bundle.go

arsenalzp · 2024-10-15T19:45:08Z

Were my changes accepted? Can I proceed with the hardest step - test?

erikgb · 2024-10-15T19:56:42Z

Were my changes accepted? Can I proceed with the hardest step - test?

Looking good to me, @arsenalzp! Well done and thanks! Please add some testing of this! 😺

arsenalzp · 2024-10-16T07:02:59Z

Existing tests were modified

erikgb · 2024-10-16T08:02:13Z

/ok-to-test

arsenalzp · 2024-10-18T12:26:11Z

It seems the issue on my side with gomarkdoc.

inteon · 2024-10-18T12:39:41Z

It seems the issue on my side with gomarkdoc.

If you run make generate, does that solve the issue?

arsenalzp · 2024-10-18T18:26:14Z

/retest

arsenalzp · 2024-10-18T18:45:13Z

pkg/apis/trust/v1alpha1/types_bundle.go:28:2: G101: Potential hardcoded credentials (gosec)
	BundleJksPasswdHashAnnotation    = "trust.cert-manager.io/jks-pwd-hash"
	^
pkg/apis/trust/v1alpha1/types_bundle.go:29:2: G101: Potential hardcoded credentials (gosec)
	BundlePkcs12PasswdHashAnnotation = "trust.cert-manager.io/pksc12-pwd-hash"

It is necessary to exclude these constants, as they have the same conditions like constant BundleHashAnnotationKey has.

erikgb · 2024-10-18T18:45:17Z

@arsenalzp I think this is a false positive that you have to suppress to pass gosec linting.

pkg/apis/trust/v1alpha1/types_bundle.go:28:2: G101: Potential hardcoded credentials (gosec)
	BundleJksPasswdHashAnnotation    = "trust.cert-manager.io/jks-pwd-hash"
	^
pkg/apis/trust/v1alpha1/types_bundle.go:29:2: G101: Potential hardcoded credentials (gosec)
	BundlePkcs12PasswdHashAnnotation = "trust.cert-manager.io/pksc12-pwd-hash"
	^
make[1]: *** [make/_shared/go/01_mod.mk:112: verify-golangci-lint] Error 1

docs/api/api.md

erikgb · 2024-10-18T19:24:18Z

@arsenalzp, please run make generate to fix the linting, as the error output suggests.

arsenalzp · 2024-10-18T19:28:42Z

@arsenalzp, please run make generate to fix the linting, as the error output suggests.

I did it, it is replacing #nosec stanzas:

$ diff --git a/docs/api/api.md b/docs/api/api.md
index ee6ebf9..6c09868 100644
--- a/docs/api/api.md
+++ b/docs/api/api.md
@@ -66,8 +66,8 @@ const (
 
     BundleLabelKey                   = "trust.cert-manager.io/bundle"
     BundleHashAnnotationKey          = "trust.cert-manager.io/hash"
-    BundleJksPasswdHashAnnotation    = "trust.cert-manager.io/jks-pwd-hash" # nosec G101
-    BundlePkcs12PasswdHashAnnotation = "trust.cert-manager.io/pksc12-pwd-hash" # nosec G101
+    BundleJksPasswdHashAnnotation    = "trust.cert-manager.io/jks-pwd-hash"
+    BundlePkcs12PasswdHashAnnotation = "trust.cert-manager.io/pksc12-pwd-hash"
 )
$

erikgb · 2024-10-29T20:23:43Z

@arsenalzp Are you giving up? 🤠 There should be no need for a new PR. Just fix the branch. 😺

arsenalzp · 2024-10-29T20:30:52Z

@arsenalzp Are you giving up? 🤠 There should be no need for a new PR. Just fix the branch. 😺

No, it is not so easy to get rid of me :-D

erikgb · 2024-10-29T20:34:12Z

/reopen

cert-manager-prow · 2024-10-29T20:34:16Z

@erikgb: Failed to re-open PR: state cannot be changed. There are no new commits on the arsenalzp:issue_433 branch.

In response to this:

/reopen

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.

cert-manager-prow · 2024-10-29T20:49:35Z

@arsenalzp: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test pull-trust-manager-integration
/test pull-trust-manager-smoke
/test pull-trust-manager-test
/test pull-trust-manager-verify

Use /test all to run all jobs.

In response to this:

/test

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.

arsenalzp · 2024-10-29T20:50:05Z

/test all

erikgb · 2024-10-29T21:00:47Z

@arsenalzp, please ping me for another review when you have addressed all the present remarks!

arsenalzp · 2024-10-29T21:06:20Z

It seems all remarks have already been resolved...

erikgb · 2024-10-29T21:37:36Z

It seems all remarks have already been resolved...

I still can't find any test of the issue this PR is solving. I think it will fix it, but it would be nice with a test verifying it's fixed.

arsenalzp · 2024-10-31T13:28:26Z

It seems all remarks have already been resolved...

I still can't find any test of the issue this PR is solving. I think it will fix it, but it would be nice with a test verifying it's fixed.

I agree.
As temporary solution it is possible to use the Bundle tests which were modified by me to reflect changes I made into the code. There is a comparison of annotations so it is possible to asses result indirectly by using hash annotations.

arsenalzp · 2024-11-07T21:32:11Z

Hello colleagues,
Do you have any feedback on this solution?

erikgb · 2024-11-09T10:27:59Z

Hello colleagues, Do you have any feedback on this solution?

@arsenalzp The fix looks good, but I would still love to see a test verifying that the reported issue is fixed. However I tend to accept your proposed changes and leave the test improvement for a follow-up PR - since this PR appears to fix an annoying bug and has been open for a while. 🤠 But can you please fix the commit message on your single commit?

Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> fix typo in variable name, regenerate API doc Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com> fix bundle test with annotation zero strings Signed-off-by: Oleksandr Krutko <alexander.krutko@gmail.com>

arsenalzp · 2024-11-17T13:21:12Z

Fixed.

erikgb

/lgtm
/approve
/hold

Looks good to me, but since @inteon had some comments on this PR I am putting a hold for now. Tim, could you please take a look? @arsenalzp has been working on this for a long time now.

I am going to prepare a PR for deduplicating the target logic more after this is merged.

cert-manager-prow · 2024-11-17T14:17:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb

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]

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

arsenalzp · 2024-11-19T11:37:00Z

/lgtm /approve /hold

Looks good to me, but since @inteon had some comments on this PR I am putting a hold for now. Tim, could you please take a look? @arsenalzp has been working on this for a long time now.

I am going to prepare a PR for deduplicating the target logic more after this is merged.

What happened with deduplication? Those changes were introduced by me last year.

erikgb · 2024-11-19T12:20:58Z

/lgtm /approve /hold
Looks good to me, but since @inteon had some comments on this PR I am putting a hold for now. Tim, could you please take a look? @arsenalzp has been working on this for a long time now.
I am going to prepare a PR for deduplicating the target logic more after this is merged.

What happened with deduplication? Those changes were introduced by me last year.

I meant deduplication of source code. Not certs in trust bundle. 😉 You fixed the latter, yes.

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 Oct 6, 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 Oct 6, 2024

cert-manager-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 7, 2024

erikgb requested changes Oct 8, 2024

View reviewed changes

erikgb requested changes Oct 12, 2024

View reviewed changes

pkg/bundle/internal/target/target.go Show resolved Hide resolved

pkg/bundle/internal/target/target.go Outdated Show resolved Hide resolved

pkg/bundle/internal/target/target.go Outdated Show resolved Hide resolved

erikgb reviewed Oct 12, 2024

View reviewed changes

pkg/bundle/internal/target/target.go Show resolved Hide resolved

erikgb reviewed Oct 12, 2024

View reviewed changes

pkg/apis/trust/v1alpha1/types_bundle.go Outdated Show resolved Hide resolved

cert-manager-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 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 Oct 16, 2024

erikgb reviewed Oct 18, 2024

View reviewed changes

docs/api/api.md Outdated Show resolved Hide resolved

docs/api/api.md Outdated Show resolved Hide resolved

cert-manager-prow bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 29, 2024

arsenalzp reopened this Oct 29, 2024

cert-manager-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 29, 2024

arsenalzp force-pushed the issue_433 branch from af84d14 to f53fd16 Compare October 29, 2024 20:48

arsenalzp force-pushed the issue_433 branch from f53fd16 to eeed5e1 Compare November 17, 2024 13:19

erikgb reviewed Nov 17, 2024

View reviewed changes

cert-manager-prow 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 Nov 17, 2024

cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 17, 2024

erikgb requested a review from inteon November 17, 2024 14:17

inteon mentioned this pull request Nov 19, 2024

Fix issue with updating of JKS/PKCS targets when password changes #449 (review) #480

Open

Fix issue with updating of JKS/PKCS targets when password changes #449

Are you sure you want to change the base?

Fix issue with updating of JKS/PKCS targets when password changes #449

Conversation

arsenalzp commented Oct 6, 2024 • edited Loading

cert-manager-prow bot commented Oct 6, 2024

erikgb commented Oct 6, 2024

arsenalzp commented Oct 6, 2024

arsenalzp commented Oct 7, 2024 • edited Loading

erikgb left a comment

Choose a reason for hiding this comment

arsenalzp commented Oct 12, 2024

erikgb left a comment • edited Loading

Choose a reason for hiding this comment

arsenalzp commented Oct 15, 2024

erikgb commented Oct 15, 2024

arsenalzp commented Oct 16, 2024

erikgb commented Oct 16, 2024

arsenalzp commented Oct 18, 2024

inteon commented Oct 18, 2024

arsenalzp commented Oct 18, 2024

arsenalzp commented Oct 18, 2024 • edited Loading

erikgb commented Oct 18, 2024

erikgb commented Oct 18, 2024 • edited Loading

arsenalzp commented Oct 18, 2024 • edited Loading

erikgb commented Oct 29, 2024

arsenalzp commented Oct 29, 2024

erikgb commented Oct 29, 2024

cert-manager-prow bot commented Oct 29, 2024

cert-manager-prow bot commented Oct 29, 2024

arsenalzp commented Oct 29, 2024

erikgb commented Oct 29, 2024

arsenalzp commented Oct 29, 2024

erikgb commented Oct 29, 2024

arsenalzp commented Oct 31, 2024

arsenalzp commented Nov 7, 2024 • edited Loading

erikgb commented Nov 9, 2024

arsenalzp commented Nov 17, 2024

erikgb left a comment

Choose a reason for hiding this comment

cert-manager-prow bot commented Nov 17, 2024

arsenalzp commented Nov 19, 2024

erikgb commented Nov 19, 2024

arsenalzp commented Oct 6, 2024 •

edited

Loading

arsenalzp commented Oct 7, 2024 •

edited

Loading

erikgb left a comment •

edited

Loading

arsenalzp commented Oct 18, 2024 •

edited

Loading

erikgb commented Oct 18, 2024 •

edited

Loading

arsenalzp commented Oct 18, 2024 •

edited

Loading

arsenalzp commented Nov 7, 2024 •

edited

Loading