search

cert-manager / trust-manager

trust-manager is an operator for distributing trust bundles across a Kubernetes cluster.
https://cert-manager.io/docs/projects/trust-manager/
Apache License 2.0
262 stars 70 forks source link

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

Closed arsenalzp closed 4 days ago

arsenalzp commented 1 month ago

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[bot] commented 1 month ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
erikgb commented 1 month ago

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

arsenalzp commented 1 month ago

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 commented 1 month ago

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.

arsenalzp commented 1 month ago

All remarks have been removed.

arsenalzp commented 1 month ago

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

erikgb commented 1 month ago

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 commented 1 month ago

Existing tests were modified

erikgb commented 1 month ago

/ok-to-test

arsenalzp commented 1 month ago

It seems the issue on my side with gomarkdoc.

inteon commented 1 month ago

It seems the issue on my side with gomarkdoc.

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

arsenalzp commented 1 month ago

/retest

arsenalzp commented 1 month ago 
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 commented 1 month ago

@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 commented 1 month ago

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

arsenalzp commented 1 month ago

@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 commented 1 month ago

I did it, it is replacing #nosec stanzas:

But we don't need, and should not have, the gosec suppressions in a markdown file! πŸ˜†

erikgb commented 1 month ago

My fault! Sorry! I added the suggestions to the markdown file. πŸ›•

arsenalzp commented 1 month ago

/retest

arsenalzp commented 1 month ago

/retest

arsenalzp commented 1 month ago

Yeah, it seems the issue with a the space character, here is example from project doc:

To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
arsenalzp commented 1 month ago

/retest

arsenalzp commented 1 month ago

/retest

inteon commented 1 month ago

FYI: you can locally generate using "make generate" end verify using "make verify-golangci-lint"

Will take a look at the issue myself on Monday.

arsenalzp commented 1 month ago

FYI: you can locally generate using "make generate" end verify using "make verify-golangci-lint"

Will take a look at the issue myself on Monday.

Hello, Yes, I did it yesterday, however the same error is still popping up:

$ make verify-golangci-lint
Running '_bin/tools/golangci-lint run --go 1.23.2 -c /home/username/go/src/trust-manager/.golangci.yaml' in directory './trust-packages/debian'

Running '_bin/tools/golangci-lint run --go 1.23.2 -c /home/username/go/src/trust-manager/.golangci.yaml' in directory '.'
pkg/apis/trust/v1alpha1/types_bundle.go:26: File is not `gofmt`-ed with `-s` (gofmt)
        BundleLabelKey                   = "trust.cert-manager.io/bundle"
        BundleHashAnnotationKey          = "trust.cert-manager.io/hash"
make: *** [make/_shared/go/01_mod.mk:112: verify-golangci-lint] Ошибка 1
$

P.S.

The issue was in source code format, assigned values positions should be aligned to one line as per gofmt output:

-       BundleLabelKey                   = "trust.cert-manager.io/bundle"
-       BundleHashAnnotationKey          = "trust.cert-manager.io/hash"
+       BundleLabelKey                      = "trust.cert-manager.io/bundle"
+       BundleHashAnnotationKey             = "trust.cert-manager.io/hash"
        BundleJksPasswdHashAnnotationKey    = "trust.cert-manager.io/jks-pwd-hash"    // #nosec G101
        BundlePkcs12PasswdHashAnnotationKey = "trust.cert-manager.io/pksc12-pwd-hash" // #nosec G101
arsenalzp commented 1 month ago

/retest

erikgb commented 3 weeks ago

image

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

arsenalzp commented 3 weeks ago

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 commented 3 weeks ago

/reopen

cert-manager-prow[bot] commented 3 weeks ago

@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](https://github.com/cert-manager/trust-manager/pull/449#issuecomment-2445276837): >/reopen Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
cert-manager-prow[bot] commented 3 weeks ago

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

Use /test all to run all jobs.

In response to [this](https://github.com/cert-manager/trust-manager/pull/449#issuecomment-2445302592): >/test Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
arsenalzp commented 3 weeks ago

/test all

erikgb commented 3 weeks ago

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

arsenalzp commented 3 weeks ago

It seems all remarks have already been resolved...

erikgb commented 3 weeks ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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

erikgb commented 2 weeks ago

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 commented 1 week ago

Fixed.

cert-manager-prow[bot] commented 1 week ago

[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](https://github.com/cert-manager/trust-manager/blob/main/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 commented 6 days ago

/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 commented 6 days ago

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