Conversation
Implement generic signing and verification framework for ApplicationProfiles and SeccompProfiles using ECDSA P-256 signatures compatible with cosign. Key features: - SignableProfile interface for extensible profile type support - Keyless signing support using OIDC identity providers - Key-based signing for offline/air-gapped environments - Signature storage in profile metadata.annotations - CLI tool (sign-profile) for signing and verifying profiles - Automatic verification in ApplicationProfileCache on profile load Core implementation: - pkg/signature/ package with ECDSA P-256 signing/verification - Profile adapters for ApplicationProfile and SeccompProfile - EnableProfileVerification config flag (default: false) - On verification failure: skip profile with warning (don't crash) Files: - pkg/signature/annotations.go, interface.go, sign.go, verify.go - pkg/signature/cosign_adapter.go, signer.go, verifier.go - pkg/signature/profiles/applicationprofile_adapter.go - pkg/signature/profiles/seccompprofile_adapter.go - cmd/sign-profile/main.go - pkg/config/config.go - add EnableProfileVerification flag - pkg/objectcache/applicationprofilecache/ - integrate verification at load time - docs/signing/README.md - comprehensive documentation Tests: - 15 signature package tests covering signing, verification, tampering detection - 5 adapter tests for ApplicationProfile and SeccompProfile adapters - All tests passing Co-authored-by: Cerebras Agent <193945191+isaact-cerebras@users.noreply.github.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com> Co-authored-by: Cerebras Agent <193945191+isaact-cerebras@users.noreply.github.com> Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
pkg/signature/sign_test.go (1)
170-175: Test case name is misleading - profile has empty annotations, not nil.The test case "No annotations" uses
NewMockSignableProfilewhich initializesannotations: make(map[string]string). This creates an empty map, notnil. The test likely passes becauseDecodeSignatureFromAnnotationsfails on an empty map, but the test name suggests it's testing thenilcheck path.Consider either:
- Renaming to "Empty annotations", or
- Using
&MockSignableProfile{annotations: nil, ...}to truly test the nil case📝 Suggested clarification
{ - name: "No annotations", - profile: NewMockSignableProfile("uid", "ns", "name", map[string]string{}), + name: "Nil annotations", + profile: &MockSignableProfile{uid: "uid", namespace: "ns", name: "name", content: map[string]string{}, annotations: nil}, wantErr: true, setupSign: false, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signature/sign_test.go` around lines 170 - 175, The test case name "No annotations" is misleading because NewMockSignableProfile creates an empty map (annotations: make(map[string]string)); update the test to either rename the case to "Empty annotations" to reflect the actual input, or change the fixture to construct a MockSignableProfile with annotations set to nil (e.g., &MockSignableProfile{annotations: nil, ...}) so it exercises the nil-path in DecodeSignatureFromAnnotations; modify the entry that currently calls NewMockSignableProfile("uid","ns","name", map[string]string{}) to your chosen fix and update the test name accordingly.pkg/signature/verify.go (1)
29-49: Consider adding a comment explaining whyuseKeyless: trueis hardcoded for verification.Both
NewCosignAdapter(true)andNewCosignVerifier(true)use hardcodedtrue. This works because the verification uses the certificate stored in the annotations (from either keyless or key-based signing), but the reasoning isn't immediately obvious. A brief comment would improve clarity.📝 Suggested comment
+ // useKeyless=true is fine for verification since we use the certificate + // stored in the profile annotations, regardless of how the profile was signed adapter, err := NewCosignAdapter(true)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signature/verify.go` around lines 29 - 49, Add a short inline comment next to the hardcoded useKeyless=true calls to NewCosignAdapter(true) and NewCosignVerifier(true) explaining that verification uses the certificate embedded in the annotations (so keyless=true is safe/intentional here), and mention that signatures may come from either keyless or key-based signing but the certificate in annotations is used for verification; place this comment adjacent to the calls in VerifySignature (or the function containing adapter/ verifier creation) so future readers understand the rationale.pkg/objectcache/applicationprofilecache/applicationprofilecache.go (1)
249-264: Consider extracting verification logic into a helper method to reduce duplication.The same verification pattern is repeated three times (lines 249-264, 336-349, and 567-587). This could be extracted into a helper like
verifyProfileIfEnabled(profile *v1beta1.ApplicationProfile, name, namespace string) error.♻️ Proposed helper method
+// verifyApplicationProfile verifies the profile signature if verification is enabled. +// Returns error if verification fails, nil otherwise (including when verification is disabled). +func (apc *ApplicationProfileCacheImpl) verifyApplicationProfile(profile *v1beta1.ApplicationProfile, context string) error { + if !apc.cfg.EnableProfileVerification { + return nil + } + profileAdapter := profiles.NewApplicationProfileAdapter(profile) + if err := signature.VerifyProfile(profileAdapter); err != nil { + logger.L().Warning(context+" verification failed, skipping", + helpers.String("profile", profile.Name), + helpers.String("namespace", profile.Namespace), + helpers.Error(err)) + return err + } + logger.L().Debug(context+" verification successful", + helpers.String("profile", profile.Name), + helpers.String("namespace", profile.Namespace)) + return nil +}Then replace each verification block with:
if err := apc.verifyApplicationProfile(fullProfile, "profile"); err != nil { continue // or return, depending on context }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/applicationprofilecache/applicationprofilecache.go` around lines 249 - 264, Extract the repeated signature verification block into a method on applicationProfileCache (e.g., apc.verifyApplicationProfile) that checks apc.cfg.EnableProfileVerification, constructs profiles.NewApplicationProfileAdapter(fullProfile), calls signature.VerifyProfile, logs the same Warning (including profile.Name, namespace, workloadID when available) and Debug messages, and returns an error when verification fails; then replace the three inline blocks that use fullProfile/profile.Name/namespace/workloadID and signature.VerifyProfile with a single call to apc.verifyApplicationProfile(...) and act on its error (continue or return) as before.pkg/signature/verify_test.go (1)
171-177: Test assertions are too weak - they only log but don't verify expected behavior.The test checks if issuers/identities are the same and logs, but doesn't assert the expected difference between key-based (
local) and keyless signing. Based on other tests,SignProfileWithKeyshould produce issuerlocal, whileSignProfileKeylessproduces a different issuer.💚 Proposed fix with meaningful assertions
- if sig1.Issuer == sig2.Issuer && sig1.Issuer != "" { - t.Log("Both profiles have same issuer") - } - - if sig1.Identity == sig2.Identity && sig1.Identity != "" { - t.Log("Both profiles have same identity") - } + // Key-based signing should have "local" issuer + if sig1.Issuer != "local" { + t.Errorf("Expected key-based signing issuer 'local', got '%s'", sig1.Issuer) + } + + // Keyless signing should have a non-local issuer (or at least be different in test env) + // Note: In test environment, keyless may also use "local" - adjust assertion as needed + if sig1.Identity != "local-key" { + t.Errorf("Expected key-based signing identity 'local-key', got '%s'", sig1.Identity) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signature/verify_test.go` around lines 171 - 177, The test currently only logs when sig1.Issuer/sig1.Identity equals sig2.*; replace those logs with concrete assertions: verify that the profile signed with SignProfileWithKey (sig1) has Issuer == "local" and that the profile signed with SignProfileKeyless (sig2) does not have Issuer == "local" (use t.Fatalf/t.Errorf to fail the test on mismatch), and similarly assert expected differences for Identity (e.g., assert sig1.Identity is set to the local identity and sig2.Identity differs or is empty per the keyless behavior). Update the checks referencing sig1, sig2, Issuer, Identity, SignProfileWithKey, and SignProfileKeyless to assert equality/inequality rather than just t.Log.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/sign-profile/main.go`:
- Around line 99-103: The CLI currently requires --key (keyFile) but never uses
it when calling signature.SignProfileWithKey; update main.go so that when
useKeyless is false you load the private key from keyFile (or extend
signature.SignProfileWithKey to accept a key path) and pass the loaded key or
keyFile into the signing call; specifically, locate the validation block using
keyFile/useKeyless and the call site of signature.SignProfileWithKey and either
(A) change the call to signature.SignProfileWithKey(..., keyFile) if the API is
extended, or (B) load/parse the private key from keyFile in main.go and pass the
resulting signer/crypto.Value into signature.SignProfileWithKey so the CLI --key
flag actually affects signing.
- Around line 106-111: parseVerifyFlags (and the similar
parseExtractSignatureFlags) never initialize the profileType flag, leaving
profileType empty so the downstream check (profileType != "auto") forces
explicit parsing and yields "unknown profile type"; fix by adding a StringVar
for profileType with a sensible default (e.g., "auto") in parseVerifyFlags and
the corresponding parseExtractSignatureFlags so profileType is initialized
before the check, ensuring the branch that handles auto-detection is taken when
no explicit type is passed.
- Around line 124-128: The generate-keypair command declares publicOnly
(parseGenerateFlags) but never uses it and always writes sig.Certificate as a
single blob; update the command to explicitly output separate public and private
key files and honor the publicOnly flag: add/recognize distinct outputs (e.g.,
--output-private and --output-public or derive public file as output + ".pub"),
change the key generation/emit code that currently writes sig.Certificate to
instead write the private key when not publicOnly and always write the public
key (or only the public when publicOnly is true), and ensure the code paths
reference the existing symbols publicOnly, outputFile (or new
outputPrivate/outputPublic vars) and the created keypair (the
sig/Certificate/privateKey values) so the produced files match the flag
semantics and the generate-keypair contract.
- Around line 41-43: The bug is that when you rewrite the inferred subcommand
variable command to "sign" because the first arg starts with "-", you still pass
os.Args[2:] into the flag parsing logic which drops that first flag; update the
argument selection so that when command was set to "sign" due to a leading flag
you parse os.Args[1:] (include the original first arg), otherwise continue
parsing os.Args[2:]. Locate the code that sets command and the subsequent flag
parsing (references: the command variable and the flag parsing calls that
currently use os.Args[2:]), introduce a small helper like argsForCommand (or
inline conditional) to choose os.Args[1:] when command was rewritten and
os.Args[2:] otherwise, and apply the same fix to the other similar sites noted
(the other occurrences around the same logic).
In `@docs/signing/README.md`:
- Around line 326-329: The example Kubernetes manifest uses an invalid container
name "app容器" under the spec -> containers -> name field; replace it with a
DNS-label-compliant name (lowercase alphanumeric and hyphens) such as
"app-container", ensuring the value for the containers.name field matches the
Kubernetes naming rules (^[a-z0-9]([-a-z0-9]*[a-z0-9])?$) and contains only
ASCII lowercase letters, digits, and hyphens.
- Around line 444-448: The ordered list item header "2. **Sign Before
Applying**" (the line that begins with that text) is incorrectly numbered
duplicate; change its leading "2." to "3." so the sequence after the previous
"2." item is correct; update only the numeric prefix of the "Sign Before
Applying" list item to "3." to fix the numbering.
In `@pkg/objectcache/applicationprofilecache/applicationprofilecache.go`:
- Around line 249-264: The profile verification block (guarded by
apc.cfg.EnableProfileVerification) currently skips on VerifyProfile error
without updating the existing profileState set earlier (see variable
profileState), causing the cache to show a successful state despite the profile
not being loaded; modify the error branch inside the signature.VerifyProfile
check to update profileState with the verification error and appropriate failed
status/completion fields (mirroring how user-defined profiles are handled on
verification failure), log the error, persist the updated profileState, and then
continue to the next profile to ensure consistent state reporting.
In `@pkg/signature/cosign_adapter.go`:
- Around line 106-113: The current ecdsaSign implementation builds the signature
by appending r.Bytes() and s.Bytes(), which strips leading zeros and breaks the
verifier that expects fixed 32-byte components; replace this with ASN.1 DER
encoding: add the "encoding/asn1" import, declare type ecdsaSignature struct {
R, S *big.Int }, change CosignAdapter.ecdsaSign to return
asn1.Marshal(ecdsaSignature{R: r, S: s}), and update the verification logic in
the same file (the code that currently slices the signature into fixed 32-byte
r/s) to use asn1.Unmarshal into ecdsaSignature and use the unmarshalled R and S
for verification.
- Around line 120-152: The code currently accepts an attacker-controlled public
key from sig.Certificate (used by cryptoutils.UnmarshalPEMToPublicKey) and only
checks sig.Issuer/sig.Identity when useKeyless is set, so fix by requiring and
validating an x509 certificate chain and trust anchor instead of directly
unmarshaling a raw public key: parse sig.Certificate as an x509.Certificate,
verify its chain/signature against configured trust roots (e.g., Fulcio root or
pinned CA) and certificate transparency bindings (Rekor/SCT) before extracting
the public key, ensure the extracted key matches the certificate public key used
for ecdsa.Verify, and replace the simple non-empty checks on
sig.Issuer/sig.Identity with authoritative checks that the certificate
SAN/subject matches expected pinned identities; if allowUntrusted is false,
reject any annotation-supplied/self-signed key material outright in the code
paths around cryptoutils.UnmarshalPEMToPublicKey, ecdsa.Verify, useKeyless,
sig.Issuer, and sig.Identity.
In `@pkg/signature/profiles/adapter_test.go`:
- Around line 274-278: The test currently ignores errors from
signature.SignProfileWithKey and signature.GetProfileSignature; change the calls
to capture their returned errors and fail the test if any occur (e.g., err !=
nil => t.Fatalf or require.NoError) before using apSig/spSig, and also assert
that apSig and spSig are non-nil/valid; update the calls to
signature.SignProfileWithKey(apAdapter) and
signature.SignProfileWithKey(spAdapter) and to
signature.GetProfileSignature(apAdapter)/GetProfileSignature(spAdapter) to check
and handle the returned errors immediately to avoid panics and misleading
assertions.
In `@pkg/signature/sign.go`:
- Around line 52-57: The current code replaces all profile annotations by
calling profile.SetAnnotations(annotations) with only the signature annotations
returned from adapter.EncodeSignatureToAnnotations(sig); instead, retrieve the
existing annotations (e.g., profile.GetAnnotations()), merge the signature
annotations into that map (overwriting keys from the new signature where
necessary) and then call profile.SetAnnotations(mergedAnnotations) so existing
metadata like kubescape.io/wlid and kubescape.io/status are preserved; locate
the EncodeSignatureToAnnotations call, the annotations variable, and the
profile.SetAnnotations invocation to implement this merge.
---
Nitpick comments:
In `@pkg/objectcache/applicationprofilecache/applicationprofilecache.go`:
- Around line 249-264: Extract the repeated signature verification block into a
method on applicationProfileCache (e.g., apc.verifyApplicationProfile) that
checks apc.cfg.EnableProfileVerification, constructs
profiles.NewApplicationProfileAdapter(fullProfile), calls
signature.VerifyProfile, logs the same Warning (including profile.Name,
namespace, workloadID when available) and Debug messages, and returns an error
when verification fails; then replace the three inline blocks that use
fullProfile/profile.Name/namespace/workloadID and signature.VerifyProfile with a
single call to apc.verifyApplicationProfile(...) and act on its error (continue
or return) as before.
In `@pkg/signature/sign_test.go`:
- Around line 170-175: The test case name "No annotations" is misleading because
NewMockSignableProfile creates an empty map (annotations:
make(map[string]string)); update the test to either rename the case to "Empty
annotations" to reflect the actual input, or change the fixture to construct a
MockSignableProfile with annotations set to nil (e.g.,
&MockSignableProfile{annotations: nil, ...}) so it exercises the nil-path in
DecodeSignatureFromAnnotations; modify the entry that currently calls
NewMockSignableProfile("uid","ns","name", map[string]string{}) to your chosen
fix and update the test name accordingly.
In `@pkg/signature/verify_test.go`:
- Around line 171-177: The test currently only logs when
sig1.Issuer/sig1.Identity equals sig2.*; replace those logs with concrete
assertions: verify that the profile signed with SignProfileWithKey (sig1) has
Issuer == "local" and that the profile signed with SignProfileKeyless (sig2)
does not have Issuer == "local" (use t.Fatalf/t.Errorf to fail the test on
mismatch), and similarly assert expected differences for Identity (e.g., assert
sig1.Identity is set to the local identity and sig2.Identity differs or is empty
per the keyless behavior). Update the checks referencing sig1, sig2, Issuer,
Identity, SignProfileWithKey, and SignProfileKeyless to assert
equality/inequality rather than just t.Log.
In `@pkg/signature/verify.go`:
- Around line 29-49: Add a short inline comment next to the hardcoded
useKeyless=true calls to NewCosignAdapter(true) and NewCosignVerifier(true)
explaining that verification uses the certificate embedded in the annotations
(so keyless=true is safe/intentional here), and mention that signatures may come
from either keyless or key-based signing but the certificate in annotations is
used for verification; place this comment adjacent to the calls in
VerifySignature (or the function containing adapter/ verifier creation) so
future readers understand the rationale.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/sign-profile/main.godocs/signing/README.mdgo.modpkg/config/config.gopkg/objectcache/applicationprofilecache/applicationprofilecache.gopkg/signature/annotations.gopkg/signature/cosign_adapter.gopkg/signature/interface.gopkg/signature/profiles/adapter_test.gopkg/signature/profiles/applicationprofile_adapter.gopkg/signature/profiles/seccompprofile_adapter.gopkg/signature/sign.gopkg/signature/sign_test.gopkg/signature/signer.gopkg/signature/verifier.gopkg/signature/verify.gopkg/signature/verify_test.go
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
cmd/sign-profile/main.go (1)
48-53:⚠️ Potential issue | 🔴 CriticalDefault
sign-profile --...still drops the first flag.Line 87 always parses
os.Args[2:]. When command is inferred (Line 43path), the first real flag is atos.Args[1], so it gets skipped.🐛 Proposed fix
-func parseSignFlags() { +func parseSignFlags(args []string) { fs := flag.NewFlagSet("sign-profile sign", flag.ExitOnError) @@ - if err := fs.Parse(os.Args[2:]); err != nil { + if err := fs.Parse(args); err != nil { fmt.Fprintf(os.Stderr, "Error parsing flags: %v\n", err) os.Exit(1) } @@ switch command { case "sign", "": - parseSignFlags() - if argsRewritten { - os.Args = append([]string{"sign-profile"}, os.Args[1:]...) - } + if argsRewritten { + parseSignFlags(os.Args[1:]) + } else { + parseSignFlags(os.Args[2:]) + }Also applies to: 78-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/sign-profile/main.go` around lines 48 - 53, The bug is that when command is inferred (switch case "sign",""), parseSignFlags() is called while os.Args still contains the original argv, then later code unshifts "sign-profile" (argsRewritten path) causing the first real flag to be dropped because parsing uses os.Args[2:]; fix by ensuring the argv mutation happens before parsing or by parsing the correct slice based on argsRewritten: move the os.Args = append([]string{"sign-profile"}, os.Args[1:]...) line to occur before calling parseSignFlags(), or change parseSignFlags() invocation to pass the correct args (use os.Args[1:] when argsRewritten is false and os.Args[2:] when a command token is present) so that parseSignFlags() sees the first flag whether command was explicit or inferred; update references to parseSignFlags, argsRewritten, and the switch handling accordingly.docs/signing/README.md (1)
450-459:⚠️ Potential issue | 🟡 MinorFix ordered-list numbering continuity in Best Practices.
Line 450 starts another
3.after Line 445 already used3.. Please renumber this sequence to keep it consistent.📝 Suggested fix
-3. **Version Your Profiles** +4. **Version Your Profiles** - Include version in metadata - Old signatures become invalid on content changes -4. **Key Management for Local Signing** +5. **Key Management for Local Signing** - Store keys in secure locations (HSM, KMS) - Rotate keys regularly - Use read-only keys for verification -5. **Audit Trail** +6. **Audit Trail** - Store signing timestamps - Track who signed what - Use GitHub Actions for audit logs🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/signing/README.md` around lines 450 - 459, The ordered list numbering in the "Best Practices" section of docs/signing/README.md is inconsistent—after the first "3." the subsequent list item "3. **Version Your Profiles**" should be renumbered to continue the sequence; update the numeric markers for "Version Your Profiles", "Key Management for Local Signing", and "Audit Trail" so they follow the previous item (e.g., 4., 5., 6.) to restore correct ordered-list continuity.pkg/signature/cosign_adapter.go (1)
202-227:⚠️ Potential issue | 🔴 CriticalStrict verification still accepts attacker-controlled certificate material.
In Line 215-223, strict mode only performs local certificate sanity checks (CA/CN/time) and does not verify certificate trust chains. Line 253-255 also treats
sig.Issuer/sig.Identityas trusted if merely non-empty. This still allows self-consistent forged profile+cert+signature bundles to pass.🔧 Suggested direction (trust-anchor + claim binding)
@@ if !allowUntrusted { - if cert.IsCA || cert.Subject.CommonName == "" { - return fmt.Errorf("invalid certificate: must not be CA and must have a valid subject") - } - - if time.Now().Before(cert.NotBefore) || time.Now().After(cert.NotAfter) { - return fmt.Errorf("certificate is not valid at this time") - } + roots, err := x509.SystemCertPool() // preferably configured Fulcio/pinned roots + if err != nil || roots == nil { + return fmt.Errorf("failed to load trust roots: %w", err) + } + if _, err := cert.Verify(x509.VerifyOptions{ + Roots: roots, + CurrentTime: time.Now(), + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, + }); err != nil { + return fmt.Errorf("certificate chain verification failed: %w", err) + } + + if c.useKeyless { + if sig.Identity == "" || cert.Subject.CommonName != sig.Identity { + return fmt.Errorf("identity mismatch between signature metadata and certificate") + } + if sig.Issuer == "" || cert.Issuer.CommonName != sig.Issuer { + return fmt.Errorf("issuer mismatch between signature metadata and certificate") + } + } }Also applies to: 252-256
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signature/cosign_adapter.go` around lines 202 - 227, The current strict path parses sig.Certificate but only runs local sanity checks (IsCA, CommonName, dates) and later trusts sig.Issuer/sig.Identity if non-empty; fix by performing a full x509 chain verification against a configured set of trust anchors and intermediates (use x509.Verify with proper Roots and Intermediates and time check) inside the block that handles sig.Certificate when allowUntrusted is false, and require that x509.Verify succeeds; additionally, do not accept sig.Issuer or sig.Identity as authoritative unless they are derived from the verified certificate (e.g., compare sig.Issuer/sig.Identity to cert.Subject/subjectAltName or parsed OID claims from cert) so only values bound to a certificate that successfully verified are trusted.
🧹 Nitpick comments (2)
pkg/objectcache/applicationprofilecache/applicationprofilecache.go (1)
252-258: Extract repeated “verification-failed” state updates into one helper.These three blocks duplicate the same state-shaping logic. Centralizing it will reduce drift and keep status semantics consistent across paths.
Also applies to: 358-365, 589-596
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/objectcache/applicationprofilecache/applicationprofilecache.go` around lines 252 - 258, Multiple code paths repeat creating an objectcache.ProfileState with Completion:"failed", Status:"verification-failed", Name:profile.Name and Error:err and then calling apc.workloadIDToProfileState.Set(workloadID, profileState); factor this into a single helper on the receiver (e.g. func (apc *ApplicationProfileCache) setVerificationFailed(workloadID string, profile *ProfileType, err error)) that constructs the ProfileState and calls apc.workloadIDToProfileState.Set; replace the three duplicated blocks (the occurrences around the current diff and at lines ~358-365 and ~589-596) with calls to that helper to keep status shaping consistent.pkg/signature/cosign_adapter.go (1)
314-319: Use strict integer parsing for timestamp annotations.
fmt.Sscanfin Line 316 can accept partial numeric prefixes (e.g.,"123abc").strconv.ParseIntis stricter for annotation validation.🔧 Suggested refactor
import ( @@ "math/big" + "strconv" "time" @@ if timestamp, ok := annotations[AnnotationTimestamp]; ok { - var ts int64 - _, err = fmt.Sscanf(timestamp, "%d", &ts) + ts, err := strconv.ParseInt(timestamp, 10, 64) if err != nil { return nil, fmt.Errorf("failed to parse timestamp: %w", err) } sig.Timestamp = ts }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/signature/cosign_adapter.go` around lines 314 - 319, The code currently uses fmt.Sscanf to parse the AnnotationTimestamp value (see AnnotationTimestamp handling and the local variable ts), which allows partial numeric parsing; replace that with strconv.ParseInt on the timestamp string (use base 10 and 64-bit) and assign the parsed int64 to ts, returning a wrapped error if ParseInt fails to ensure strict integer validation of the annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/config.go`:
- Line 184: The config key enableProfileVerification set with viper.SetDefault
isn't bound to the documented ENABLE_PROFILE_VERIFICATION env var because
viper.AutomaticEnv() doesn't map camelCase to SCREAMING_SNAKE_CASE; update the
config initialization to explicitly bind the env var (e.g., call
viper.BindEnv("enableProfileVerification", "ENABLE_PROFILE_VERIFICATION")) or
configure an env key replacer (via viper.SetEnvKeyReplacer) before
viper.AutomaticEnv() so that the enableProfileVerification key correctly picks
up ENABLE_PROFILE_VERIFICATION from the environment.
In `@pkg/signature/cosign_adapter.go`:
- Around line 241-247: The code calling asn1.Unmarshal into ecdsaSignature
ignores the returned rest and doesn't validate R/S before calling ecdsa.Verify;
change the asn1.Unmarshal call to capture the rest value and return an error if
len(rest) != 0, and add checks that ecdsaSig.R and ecdsaSig.S are non-nil (and
return an error if they are) before invoking ecdsa.Verify with ecdsaPubKey,
digestBytes and ecdsaSig.R/S; this ensures strict DER parsing and avoids nil
dereferences.
In `@pkg/signature/profiles/applicationprofile_adapter.go`:
- Around line 18-23: GetAnnotations on ApplicationProfileAdapter returns a newly
created map when profile.Annotations is nil but does not persist it, so callers'
mutations are lost; change GetAnnotations to allocate a new map when
a.profile.Annotations == nil, assign it back to a.profile.Annotations (so the
profile holds the map) and then return it, referencing the GetAnnotations
method, the ApplicationProfileAdapter receiver a, the profile.Annotations field,
and the existing SetAnnotations behavior if needed for consistency.
In `@pkg/signature/profiles/seccompprofile_adapter.go`:
- Around line 18-23: GetAnnotations currently returns a newly allocated map when
s.profile.Annotations is nil but does not attach it to the adapter, so mutations
are lost; update SeccompProfileAdapter.GetAnnotations to initialize
s.profile.Annotations in-place (set s.profile.Annotations =
make(map[string]string) when nil) and then return s.profile.Annotations so
callers can safely mutate the returned map directly.
In `@pkg/signature/sign_test.go`:
- Around line 196-199: The test setup calls SignProfileKeyless(tt.profile) but
ignores its returned error; update the setup branch where tt.setupSign is true
to capture and assert the error so failures surface immediately (e.g., err :=
SignProfileKeyless(tt.profile) and fail the test on non-nil error using the test
helper in scope such as t.Fatalf/t.Fatalf-like assertion or require.NoError).
Ensure the alternative path with tt.setupAnnotations remains unchanged and use
the same t instance for error reporting.
In `@pkg/signature/sign.go`:
- Around line 11-35: SignProfile currently dereferences the incoming profile
(calls profile.GetContent()) without validating it, which panics on a nil input;
update SignProfile to validate that the profile argument is not nil at the start
and return a clear error (e.g., fmt.Errorf("nil profile")) instead of
proceeding, and also ensure any subsequent use of profile (GetContent) is only
after that nil-check; reference the SignProfile function and the
SignableProfile/GetContent call when making the change.
In `@pkg/signature/verifier.go`:
- Around line 18-24: The Verify and VerifyAllowUntrusted methods on
CosignVerifier dereference v.adapter without nil-checking; update both methods
(Verify and VerifyAllowUntrusted) to first verify that v and v.adapter are
non-nil and return a clear error (e.g., "uninitialized CosignVerifier adapter"
or similar) if they are nil, otherwise call v.adapter.VerifyData(data, sig,
...); reference CosignVerifier, Verify, VerifyAllowUntrusted, and
adapter.VerifyData when applying the change.
In `@pkg/signature/verify.go`:
- Around line 11-23: The VerifyProfile entrypoint assumes the incoming profile
is non-nil and can panic when calling profile.GetAnnotations(); add a nil guard
at the start of VerifyProfile to validate the profile argument (e.g., if profile
== nil) and return a clear error before proceeding, so subsequent calls like
profile.GetAnnotations() are safe; update any tests or callers if needed to
handle the new error path.
---
Duplicate comments:
In `@cmd/sign-profile/main.go`:
- Around line 48-53: The bug is that when command is inferred (switch case
"sign",""), parseSignFlags() is called while os.Args still contains the original
argv, then later code unshifts "sign-profile" (argsRewritten path) causing the
first real flag to be dropped because parsing uses os.Args[2:]; fix by ensuring
the argv mutation happens before parsing or by parsing the correct slice based
on argsRewritten: move the os.Args = append([]string{"sign-profile"},
os.Args[1:]...) line to occur before calling parseSignFlags(), or change
parseSignFlags() invocation to pass the correct args (use os.Args[1:] when
argsRewritten is false and os.Args[2:] when a command token is present) so that
parseSignFlags() sees the first flag whether command was explicit or inferred;
update references to parseSignFlags, argsRewritten, and the switch handling
accordingly.
In `@docs/signing/README.md`:
- Around line 450-459: The ordered list numbering in the "Best Practices"
section of docs/signing/README.md is inconsistent—after the first "3." the
subsequent list item "3. **Version Your Profiles**" should be renumbered to
continue the sequence; update the numeric markers for "Version Your Profiles",
"Key Management for Local Signing", and "Audit Trail" so they follow the
previous item (e.g., 4., 5., 6.) to restore correct ordered-list continuity.
In `@pkg/signature/cosign_adapter.go`:
- Around line 202-227: The current strict path parses sig.Certificate but only
runs local sanity checks (IsCA, CommonName, dates) and later trusts
sig.Issuer/sig.Identity if non-empty; fix by performing a full x509 chain
verification against a configured set of trust anchors and intermediates (use
x509.Verify with proper Roots and Intermediates and time check) inside the block
that handles sig.Certificate when allowUntrusted is false, and require that
x509.Verify succeeds; additionally, do not accept sig.Issuer or sig.Identity as
authoritative unless they are derived from the verified certificate (e.g.,
compare sig.Issuer/sig.Identity to cert.Subject/subjectAltName or parsed OID
claims from cert) so only values bound to a certificate that successfully
verified are trusted.
---
Nitpick comments:
In `@pkg/objectcache/applicationprofilecache/applicationprofilecache.go`:
- Around line 252-258: Multiple code paths repeat creating an
objectcache.ProfileState with Completion:"failed", Status:"verification-failed",
Name:profile.Name and Error:err and then calling
apc.workloadIDToProfileState.Set(workloadID, profileState); factor this into a
single helper on the receiver (e.g. func (apc *ApplicationProfileCache)
setVerificationFailed(workloadID string, profile *ProfileType, err error)) that
constructs the ProfileState and calls apc.workloadIDToProfileState.Set; replace
the three duplicated blocks (the occurrences around the current diff and at
lines ~358-365 and ~589-596) with calls to that helper to keep status shaping
consistent.
In `@pkg/signature/cosign_adapter.go`:
- Around line 314-319: The code currently uses fmt.Sscanf to parse the
AnnotationTimestamp value (see AnnotationTimestamp handling and the local
variable ts), which allows partial numeric parsing; replace that with
strconv.ParseInt on the timestamp string (use base 10 and 64-bit) and assign the
parsed int64 to ts, returning a wrapped error if ParseInt fails to ensure strict
integer validation of the annotation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
cmd/sign-profile/main.godocs/signing/README.mdgo.modpkg/config/config.gopkg/objectcache/applicationprofilecache/applicationprofilecache.gopkg/signature/annotations.gopkg/signature/cosign_adapter.gopkg/signature/interface.gopkg/signature/profiles/adapter_test.gopkg/signature/profiles/applicationprofile_adapter.gopkg/signature/profiles/seccompprofile_adapter.gopkg/signature/sign.gopkg/signature/sign_test.gopkg/signature/signer.gopkg/signature/verifier.gopkg/signature/verify.gopkg/signature/verify_test.go
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Contributor
Author
|
@coderabbitai please close all addressed comments. |
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
|
✅ Actions performedComments resolved. Auto-approval is disabled; enable |
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
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.
Implement generic signing and verification framework for ApplicationProfiles and SeccompProfiles using ECDSA P-256 signatures compatible with cosign.
Key features:
Core implementation:
Files:
Tests:
Overview
Summary by CodeRabbit
New Features
Documentation
Chores