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

[arsenalzp]

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]

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]

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

[arsenalzp]

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]

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.

[arsenalzp]

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.

[arsenalzp]

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

[erikgb]

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]

Existing tests were modified

[erikgb]

/ok-to-test

[arsenalzp]

It seems the issue on my side with gomarkdoc.

[inteon]

It seems the issue on my side with gomarkdoc.

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

[arsenalzp]

/retest

[arsenalzp]

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]

@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

[erikgb]

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

[arsenalzp]

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

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

[arsenalzp]

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

/reopen

[cert-manager-prow]

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign inteon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign inteon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[cert-manager-prow]

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

/test all

[erikgb]

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

[arsenalzp]

It seems all remarks have already been resolved...

[erikgb]

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]

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]

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

[erikgb]

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?