Add collect keyless params task & related keyless improvements#3171
Open
simonbaird wants to merge 5 commits intoconforma:mainfrom
Open
Add collect keyless params task & related keyless improvements#3171simonbaird wants to merge 5 commits intoconforma:mainfrom
simonbaird wants to merge 5 commits intoconforma:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request introduces TUF (The Update Framework) infrastructure for acceptance testing, adds keyless signing parameter collection capabilities, implements certificate identity regexp variants for verification tasks, and extends the Kubernetes acceptance test framework with ConfigMap and namespace management features. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant KubAPI as Kubernetes API
participant ConfigMap as ConfigMap<br/>(cluster-config)
participant VerifyTask as Verify Task
participant TUF as TUF Server
participant Rekor as Rekor
Test->>KubAPI: Create namespace
KubAPI-->>Test: namespace created
Test->>KubAPI: Create ConfigMap<br/>(keyless config)
KubAPI-->>Test: ConfigMap created
Test->>VerifyTask: Execute with keyless params<br/>(CERTIFICATE_IDENTITY,<br/>CERTIFICATE_OIDC_ISSUER)
activate VerifyTask
VerifyTask->>VerifyTask: Detect keyless mode<br/>(non-empty cert params)
VerifyTask->>TUF: Fetch root.json
TUF-->>VerifyTask: root metadata
VerifyTask->>Rekor: Verify transparency log
Rekor-->>VerifyTask: verification result
VerifyTask->>VerifyTask: Validate certificate identity<br/>against OIDC issuer
deactivate VerifyTask
VerifyTask-->>Test: verification result
Note over Test,Rekor: Alternatively with regexp params<br/>uses CERTIFICATE_IDENTITY_REGEXP<br/>and CERTIFICATE_OIDC_ISSUER_REGEXP
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.3)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
Contributor
Review Summary by QodoAdd collect-keyless-signing-params task and keyless signing improvements
WalkthroughsDescription• Add collect-keyless-signing-params Tekton task for reading keyless signing configuration from ConfigMaps • Support running TUF service inside Kind cluster using nginx with wiremock-extracted files • Add keyless image verification acceptance test with local cluster-internal TUF • Enhance verification tasks with certificate identity/issuer regexp parameters • Add ConfigMap and namespace creation capabilities to Kubernetes test infrastructure Diagramflowchart LR
A["Tekton Tasks"] -->|add regexp params| B["verify-enterprise-contract<br/>verify-conforma-konflux-ta"]
C["Kubernetes Infrastructure"] -->|create ConfigMaps| D["Test Cluster"]
E["TUF Service"] -->|nginx in cluster| D
F["Acceptance Tests"] -->|keyless verification| B
G["collect-keyless-signing-params"] -->|read cluster config| D
File Changes1. tasks/collect-keyless-signing-params/0.1/collect-keyless-signing-params.yaml
|
Contributor
Code Review by Qodo
1. ConfigMap update missing RV
|
4 tasks
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
tasks/collect-keyless-signing-params/0.1/collect-keyless-signing-params.yaml
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
acceptance/kubernetes/kind/kubernetes.go (2)
241-248: RBAC is created on every ConfigMap creation.
ensureConfigMapRBACis called on everyCreateConfigMapinvocation. While the function is idempotent, this adds unnecessary API calls. Consider caching whether RBAC has been created or moving RBAC setup to cluster initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/kubernetes.go` around lines 241 - 248, The code currently calls ensureConfigMapRBAC on every CreateConfigMap call causing unnecessary API chatter; update the logic to run RBAC setup once and skip subsequent calls by adding a guarded/cache flag (e.g., a field on the Kubernetes struct like rbacConfigured bool or sync.Once to protect ensureConfigMapRBAC) and have CreateConfigMap check that flag/Once before invoking ensureConfigMapRBAC; alternatively move the ensureConfigMapRBAC call to cluster initialization path (where cluster setup functions run) so CreateConfigMap no longer invokes ensureConfigMapRBAC on each call.
229-239: Useerrors.IsAlreadyExistsinstead of string matching for error handling.String-based error checking is fragile and could break if error message wording changes. The Kubernetes client-go library provides proper error type checking.
♻️ Suggested fix using proper error type checking
+import ( + k8serrors "k8s.io/apimachinery/pkg/api/errors" +) // Create the ConfigMap (or update if it already exists) if _, err := k.client.CoreV1().ConfigMaps(namespace).Create(ctx, configMap, metav1.CreateOptions{}); err != nil { - if strings.Contains(err.Error(), "already exists") { + if k8serrors.IsAlreadyExists(err) { // ConfigMap exists, so update it with the new content if _, err := k.client.CoreV1().ConfigMaps(namespace).Update(ctx, configMap, metav1.UpdateOptions{}); err != nil { return fmt.Errorf("failed to update existing ConfigMap: %w", err) } } else { return err } }Apply the same pattern to lines 268, 294, and 311.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@acceptance/kubernetes/kind/kubernetes.go` around lines 229 - 239, Replace fragile string matching on err.Error() with Kubernetes API error type checking using errors.IsAlreadyExists: when Create returns an error, call apierrors.IsAlreadyExists(err) (importing "k8s.io/apimachinery/pkg/api/errors" as apierrors) and if true perform the Update; otherwise return the error. Apply this change to the ConfigMap create/update block that uses k.client.CoreV1().ConfigMaps(namespace).Create(...) and the analogous create handlers at the other locations mentioned (the blocks around the usages at lines referenced in the review) so all Create/Update flows use apierrors.IsAlreadyExists(err) instead of strings.Contains(err.Error(), "already exists").hack/tuf/tuf.yaml (1)
27-97: Consider adding security context for containers.Static analysis flagged missing security hardening (CKV_K8S_20, CKV_K8S_23). While this is acceptable for test infrastructure in a Kind cluster, adding a security context would align with best practices and silence these warnings.
♻️ Optional security context addition
initContainers: - name: setup-tuf-data image: busybox:1.35 + securityContext: + allowPrivilegeEscalation: false + runAsNonRoot: true + runAsUser: 65534 command: - shNote: If runAsNonRoot causes permission issues with volume mounts, this can be omitted for test infrastructure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/tuf/tuf.yaml` around lines 27 - 97, Summary: Add a securityContext to the initContainer "setup-tuf-data" (and the following containers block) to satisfy CKV_K8S_20/23. Instruction: In the initContainers entry for name: setup-tuf-data (and each container under containers) add a securityContext with fields such as runAsNonRoot: true (or runAsUser: 1000 if needed), allowPrivilegeEscalation: false, readOnlyRootFilesystem: true, and capabilities: drop: ["ALL"]; ensure any volume-mounted paths still have correct ownership or switch to a non-root UID that matches volume permissions if mounting fails; apply the same securityContext pattern to the main containers to silence the static analysis warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/image/image.go`:
- Around line 955-983: The DSSE parsing in unmarshallSignatures incorrectly
short-circuits fallback because dsseEnvelope only declares Signatures; update
dsseEnvelope to include DSSE-specific fields (e.g., "payload" and "payloadType"
or other required DSSE fields used by your format) and in unmarshallSignatures
validate those fields (and that envelope.Signatures has exactly one entry)
before returning a cosign.Signatures; if those DSSE-specific fields are missing
or invalid, proceed to unmarshal into cosign.AttestationPayload as the fallback.
Ensure you reference dsseEnvelope and unmarshallSignatures when making the
changes so the parser only accepts genuine DSSE envelopes.
In `@features/task_validate_image.feature`:
- Around line 488-514: Add the missing assertion asserting the task result
"buildIdentity" is empty for the "Collect keyless signing parameters from
ConfigMap with keyless signing disabled" scenario: after the other task result
checks (e.g., "buildIdentityRegexp" and "keylessSigningEnabled") add an
assertion that the task result "buildIdentity" equals "" so the step
"collect-signing-params" output is validated consistently with the disabled
case.
In `@hack/tuf/create-tuf-files.sh`:
- Around line 1-5: Add the project's standard license header at the top of the
script so lint passes: insert the repository's required comment block (the
standard license/header text) immediately before the existing shebang
(#!/bin/bash) in hack/tuf/create-tuf-files.sh, preserving the rest of the file
(including set -e); ensure the header format and exact text match other files in
the repo.
- Around line 34-36: The condition currently excludes any filename containing
"root.json" (e.g., "1.root.json"), so change the test to only skip the exact
unversioned file "root.json"; update the if that checks body_file (the branch
that appends to body_files via body_files+=("$body_file")) to use an exact
string comparison ([[ "$body_file" != "root.json" ]]) or an anchored regex ([[ !
"$body_file" =~ ^root\.json$ ]]) so rotated files like "1.root.json" are allowed
through.
In `@hack/tuf/tuf.sh`:
- Around line 81-94: The nginx document root only mounts the tuf-root-data
ConfigMap; add the generated tuf-files ConfigMap into the same mounted volume by
changing the volume named "tuf-data" into a projected volume that includes both
configMap sources (tuf-root-data and tuf-files) so the volumeMount at
/usr/share/nginx/html continues to serve timestamp.json, snapshot.json and
targets; leave the "nginx-config" volume/volumeMount as-is and ensure the
projected volume uses the same name "tuf-data" so the existing volumeMount
remains valid.
In `@hack/tuf/tuf.yaml`:
- Around line 1-8: Add the Apache 2.0 license header at the very top of this
YAML file (before the existing document start '---'), ensuring the header is a
commented block so it doesn't interfere with YAML parsing; update the file that
contains the ConfigMap named "tuf-port-number" (kind: ConfigMap, data key: PORT)
to include the full Apache 2.0 license comment header.
In
`@tasks/collect-keyless-signing-params/0.1/collect-keyless-signing-params.yaml`:
- Around line 117-137: The current kubectl get call hides all errors and always
treats failures as “ConfigMap not found”; change the logic in the block that
runs kubectl get configmap "${configMapName}" -n "${configMapNamespace}" -o json
> "${KFLX_CONFIG_PATH}" so that you capture kubectl’s stderr and exit code,
treat a true “NotFound” (or 404) response as the existing “not found” branch,
but for any other non-zero exit status (RBAC/authorization errors, apiserver
errors, network issues) log the stderr (or a clear error message referencing
"${configMapName}" and "${configMapNamespace}") and exit non-zero instead of
defaulting keylessSigningEnabled to false; keep the subsequent jq reads
(keylessSigningEnabled, defaultOIDCIssuer, etc.) unchanged once the ConfigMap is
successfully fetched to "${KFLX_CONFIG_PATH}".
---
Nitpick comments:
In `@acceptance/kubernetes/kind/kubernetes.go`:
- Around line 241-248: The code currently calls ensureConfigMapRBAC on every
CreateConfigMap call causing unnecessary API chatter; update the logic to run
RBAC setup once and skip subsequent calls by adding a guarded/cache flag (e.g.,
a field on the Kubernetes struct like rbacConfigured bool or sync.Once to
protect ensureConfigMapRBAC) and have CreateConfigMap check that flag/Once
before invoking ensureConfigMapRBAC; alternatively move the ensureConfigMapRBAC
call to cluster initialization path (where cluster setup functions run) so
CreateConfigMap no longer invokes ensureConfigMapRBAC on each call.
- Around line 229-239: Replace fragile string matching on err.Error() with
Kubernetes API error type checking using errors.IsAlreadyExists: when Create
returns an error, call apierrors.IsAlreadyExists(err) (importing
"k8s.io/apimachinery/pkg/api/errors" as apierrors) and if true perform the
Update; otherwise return the error. Apply this change to the ConfigMap
create/update block that uses
k.client.CoreV1().ConfigMaps(namespace).Create(...) and the analogous create
handlers at the other locations mentioned (the blocks around the usages at lines
referenced in the review) so all Create/Update flows use
apierrors.IsAlreadyExists(err) instead of strings.Contains(err.Error(), "already
exists").
In `@hack/tuf/tuf.yaml`:
- Around line 27-97: Summary: Add a securityContext to the initContainer
"setup-tuf-data" (and the following containers block) to satisfy CKV_K8S_20/23.
Instruction: In the initContainers entry for name: setup-tuf-data (and each
container under containers) add a securityContext with fields such as
runAsNonRoot: true (or runAsUser: 1000 if needed), allowPrivilegeEscalation:
false, readOnlyRootFilesystem: true, and capabilities: drop: ["ALL"]; ensure any
volume-mounted paths still have correct ownership or switch to a non-root UID
that matches volume permissions if mounting fails; apply the same
securityContext pattern to the main containers to silence the static analysis
warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 792d01cd-dd7b-4142-9146-28ad0fe7334a
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (24)
.gitignoreMakefileacceptance/image/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/kubernetes/kubernetes.goacceptance/kubernetes/stub/stub.goacceptance/kubernetes/types/types.goacceptance/tuf/tuf.goacceptance/wiremock/wiremock.godocs/modules/ROOT/pages/collect-keyless-signing-params.adocdocs/modules/ROOT/pages/verify-conforma-konflux-ta.adocdocs/modules/ROOT/pages/verify-enterprise-contract.adocdocs/modules/ROOT/partials/tasks_nav.adocfeatures/task_validate_image.featurehack/test/kustomization.yamlhack/tuf/create-tuf-files.shhack/tuf/kustomization.yamlhack/tuf/namespace.yamlhack/tuf/tuf.shhack/tuf/tuf.yamltasks/collect-keyless-signing-params/0.1/collect-keyless-signing-params.yamltasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
tasks/collect-keyless-signing-params/0.1/collect-keyless-signing-params.yaml
Outdated
Show resolved
Hide resolved
The goal is to be able to have a working TUF root in the Kind cluster so we can use it in a Tekton task during acceptance tests. Also, expose some env vars that we'll need in upcoming Tekton task acceptance tests. The way this is done is that we run nginx in the cluster, but with files that are extracted (using a bash script added in this commit) from the existing wiremock data. Beware we now have two different ways of stubbing/mocking a TUF instance, one new way that works in the cluster and can be use when testing Tekton tasks, and one old way that works directly in a test container and uses wiremock. (In case it's not obvious, there was a lot of heavy lifing done by Claude here.) Ref: https://issues.redhat.com/browse/EC-1695 Co-authored-by: Claude Code <noreply@anthropic.com>
Also namespaces, since we want the ConfigMap in a particular namespace. An RBAC is created also so the ConfigMap is readable by every service account. This will be used in the acceptance test added in an upcoming commit. Ref: https://issues.redhat.com/browse/EC-1695 Co-authored-by: Claude Code <noreply@anthropic.com>
simonbaird
force-pushed
the
collect-keyless-params-task
branch
from
2fd2640 to
92180c0
Compare
I got tired of looking at the repeated policies in task_validate_image.feature. This technique could perhaps be extended to other feature files, but I don't want to do that right now. Unrelated to, but created while working on the PR for... Ref: https://issues.redhat.com/browse/EC-1695 Co-authored-by: Claude Code <noreply@anthropic.com>
- Add acceptance tests that use the local test image instead of the "real" image in quay. This was enabled by the previous commit related to running nginx TUF in the cluster. - Add support for regexp style params keyless signature verification params - Minor tweak to the handling of ignore rekor so we don't ignore it when doing keyless. - Minor improvements/tidying with the task param logic Note: I was trying to maintain several smaller commits, but it became difficult after some (possibly unrelated) local acceptance test problems. Ref: https://redhat.atlassian.net/browse/EC-1695
Ref: https://redhat.atlassian.net/browse/EC-1695 Co-authored-by: Claude Code <noreply@anthropic.com>
simonbaird
force-pushed
the
collect-keyless-params-task
branch
from
92180c0 to
142a144
Compare
Contributor
Review Summary by QodoAdd collect-keyless-signing-params task and keyless signing improvements
WalkthroughsDescription• Add collect-keyless-signing-params Tekton task to extract signing configuration from ConfigMaps • Support running nginx TUF server inside Kind cluster for acceptance testing • Add ConfigMap and namespace creation capabilities to Kubernetes test infrastructure • Implement regexp support for certificate identity/issuer matching in verification tasks • Reduce policy repetition in feature files with helper step definitions • Add acceptance tests for keyless signing with local test images Diagramflowchart LR
A["Tekton Task<br/>collect-keyless-signing-params"] -->|reads| B["ConfigMap<br/>cluster-config"]
B -->|provides| C["Signing Parameters<br/>OIDC, Rekor, TUF URLs"]
D["TUF Server<br/>nginx in cluster"] -->|serves| E["Acceptance Tests<br/>keyless verification"]
F["Test Infrastructure<br/>ConfigMap/Namespace creation"] -->|supports| E
G["Verification Tasks<br/>regexp certificate matching"] -->|uses| C
File Changes1. tasks/collect-keyless-signing-params/0.1/collect-keyless-signing-params.yaml
|
Contributor
Code Review by Qodo
1. Acceptance go test now fails
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
features/task_validate_image.feature (1)
417-423:⚠️ Potential issue | 🟡 MinorAdd the missing
buildIdentityfallback assertion in the missing-ConfigMap scenario.This scenario validates most empty defaults but skips
buildIdentity, which leaves a small regression gap forcollect-signing-paramsoutput consistency.🔧 Suggested test assertion
And the task result "tufExternalUrl" should equal "" + And the task result "buildIdentity" should equal "" And the task result "buildIdentityRegexp" should equal "" And the task result "keylessSigningEnabled" should equal "false"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/task_validate_image.feature` around lines 417 - 423, Missing assertion: the scenario that checks empty defaults for the "collect-signing-params" step omits the "buildIdentity" task result. Add an assertion alongside the other task result checks to verify that task result "buildIdentity" equals "" (i.e., the empty fallback) so the test covers the missing-ConfigMap fallback; place this new assertion with the other lines checking "defaultOIDCIssuer", "rekorExternalUrl", "fulcioExternalUrl", "tufExternalUrl", "buildIdentityRegexp", and "keylessSigningEnabled".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@acceptance/kubernetes/kind/kubernetes.go`:
- Around line 282-299: The ClusterRoleBinding currently grants read access to
all cluster service accounts via the Subject Group "system:serviceaccounts";
update the ClusterRoleBinding (variable clusterRoleBinding of type
rbacv1.ClusterRoleBinding) to scope access to the specific test service account
by replacing the Subjects entry with a Subject of Kind "ServiceAccount", Name
set to the test SA (e.g. the task service account name used in tests), and
Namespace set to the test namespace; keep the RoleRef
(acceptance-configmap-reader) but ensure the binding name and Subject Namespace
match the acceptance test namespace so the permission is limited to only that
service account.
In `@hack/tuf/tuf.yaml`:
- Around line 42-129: Add an explicit securityContext to both the initContainer
"setup-tuf-data" and main container "tuf-server": set runAsNonRoot: true (and
runAsUser: 1000 or a non-root UID), allowPrivilegeEscalation: false,
capabilities: drop: ["ALL"], readOnlyRootFilesystem: true, and a seccompProfile
(type: RuntimeDefault) so both containers are constrained by seccomp; ensure
these keys are placed under each container spec (initContainers[] -> name:
setup-tuf-data and containers[] -> name: tuf-server) and adjust runAsUser if the
busybox copy logic or nginx needs a specific non-root UID.
---
Duplicate comments:
In `@features/task_validate_image.feature`:
- Around line 417-423: Missing assertion: the scenario that checks empty
defaults for the "collect-signing-params" step omits the "buildIdentity" task
result. Add an assertion alongside the other task result checks to verify that
task result "buildIdentity" equals "" (i.e., the empty fallback) so the test
covers the missing-ConfigMap fallback; place this new assertion with the other
lines checking "defaultOIDCIssuer", "rekorExternalUrl", "fulcioExternalUrl",
"tufExternalUrl", "buildIdentityRegexp", and "keylessSigningEnabled".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c12462d3-731a-4e75-ab5c-b8dd85512824
⛔ Files ignored due to path filters (1)
features/__snapshots__/task_validate_image.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
.gitignoreMakefileacceptance/image/image.goacceptance/kubernetes/kind/kind.goacceptance/kubernetes/kind/kubernetes.goacceptance/kubernetes/kubernetes.goacceptance/kubernetes/stub/stub.goacceptance/kubernetes/types/types.goacceptance/tuf/tuf.goacceptance/wiremock/wiremock.godocs/modules/ROOT/pages/collect-keyless-signing-params.adocdocs/modules/ROOT/pages/verify-conforma-konflux-ta.adocdocs/modules/ROOT/pages/verify-enterprise-contract.adocdocs/modules/ROOT/partials/tasks_nav.adocfeatures/task_validate_image.featurehack/test/kustomization.yamlhack/tuf/create-tuf-files.shhack/tuf/kustomization.yamlhack/tuf/namespace.yamlhack/tuf/tuf.yamltasks/collect-keyless-signing-params/0.1/collect-keyless-signing-params.yamltasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- docs/modules/ROOT/partials/tasks_nav.adoc
- tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
- acceptance/tuf/tuf.go
- acceptance/image/image.go
- hack/test/kustomization.yaml
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
- docs/modules/ROOT/pages/verify-conforma-konflux-ta.adoc
- Makefile
- docs/modules/ROOT/pages/verify-enterprise-contract.adoc
Comment on lines
+282
to
+299
| // Create ClusterRoleBinding for all service accounts (idempotent) | ||
| clusterRoleBinding := &rbacv1.ClusterRoleBinding{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "acceptance-configmap-reader-binding", | ||
| }, | ||
| RoleRef: rbacv1.RoleRef{ | ||
| APIGroup: "rbac.authorization.k8s.io", | ||
| Kind: "ClusterRole", | ||
| Name: "acceptance-configmap-reader", | ||
| }, | ||
| Subjects: []rbacv1.Subject{ | ||
| { | ||
| Kind: "Group", | ||
| Name: "system:serviceaccounts", | ||
| APIGroup: "rbac.authorization.k8s.io", | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
RBAC binding is too broad (system:serviceaccounts cluster-wide).
Line 295 binds ConfigMap read access to all service accounts in the cluster. For acceptance tests this should be scoped to the specific task service account/namespace only.
🔒 Scope RBAC to the test service account instead of all SAs
-func (k *kindCluster) ensureConfigMapRBAC(ctx context.Context) error {
+func (k *kindCluster) ensureConfigMapRBAC(ctx context.Context, configMapNamespace string) error {
+ t := testenv.FetchState[testState](ctx)
@@
- clusterRoleBinding := &rbacv1.ClusterRoleBinding{
+ roleBinding := &rbacv1.RoleBinding{
ObjectMeta: metav1.ObjectMeta{
- Name: "acceptance-configmap-reader-binding",
+ Name: fmt.Sprintf("acceptance-configmap-reader-%s", t.namespace),
+ Namespace: configMapNamespace,
},
RoleRef: rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "ClusterRole",
Name: "acceptance-configmap-reader",
},
Subjects: []rbacv1.Subject{
{
- Kind: "Group",
- Name: "system:serviceaccounts",
- APIGroup: "rbac.authorization.k8s.io",
+ Kind: "ServiceAccount",
+ Name: "default",
+ Namespace: t.namespace,
},
},
}
- if _, err := k.client.RbacV1().ClusterRoleBindings().Create(ctx, clusterRoleBinding, metav1.CreateOptions{}); err != nil {
+ if _, err := k.client.RbacV1().RoleBindings(configMapNamespace).Create(ctx, roleBinding, metav1.CreateOptions{}); err != nil {
...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@acceptance/kubernetes/kind/kubernetes.go` around lines 282 - 299, The
ClusterRoleBinding currently grants read access to all cluster service accounts
via the Subject Group "system:serviceaccounts"; update the ClusterRoleBinding
(variable clusterRoleBinding of type rbacv1.ClusterRoleBinding) to scope access
to the specific test service account by replacing the Subjects entry with a
Subject of Kind "ServiceAccount", Name set to the test SA (e.g. the task service
account name used in tests), and Namespace set to the test namespace; keep the
RoleRef (acceptance-configmap-reader) but ensure the binding name and Subject
Namespace match the acceptance test namespace so the permission is limited to
only that service account.
Comment on lines
+42
to
+129
| spec: | ||
| initContainers: | ||
| - name: setup-tuf-data | ||
| image: busybox:1.35 | ||
| command: | ||
| - sh | ||
| - -c | ||
| - | | ||
| cp /tuf-files/* /tuf-data/ | ||
|
|
||
| # Create the versioned metadata files that TUF expects | ||
| cp /tuf-data/body-timestamp.json-*.json /tuf-data/timestamp.json | ||
| cp /tuf-data/body-1.snapshot.json-*.json /tuf-data/1.snapshot.json | ||
| cp /tuf-data/body-1.targets.json-*.json /tuf-data/1.targets.json | ||
|
|
||
| # Create root.json from embedded content | ||
| cp /tuf-root/root.json /tuf-data/root.json | ||
|
|
||
| # Create targets directory and copy target files with hash-based names dynamically | ||
| mkdir -p /tuf-data/targets | ||
|
|
||
| # Process all target files using patterns instead of hardcoded names | ||
| for file in /tuf-data/body-targets-*; do | ||
| [ -f "$file" ] || continue | ||
|
|
||
| # Extract the original filename by removing the body-targets- prefix and the -HASH.ext suffix | ||
| basename_file=$(basename "$file") | ||
|
|
||
| # Pattern: body-targets-<LONG-HASH>.<filename>-<SHORT-HASH>.<ext> | ||
| # We want to extract <LONG-HASH>.<filename>.<ext> | ||
| # Handle different patterns by removing the suffix intelligently | ||
| without_prefix=$(echo "$basename_file" | sed 's/^body-targets-//') | ||
|
|
||
| # Special case for tsa_intermedi -> tsa_intermediate_0.crt | ||
| if echo "$without_prefix" | grep -q "tsa_intermedi-"; then | ||
| target_name=$(echo "$without_prefix" | sed 's/tsa_intermedi-.*/tsa_intermediate_0.crt.pem/') | ||
| # For files ending with .-HASH.ext, remove the .-HASH part | ||
| elif echo "$without_prefix" | grep -q '\.-[A-Za-z0-9]*\.'; then | ||
| target_name=$(echo "$without_prefix" | sed 's/\.-[A-Za-z0-9]*\./\./') | ||
| # For files ending with -HASH.ext, remove the -HASH part but keep the extension | ||
| elif echo "$without_prefix" | grep -q '\-[A-Za-z0-9]*\.[a-z]*$'; then | ||
| # Extract everything before -HASH, then add the extension | ||
| ext=$(echo "$without_prefix" | sed 's/.*\.//') | ||
| base=$(echo "$without_prefix" | sed 's/\(.*\)-[A-Za-z0-9]*\.[a-z]*$/\1/') | ||
| target_name="$base.$ext" | ||
| else | ||
| # Fallback: remove -HASH.ext and add the extension | ||
| ext=$(echo "$basename_file" | sed 's/.*\.//') | ||
| base=$(echo "$without_prefix" | sed 's/-[A-Za-z0-9]*\.[a-z]*$//') | ||
| target_name="$base.$ext" | ||
| fi | ||
|
|
||
| if [ -n "$target_name" ]; then | ||
| cp "$file" "/tuf-data/targets/$target_name" | ||
| echo "Copied $basename_file -> targets/$target_name" | ||
| else | ||
| echo "Warning: Could not parse filename pattern for $basename_file" | ||
| fi | ||
| done | ||
|
|
||
| ls -la /tuf-data/ | ||
| ls -la /tuf-data/targets/ | ||
| volumeMounts: | ||
| - name: tuf-files | ||
| mountPath: /tuf-files | ||
| readOnly: true | ||
| - name: tuf-root | ||
| mountPath: /tuf-root | ||
| readOnly: true | ||
| - name: tuf-data | ||
| mountPath: /tuf-data | ||
| containers: | ||
| - name: tuf-server | ||
| image: docker.io/nginx:1.25-alpine | ||
| ports: | ||
| - name: http | ||
| containerPort: 8080 | ||
| env: | ||
| - name: NGINX_PORT | ||
| value: "8080" | ||
| volumeMounts: | ||
| - name: tuf-data | ||
| mountPath: /usr/share/nginx/html | ||
| readOnly: true | ||
| - name: nginx-config | ||
| mountPath: /etc/nginx/conf.d/default.conf | ||
| subPath: nginx.conf | ||
| volumes: |
There was a problem hiding this comment.
Harden the TUF deployment with explicit container security context.
Line 42 onward defines containers without explicit hardening (allowPrivilegeEscalation, dropped capabilities, seccomp). This weakens security guarantees in CI/shared clusters.
🔐 Suggested hardening baseline
spec:
template:
metadata:
labels:
app.kubernetes.io/name: tuf-server
spec:
+ securityContext:
+ seccompProfile:
+ type: RuntimeDefault
initContainers:
- name: setup-tuf-data
image: busybox:1.35
+ securityContext:
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop: ["ALL"]
command:
- sh
- -c
- |
cp /tuf-files/* /tuf-data/
@@
containers:
- name: tuf-server
image: docker.io/nginx:1.25-alpine
+ securityContext:
+ allowPrivilegeEscalation: false
+ capabilities:
+ drop: ["ALL"]
ports:
- name: http
containerPort: 8080🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/tuf/tuf.yaml` around lines 42 - 129, Add an explicit securityContext to
both the initContainer "setup-tuf-data" and main container "tuf-server": set
runAsNonRoot: true (and runAsUser: 1000 or a non-root UID),
allowPrivilegeEscalation: false, capabilities: drop: ["ALL"],
readOnlyRootFilesystem: true, and a seccompProfile (type: RuntimeDefault) so
both containers are constrained by seccomp; ensure these keys are placed under
each container spec (initContainers[] -> name: setup-tuf-data and containers[]
-> name: tuf-server) and adjust runAsUser if the busybox copy logic or nginx
needs a specific non-root UID.
Comment on lines
+21
to
+27
| resources: | ||
| - namespace.yaml | ||
| - tuf.yaml | ||
| # These two files are generated using `make tuf-yaml` from wiremock data. | ||
| # See also the hack/tuf/create-tuf-files.sh script in this directory. | ||
| - tuf-files-configmap.yaml | ||
| - tuf-root-configmap.yaml |
Contributor
There was a problem hiding this comment.
1. Acceptance go test now fails 🐞 Bug ⛯ Reliability
Acceptance tests run via the documented cd acceptance && go test ./... flow can fail because the kind cluster kustomization now includes hack/tuf, which references generated ConfigMap YAMLs that won’t exist unless make tuf-yaml has been run first. This yields a kustomize render error before tests even start.
Agent Prompt
### Issue description
Acceptance tests can fail when run directly from the `acceptance` module (`cd acceptance && go test ./...`) because `hack/tuf/kustomization.yaml` references `tuf-files-configmap.yaml` and `tuf-root-configmap.yaml`, which are generated by `make tuf-yaml` and are not present by default.
### Issue Context
`kind.Start()` renders `hack/test` via `kustomize.Render("test")`. `hack/test/kustomization.yaml` now includes `../tuf`, and `hack/tuf/kustomization.yaml` includes the generated YAMLs as resources. The Makefile’s `acceptance` target runs `make tuf-yaml`, but the acceptance README also documents running `go test` directly.
### Fix Focus Areas
- acceptance/kubernetes/kind/kind.go[281-296]
- hack/tuf/kustomization.yaml[21-27]
- acceptance/README.md[13-28]
- Makefile[122-162]
### Suggested fix approaches
1) Add a preflight check in `renderTestConfiguration` (or before calling it) that verifies the generated YAML files exist; if not, return a clear error instructing users to run `make tuf-yaml` (or `make acceptance`).
2) Alternatively (or additionally), update `acceptance/README.md` to explicitly require `make tuf-yaml` before `go test`, or add an automated generation step in the acceptance test harness (with clear logging).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
There's quite a lot here, but it's broken down into a (hopefully) sensible set of commits for your reviewing convenience.
Ref: https://issues.redhat.com/browse/EC-1695