Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

wants to merge 1 commit into from
+128 −83

Conversation

arsenalzp
Copy link
Contributor

@arsenalzp arsenalzp commented Oct 6, 2024 •
edited
Loading

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 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
Copy link
Contributor

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.

@cert-manager-prow 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
@erikgb
Copy link
Contributor

erikgb commented Oct 6, 2024

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

@arsenalzp
Copy link
Contributor Author

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.

@cert-manager-prow 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
@arsenalzp
Copy link
Contributor Author

arsenalzp commented Oct 7, 2024 •
edited
Loading

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
erikgb requested changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated 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
pkg/bundle/internal/target/target.go Outdated Show resolved Hide resolved
pkg/bundle/internal/target/target.go Outdated Show resolved Hide resolved
@arsenalzp
Copy link
Contributor Author

All remarks have been removed.

erikgb
erikgb requested changes Oct 12, 2024
View reviewed changes
Copy link
Contributor

@erikgb erikgb left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
@arsenalzp
Copy link
Contributor Author

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

@erikgb
Copy link
Contributor

erikgb commented Oct 15, 2024

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! 😺

@cert-manager-prow 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
@arsenalzp
Copy link
Contributor Author

Existing tests were modified

@erikgb
Copy link
Contributor

erikgb commented Oct 16, 2024

/ok-to-test

@cert-manager-prow 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
@arsenalzp
Copy link
Contributor Author

It seems the issue on my side with gomarkdoc.

@inteon
Copy link
Member

inteon commented Oct 18, 2024

It seems the issue on my side with gomarkdoc.

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

@arsenalzp
Copy link
Contributor Author

/retest

@arsenalzp
Copy link
Contributor Author

arsenalzp commented Oct 18, 2024 •
edited
Loading 

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
Copy link
Contributor

erikgb commented Oct 18, 2024

@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
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
@erikgb
Copy link
Contributor

erikgb commented Oct 18, 2024 •
edited
Loading

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

@arsenalzp
Copy link
Contributor Author

arsenalzp commented Oct 18, 2024 •
edited
Loading

@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"
 )
$

@arsenalzp arsenalzp closed this Oct 29, 2024
@cert-manager-prow cert-manager-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 29, 2024
@erikgb
Copy link
Contributor

erikgb commented Oct 29, 2024

image

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

@arsenalzp
Copy link
Contributor Author

image

@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
Copy link
Contributor

erikgb commented Oct 29, 2024

/reopen

@cert-manager-prow
Copy link
Contributor

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

@arsenalzp arsenalzp reopened this Oct 29, 2024
@cert-manager-prow 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
@cert-manager-prow
Copy link
Contributor

@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
Copy link
Contributor Author

/test all

@erikgb
Copy link
Contributor

erikgb commented Oct 29, 2024

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

@arsenalzp
Copy link
Contributor Author

It seems all remarks have already been resolved...

@erikgb
Copy link
Contributor

erikgb commented Oct 29, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

arsenalzp commented Nov 7, 2024 •
edited
Loading

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

@erikgb
Copy link
Contributor

erikgb commented Nov 9, 2024

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?

@arsenalzp
Fix issue with updating of JKS/PKCS targets when password changes
eeed5e1 
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
Copy link
Contributor Author

Fixed.

erikgb
erikgb reviewed Nov 17, 2024
View reviewed changes
Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/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 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
Copy link
Contributor

[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:

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 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 erikgb requested a review from inteon November 17, 2024 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erikgb erikgb erikgb requested changes

@inteon inteon Awaiting requested review from inteon

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. 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. ok-to-test size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Should update target JKS/PKCS when password changed
3 participants
@arsenalzp @erikgb @inteon