feat: support secret as a target #193

Jiawei0227 · 2023-10-07T01:06:11Z

Support #10

Add a new API Secret under taget to allow creating secret format CA certificates.
This is compatible with existing ConfigMap and can co-exist or exist alone.

jetstack-bot · 2023-10-07T01:06:21Z

Hi @Jiawei0227. 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/test-infra repository.

Jiawei0227 · 2023-10-07T01:07:48Z

/ok-to-test

/cc @SgtCoDFish @rudexi

jetstack-bot · 2023-10-07T01:07:50Z

@Jiawei0227: GitHub didn't allow me to request PR reviews from the following users: rudexi.

Note that only cert-manager members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/ok-to-test

/cc @SgtCoDFish @rudexi

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/test-infra repository.

jetstack-bot · 2023-10-07T01:08:02Z

@Jiawei0227: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

/cc @SgtCoDFish @rudexi

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/test-infra repository.

rudexi · 2023-10-07T01:14:28Z

/ok-to-test

jetstack-bot · 2023-10-07T01:14:42Z

@rudexi: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-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/test-infra repository.

inteon · 2023-10-09T11:31:09Z

/ok-to-test

inteon · 2023-10-09T11:35:10Z

Thanks @Jiawei0227 for rebasing this PR, could you dco signoff your commits & also add @rudexi as a co-author?

Jiawei0227 · 2023-10-09T17:25:40Z

Thanks @Jiawei0227 for rebasing this PR, could you dco signoff your commits & also add @rudexi as a co-author?

Done. This CL is mostly a rewrite since the previous PR is completely un-rebaseable. But I am okay with co-authoring @rudexi since this PR is inspired by him

erikgb · 2023-10-10T19:21:56Z

/retest

SgtCoDFish

Thanks for your work on this! I have a couple of suggestions - what do you think?

SgtCoDFish · 2023-10-11T09:09:57Z

deploy/charts/trust-manager/values.yaml

+# -- List of secrets trust-manager is allowed to read/write. Used when the secret target is set when creating
+# a bundle. If the list is empty it means read/write access to all secrets.
+authorizedSecrets: ["ca-bundle"]


suggestion: I'm not sure if this is the safest way to do this.

I have a few concerns:

Having a default here feels wrong - since most clusters won't need to be able to read/write to secrets called ca-bundle. I'd rather the default be that there's nothing extra which we have access to, so that users have to explicitly expand trust-manager's permissions.

Having an empty list confer permissions to everything is really confusing to me (the intuitive reading would be that an empty list means there are no allowed secrets at all)

There doesn't seem to be an obvious way to allow no secrets, in the event that I don't want to use secrets at all.

I'd personally rather the default be "no secrets are authorized". How about something like the following?

secretTargets: # -- If set to true, enable writing trust bundles to Kubernetes Secrets as a target enabled: false # -- A list of secret names which trust-manager will be permitted to read and write across all namespaces. These will be the only allowable Secrets that can be used as targets. Must be a non-empty list if secretTargets.enabled is true authorizedSecrets: [] # NOTE: I originally wrote [""] here but [] is better

What would be the difference between authorizedSecrets: [""] and authorizedSecrets: [] ?

Whoops, that was a typo, my bad! I think the default should be authorizedSecrets: [].

If enabled is set to true, we should check that authorizedSecrets is a list with at least one valid secret name. "" wouldn't be a valid secret name.

For resourceNames, an empty set means that everything is allowed

I was thinking https://kubernetes.io/docs/reference/access-authn-authz/rbac/#referring-to-resources the resourceNames field in k8s and an empty list there infer permission to all.

However, I understand the concern here. But then the question is probably what option we will use to indicate "permission to all"? The problem is resourceNames does not allow wildcard matching (kubernetes/kubernetes#56582). I added an enabled field so that user have to explicitly enable it. wdyt?

I think we need a separate config to enable RBAC to any secret. It seems scary to rely on a Helm list value to express this desire.

okay, I add another option authorizedSecretsAll to indicate permission to all. And now empty list of authorizedSecrets means access to nothing

pkg/bundle/sync.go

Jiawei0227 · 2023-10-11T18:12:04Z

/retest

SgtCoDFish

Few more comments - really appreciate the work you've put into this. We're nearly there 😁

deploy/charts/trust-manager/values.yaml

pkg/apis/trust/v1alpha1/types_bundle.go

test/env/data.go

SgtCoDFish · 2023-10-17T10:50:53Z

test/env/data.go

@@ -39,7 +39,7 @@ import (
 )

 const (
-	EventuallyTimeout      = "30s"
+	EventuallyTimeout      = "90s"


question: Why are we bumping this? 30s seems like it should be plenty for this use case to me, but I'd happily bump if you've been running into problems

I run this locally, if its the first time I bring up KIND cluster it will timeout. It took around 40s etc to finish all the testing in my local.

Inspired by cert-manager#108 Signed-off-by: Jiawei Wang <[email protected]>,Guillaume Ludinard <[email protected]>

Jiawei0227 · 2023-10-18T04:18:21Z

/retest

SgtCoDFish

/lgtm
/approve

I ran this locally and it worked great! Thank you so much for your efforts on this, really appreciate it!

jetstack-bot · 2023-10-18T12:07:40Z

[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

inteon · 2023-10-23T18:40:17Z

deploy/charts/trust-manager/templates/clusterrole.yaml

+  - ""
+  resources:
+  - "secrets"
+  verbs: ["create", "list", "watch"]


@SgtCoDFish shouldn't this also be part of the .Values.secretTargets.enabled section?

We still by default grant trust-manager access to read ALL secret resources.

I think we must conditionally watch secrets from the controller if we add a condition here. And that might be the reason behind this bug.

Here is a WIP PR to fix this: #207. My Tuesday (tomorrow) is filled with meetings, so feel free to pick it up. My gut feeling says that this new feature needs more work....

Yeah that's my bad... thought I checked the permissions and clearly wasn't thorough / careful enough in my efforts to move this forwards. Sorry.

Thanks for confirming that this was not the intended behavior @SgtCoDFish.

jetstack-bot added dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 7, 2023

jetstack-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 7, 2023

Jiawei0227 mentioned this pull request Oct 7, 2023

Supporting a secret target #108

Closed

jetstack-bot requested a review from SgtCoDFish October 7, 2023 01:07

jetstack-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 9, 2023

Jiawei0227 force-pushed the secrets branch from b487903 to 002ab55 Compare October 9, 2023 17:25

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 Oct 9, 2023

Jiawei0227 force-pushed the secrets branch 3 times, most recently from 1710f91 to 00260c6 Compare October 9, 2023 23:58

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2023

Jiawei0227 force-pushed the secrets branch from 00260c6 to 834772e Compare October 10, 2023 17:18

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2023

Jiawei0227 force-pushed the secrets branch from 834772e to bed8234 Compare October 10, 2023 17:29

SgtCoDFish requested changes Oct 11, 2023

View reviewed changes

Jiawei0227 force-pushed the secrets branch from bed8234 to 1d53da0 Compare October 11, 2023 18:08

Jiawei0227 force-pushed the secrets branch from 1d53da0 to 23412d5 Compare October 11, 2023 18:52

SgtCoDFish reviewed Oct 17, 2023

View reviewed changes

feat: support secret as a target

0bd29a2

Inspired by cert-manager#108 Signed-off-by: Jiawei Wang <[email protected]>,Guillaume Ludinard <[email protected]>

Jiawei0227 force-pushed the secrets branch from 23412d5 to 0bd29a2 Compare October 18, 2023 04:16

SgtCoDFish approved these changes Oct 18, 2023

View reviewed changes

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2023

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2023

jetstack-bot merged commit f5fadf2 into cert-manager:main Oct 18, 2023

erikgb mentioned this pull request Oct 18, 2023

Feature: support secret target #10

Closed

SgtCoDFish mentioned this pull request Oct 18, 2023

Fix build on macOS / values.yaml wording tweaks #200

Merged

inteon reviewed Oct 23, 2023

View reviewed changes

erikgb mentioned this pull request Oct 23, 2023

fix: should not have have read access to all secrets when secret targets disabled #207

Merged

This was referenced Oct 25, 2023

BUGFIX: fix bugs in validation logic for secret target & add tests #212

Merged

BUGFIX: support switching between target types #211

Merged

erikgb mentioned this pull request May 1, 2024

Fix Bundle target print column #344

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support secret as a target #193

feat: support secret as a target #193

Jiawei0227 commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

Jiawei0227 commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

rudexi commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

inteon commented Oct 9, 2023

inteon commented Oct 9, 2023 •

edited

Loading

Jiawei0227 commented Oct 9, 2023

erikgb commented Oct 10, 2023

SgtCoDFish left a comment

SgtCoDFish Oct 11, 2023 •

edited

Loading

inteon Oct 11, 2023

SgtCoDFish Oct 11, 2023

Jiawei0227 Oct 11, 2023

erikgb Oct 11, 2023

Jiawei0227 Oct 11, 2023

Jiawei0227 commented Oct 11, 2023

SgtCoDFish left a comment

SgtCoDFish Oct 17, 2023

Jiawei0227 Oct 18, 2023

Jiawei0227 commented Oct 18, 2023

SgtCoDFish left a comment

jetstack-bot commented Oct 18, 2023

inteon Oct 23, 2023

inteon Oct 23, 2023

erikgb Oct 23, 2023 •

edited

Loading

erikgb Oct 23, 2023

SgtCoDFish Oct 24, 2023

inteon Oct 24, 2023

feat: support secret as a target #193

feat: support secret as a target #193

Conversation

Jiawei0227 commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

Jiawei0227 commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

rudexi commented Oct 7, 2023

jetstack-bot commented Oct 7, 2023

inteon commented Oct 9, 2023

inteon commented Oct 9, 2023 • edited Loading

Jiawei0227 commented Oct 9, 2023

erikgb commented Oct 10, 2023

SgtCoDFish left a comment

Choose a reason for hiding this comment

SgtCoDFish Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jiawei0227 commented Oct 11, 2023

SgtCoDFish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jiawei0227 commented Oct 18, 2023

SgtCoDFish left a comment

Choose a reason for hiding this comment

jetstack-bot commented Oct 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikgb Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inteon commented Oct 9, 2023 •

edited

Loading

SgtCoDFish Oct 11, 2023 •

edited

Loading

erikgb Oct 23, 2023 •

edited

Loading