diff --git a/app/cli/internal/policydevel/eval.go b/app/cli/internal/policydevel/eval.go index d18b5d7c1..ddf4c208d 100644 --- a/app/cli/internal/policydevel/eval.go +++ b/app/cli/internal/policydevel/eval.go @@ -100,7 +100,8 @@ func Evaluate(opts *EvalOptions, logger zerolog.Logger) (*EvalSummary, error) { // crafter produced, rather than replacing them. // // The crafter's annotations carry more than metadata: chainloop.material.redacted -// is how a policy learns that secrets were found and stripped out, and it is what +// is how a policy learns that the content was scanned for secrets and any found +// were stripped out, and it is what // makes content resolution fail closed rather than fall back to the un-redacted // file on disk. Dropping it would hide the redaction from the policy and re-open // the path this exists to close. diff --git a/app/cli/internal/policydevel/eval_test.go b/app/cli/internal/policydevel/eval_test.go index 960ab432c..65dd4c1c6 100644 --- a/app/cli/internal/policydevel/eval_test.go +++ b/app/cli/internal/policydevel/eval_test.go @@ -238,13 +238,14 @@ func TestEvaluateSimplifiedPolicies(t *testing.T) { const fixtureGitHubPAT = "ghp_erOZlZv0B1e3amrQ" + "ugdwZ8Ro2W4kDql9WPTf" // writeSessionFixture materialises an AI coding session fixture with its -// credential placeholder resolved, so that the crafter sees a real secret on disk. -func writeSessionFixture(t *testing.T) string { +// credential placeholder resolved to token: fixtureGitHubPAT for the crafter to +// see a real secret on disk, anything else for a clean session. +func writeSessionFixture(t *testing.T, token string) string { t.Helper() content, err := os.ReadFile("testdata/ai-coding-session-with-secret.json") require.NoError(t, err) - content = bytes.ReplaceAll(content, []byte("__GITHUB_PAT__"), []byte(fixtureGitHubPAT)) + content = bytes.ReplaceAll(content, []byte("__GITHUB_PAT__"), []byte(token)) path := filepath.Join(t.TempDir(), "ai-coding-session.json") require.NoError(t, os.WriteFile(path, content, 0600)) @@ -279,7 +280,7 @@ func TestEvaluateReadsRedactedMaterial(t *testing.T) { opts := &EvalOptions{ PolicyPath: "testdata/ai-coding-session-no-secrets-policy.yaml", MaterialKind: "CHAINLOOP_AI_CODING_SESSION", - MaterialPath: writeSessionFixture(t), + MaterialPath: writeSessionFixture(t, fixtureGitHubPAT), Annotations: tc.annotations, // Debug surfaces the exact bytes handed to the engine, which is // what the strongest assertion below inspects. @@ -307,6 +308,35 @@ func TestEvaluateReadsRedactedMaterial(t *testing.T) { } } +// A session the scan found nothing in is still marked redacted, as `attestation +// add` marks it, and the marker must not make evaluation fail closed: the policy +// is evaluated against the scanned content. +func TestEvaluateCleanScannedMaterial(t *testing.T) { + const token = "not-a-credential" + + opts := &EvalOptions{ + PolicyPath: "testdata/ai-coding-session-no-secrets-policy.yaml", + MaterialKind: "CHAINLOOP_AI_CODING_SESSION", + MaterialPath: writeSessionFixture(t, token), + Debug: true, + } + + result, err := Evaluate(opts, zerolog.New(os.Stderr)) + require.NoError(t, err) + require.NotNil(t, result) + + assert.False(t, result.Result.Skipped) + assert.Empty(t, result.Result.Violations) + + require.NotNil(t, result.DebugInfo) + require.NotEmpty(t, result.DebugInfo.Inputs) + for _, input := range result.DebugInfo.Inputs { + assert.Contains(t, string(input), token) + assert.Contains(t, string(input), `"chainloop.material.redacted":"true"`) + assert.Contains(t, string(input), `"chainloop.material.redaction.count":"0"`) + } +} + func TestMergeAnnotations(t *testing.T) { testCases := []struct { name string diff --git a/pkg/attestation/crafter/api/attestation/v1/crafting_state.go b/pkg/attestation/crafter/api/attestation/v1/crafting_state.go index 23cff7381..cb623d993 100644 --- a/pkg/attestation/crafter/api/attestation/v1/crafting_state.go +++ b/pkg/attestation/crafter/api/attestation/v1/crafting_state.go @@ -59,14 +59,16 @@ var ( AnnotationsSBOMMainComponentType = CreateAnnotation("material.sbom.main_component.type") AnnotationsSBOMMainComponentVersion = CreateAnnotation("material.sbom.main_component.version") - // AnnotationMaterialRedacted marks a material whose stored content was - // transformed by its crafter before upload, to strip secrets out of it. Two - // things follow from it: the recorded digest describes the redacted artifact - // rather than the file on disk, and policy evaluation must be handed that - // sanitized copy explicitly, because the file on disk still holds the secrets - // (see GetEvaluableContent, which fails closed without it). + // AnnotationMaterialRedacted marks a material whose content went through + // secret redaction before upload, whether or not anything was found. When + // something was replaced (AnnotationMaterialRedactionCount above zero) the + // recorded digest describes the redacted artifact rather than the file on + // disk, which still holds the secrets. Either way policy evaluation must be + // handed the scanned copy explicitly (see GetEvaluableContent, which fails + // closed without it). AnnotationMaterialRedacted = CreateAnnotation("material.redacted") - // AnnotationMaterialRedactionCount is how many secrets were replaced. + // AnnotationMaterialRedactionCount is how many secrets were replaced, zero + // for a scan that found nothing. AnnotationMaterialRedactionCount = CreateAnnotation("material.redaction.count") // AnnotationMaterialRedactionRules lists the detection rules that matched, // so a policy can act on the kind of credential that was present. diff --git a/pkg/attestation/crafter/crafter.go b/pkg/attestation/crafter/crafter.go index b69a6d948..03f997054 100644 --- a/pkg/attestation/crafter/crafter.go +++ b/pkg/attestation/crafter/crafter.go @@ -821,9 +821,9 @@ func (c *Crafter) stageMaterial(ctx context.Context, m *schemaapi.CraftingSchema policies.WithDefaultGate(c.CraftingState.Attestation.GetBlockOnPolicyViolation()), policies.WithProjectContext(projectName, projectVersion), ) - // crafted.Content is what a crafter that did not store the artifact verbatim - // stored in its place (an AI coding session with secrets redacted out of it), - // and it is what the policies must see. Reading the file at value instead would + // crafted.Content is what a crafter that transformed the artifact scanned and + // stored (an AI coding session, with any secrets redacted out of it), and it + // is what the policies must see. Reading the file at value instead would // feed user-authored Rego the very secrets redaction removed. nil for every // other material, which resolves its content the usual way. policyGroupResults, err := pgv.VerifyMaterial(ctx, mt, value, crafted.Content) diff --git a/pkg/attestation/crafter/crafter_test.go b/pkg/attestation/crafter/crafter_test.go index cede069ca..842f1bf39 100644 --- a/pkg/attestation/crafter/crafter_test.go +++ b/pkg/attestation/crafter/crafter_test.go @@ -20,6 +20,8 @@ import ( "archive/zip" "compress/gzip" "context" + "crypto/sha256" + "encoding/hex" "fmt" "maps" "os" @@ -811,6 +813,42 @@ func (s *crafterSuite) TestAddMaterialRedactedSessionIsWhatPoliciesSee() { assert.Contains(s.T(), string(onDisk), githubPAT) } +// TestAddMaterialCleanSessionIsMarkedScanned covers a session the scan found +// nothing in. It is marked redacted, so that policies can tell it apart from a +// session that was never scanned, and that marker must not make evaluation fail +// closed: the crafter hands the scanned bytes to the policy engine. +func (s *crafterSuite) TestAddMaterialCleanSessionIsMarkedScanned() { + sessionPath := materializeSessionFixture(s.T(), "./materials/testdata/ai-coding-session.json") + onDisk, err := os.ReadFile(sessionPath) + require.NoError(s.T(), err) + + uploader := mUploader.NewUploader(s.T()) + uploader.On("Upload", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(&casclient.UpDownStatus{Digest: "deadbeef", Filename: "ai-coding-session.json"}, nil) + backend := &casclient.CASBackend{Uploader: uploader} + + c, err := newInitializedCrafter(s.T(), "testdata/contracts/with_ai_session_policy.yaml", + &v1.WorkflowMetadata{}, false, "", runners.NewGeneric()) + require.NoError(s.T(), err) + + m, err := c.AddMaterialContractFree(context.Background(), "", + schemaapi.CraftingSchema_Material_CHAINLOOP_AI_CODING_SESSION.String(), + "ai-session", sessionPath, backend, nil) + require.NoError(s.T(), err) + + assert.Equal(s.T(), v1.AnnotationValueTrue, m.Annotations[v1.AnnotationMaterialRedacted]) + assert.Equal(s.T(), "0", m.Annotations[v1.AnnotationMaterialRedactionCount]) + assert.NotContains(s.T(), m.Annotations, v1.AnnotationMaterialRedactionSkipped) + + // Nothing was replaced, so the digest is still that of the source file. + sum := sha256.Sum256(onDisk) + assert.Equal(s.T(), "sha256:"+hex.EncodeToString(sum[:]), m.GetArtifact().GetDigest()) + + evaluations := c.CraftingState.Attestation.PolicyEvaluations + require.Len(s.T(), evaluations, 1) + assert.Empty(s.T(), evaluations[0].Violations) +} + // materializeSessionFixture writes a copy of an AI coding session fixture with // its credential placeholders resolved, and returns its path. The crafter reads // the artifact from disk, so the substitution has to land in a real file. diff --git a/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go b/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go index 0b9c0ab8d..891592063 100644 --- a/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go +++ b/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go @@ -79,7 +79,7 @@ func NewChainloopAICodingSessionCrafter(schema *schemaapi.CraftingSchema_Materia // material definition. // // The file on disk is left untouched, so it is no longer the stored content once -// anything was redacted. The sanitized copy is returned as CraftResult.Content +// anything was redacted. The scanned copy is returned as CraftResult.Content // for whoever needs to read the artifact back — today policy evaluation, which // must not be handed the credentials the session captured. func (c *ChainloopAICodingSessionCrafter) Craft(ctx context.Context, artifactPath string) (*CraftResult, error) { @@ -115,7 +115,7 @@ func (c *ChainloopAICodingSessionCrafter) Craft(ctx context.Context, artifactPat return nil, fmt.Errorf("AI coding session validation failed: %w", err) } - redacted, report, err := c.redact(ctx, f) + scanned, report, err := c.redact(ctx, f) if err != nil { return nil, err } @@ -123,8 +123,8 @@ func (c *ChainloopAICodingSessionCrafter) Craft(ctx context.Context, artifactPat // Substituting the stored content only when something was actually replaced // keeps a clean session's digest reproducible from its source file. var craftOpts []uploadAndCraftOption - if redacted != nil { - craftOpts = append(craftOpts, withContentOverride(redacted)) + if report.Changed() { + craftOpts = append(craftOpts, withContentOverride(scanned)) } material, err := uploadAndCraft(ctx, c.input, c.backend, artifactPath, c.logger, craftOpts...) @@ -147,12 +147,12 @@ func (c *ChainloopAICodingSessionCrafter) Craft(ctx context.Context, artifactPat // Surface how the session was run material.Annotations[annotationAICodingSessionMode] = aicodingsession.ResolveMode(data.Session.Mode) - return &CraftResult{Material: material, Content: redacted}, nil + return &CraftResult{Material: material, Content: scanned}, nil } -// redact strips secrets out of the session content, returning the sanitized copy -// to store in place of the file on disk, or nil when nothing was replaced and the -// file itself is what gets stored. +// redact strips secrets out of the session content and returns the scanned +// copy: the sanitized bytes when something was replaced, the content itself when +// the scan found nothing, nil when redaction is skipped. // // Redaction fails closed: if the content cannot be scanned or the result no // longer matches the schema, the material is not crafted at all rather than @@ -170,7 +170,7 @@ func (c *ChainloopAICodingSessionCrafter) redact(ctx context.Context, content [] } if !report.Changed() { - return nil, report, nil + return content, report, nil } return redacted, report, nil @@ -178,6 +178,8 @@ func (c *ChainloopAICodingSessionCrafter) redact(ctx context.Context, content [] // annotateRedaction records what redaction did, so that it is visible in the // attestation and actionable by policies rather than an invisible rewrite. +// The material is marked redacted whenever the scan ran, even when it found +// nothing (see api.AnnotationMaterialRedacted). func (c *ChainloopAICodingSessionCrafter) annotateRedaction(material *api.Attestation_Material, report *redaction.Report) { if c.skipRedaction { material.Annotations[api.AnnotationMaterialRedactionSkipped] = api.AnnotationValueTrue @@ -196,13 +198,14 @@ func (c *ChainloopAICodingSessionCrafter) annotateRedaction(material *api.Attest Msg("some detected secrets could not be redacted") } + material.Annotations[api.AnnotationMaterialRedacted] = api.AnnotationValueTrue + material.Annotations[api.AnnotationMaterialRedactionCount] = strconv.Itoa(report.Replacements) + if !report.Changed() { return } rules := report.RuleIDs() - material.Annotations[api.AnnotationMaterialRedacted] = api.AnnotationValueTrue - material.Annotations[api.AnnotationMaterialRedactionCount] = strconv.Itoa(report.Replacements) material.Annotations[api.AnnotationMaterialRedactionRules] = strings.Join(rules, ",") c.logger.Info().Int("count", report.Replacements).Strs("rules", rules). diff --git a/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go b/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go index 087e2f72a..3837cb16f 100644 --- a/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go +++ b/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go @@ -187,6 +187,8 @@ func TestChainloopAICodingSessionCrafterRedaction(t *testing.T) { const ( withSecrets = "./aicodingsession/testdata/session-with-secrets.json" clean = "./testdata/ai-coding-session.json" + // The rules that match the secrets in withSecrets. + withSecretsRules = "anthropic-api-key,aws-access-token,aws-secret-access-key,github-pat" ) testCases := []struct { @@ -195,16 +197,14 @@ func TestChainloopAICodingSessionCrafterRedaction(t *testing.T) { skipRedaction bool inlineBackend bool skipUpload bool - wantRedacted bool wantCount string wantRules string }{ { - name: "secrets are stripped before upload", - filePath: withSecrets, - wantRedacted: true, - wantCount: "7", - wantRules: "anthropic-api-key,aws-access-token,aws-secret-access-key,github-pat", + name: "secrets are stripped before upload", + filePath: withSecrets, + wantCount: "7", + wantRules: withSecretsRules, }, { // An inline backend embeds the content into the attestation itself, @@ -212,19 +212,17 @@ func TestChainloopAICodingSessionCrafterRedaction(t *testing.T) { name: "an inline backend embeds the redacted copy", filePath: withSecrets, inlineBackend: true, - wantRedacted: true, wantCount: "7", - wantRules: "anthropic-api-key,aws-access-token,aws-secret-access-key,github-pat", + wantRules: withSecretsRules, }, { // Neither uploaded nor stored inline, so the sanitized copy exists // nowhere but in what the crafter hands back. - name: "skipping the upload still yields the redacted copy", - filePath: withSecrets, - skipUpload: true, - wantRedacted: true, - wantCount: "7", - wantRules: "anthropic-api-key,aws-access-token,aws-secret-access-key,github-pat", + name: "skipping the upload still yields the redacted copy", + filePath: withSecrets, + skipUpload: true, + wantCount: "7", + wantRules: withSecretsRules, }, { name: "the opt-out is recorded in the attestation", @@ -232,8 +230,11 @@ func TestChainloopAICodingSessionCrafterRedaction(t *testing.T) { skipRedaction: true, }, { - name: "a clean session is not marked as redacted", - filePath: clean, + // The scan ran and found nothing: the material says so, so that a + // clean verdict is distinguishable from a session never scanned. + name: "a clean session is marked as scanned with nothing replaced", + filePath: clean, + wantCount: "0", }, } @@ -281,39 +282,38 @@ func TestChainloopAICodingSessionCrafterRedaction(t *testing.T) { assert.Equal(t, tc.wantCount, got.Annotations[api.AnnotationMaterialRedactionCount]) assert.Equal(t, tc.wantRules, got.Annotations[api.AnnotationMaterialRedactionRules]) - switch { - case tc.wantRedacted: - assert.Equal(t, "true", got.Annotations[api.AnnotationMaterialRedacted]) - // The digest describes the redacted artifact, not the source file. - assert.NotEqual(t, sha256Digest(string(original)), got.GetArtifact().Digest) - - // The sanitized copy is what policies must be handed. Comparing - // it against the recorded digest is the strongest available form - // of "policies see exactly what was stored": it holds even for - // skip-upload, where the stored bytes are kept nowhere else. - require.NotNil(t, content, "a redacted session must hand back its sanitized copy") - assert.Equal(t, sha256Digest(string(content)), got.GetArtifact().Digest) - assert.NotContains(t, string(content), awsKey) - assert.Contains(t, string(content), "[CHAINLOOP_TRACE_REDACTED:aws-access-token]") - - if stored != nil { - assert.Equal(t, string(stored), string(content)) - assert.NotContains(t, string(stored), awsKey) - assert.Contains(t, string(stored), "[CHAINLOOP_TRACE_REDACTED:aws-access-token]") - } - case tc.skipRedaction: + if tc.skipRedaction { assert.Equal(t, "true", got.Annotations[api.AnnotationMaterialRedactionSkipped]) + assert.NotContains(t, got.Annotations, api.AnnotationMaterialRedacted) assert.Contains(t, string(stored), awsKey) assert.Equal(t, sha256Digest(string(original)), got.GetArtifact().Digest) // Nothing was transformed, so nothing is held in memory for the // policy engine: it reads the file, which is what was stored. assert.Nil(t, content) - default: - assert.NotContains(t, got.Annotations, api.AnnotationMaterialRedacted) + } else { + // Marked redacted whenever the scan ran, even when it found nothing. + assert.Equal(t, "true", got.Annotations[api.AnnotationMaterialRedacted]) assert.NotContains(t, got.Annotations, api.AnnotationMaterialRedactionSkipped) - // Nothing to redact, so the digest stays reproducible from the file. - assert.Equal(t, sha256Digest(string(original)), got.GetArtifact().Digest) - assert.Nil(t, content) + + // The scanned copy is what policies must be handed, since a + // material marked redacted fails closed without it. Comparing it + // against the recorded digest is the strongest available form of + // "policies see exactly what was stored": it holds even for + // skip-upload, where the stored bytes are kept nowhere else. + require.NotNil(t, content, "a scanned session must hand back the scanned copy") + assert.Equal(t, sha256Digest(string(content)), got.GetArtifact().Digest) + if stored != nil { + assert.Equal(t, string(stored), string(content)) + } + + if tc.wantCount == "0" { + // Nothing was replaced, so the stored artifact is the file + // itself and the digest stays reproducible from it. + assert.Equal(t, string(original), string(content)) + } else { + assert.NotContains(t, string(content), awsKey) + assert.Contains(t, string(content), "[CHAINLOOP_TRACE_REDACTED:aws-access-token]") + } } // Redaction must never touch the source file. diff --git a/pkg/attestation/crafter/materials/materials.go b/pkg/attestation/crafter/materials/materials.go index d6039ebf1..d897cb4fd 100644 --- a/pkg/attestation/crafter/materials/materials.go +++ b/pkg/attestation/crafter/materials/materials.go @@ -355,17 +355,17 @@ type Craftable interface { // CraftResult is what crafting an artifact yields. type CraftResult struct { Material *api.Attestation_Material - // Content is what the crafter stored in place of the artifact, when it did - // not store it verbatim — today, an AI coding session with secrets redacted - // out of it. For such a material the file on disk is no longer the stored - // content, so anything that needs to read the artifact back has to be given - // these bytes instead; what they are then used for is the caller's business, - // and today they feed policy evaluation, which must not see the data the - // transformation removed. + // Content is what the crafter scanned and stored, when it ran the artifact + // through a transformation before storing it — today, an AI coding session + // scanned for secrets, with any found redacted out of it. For such a material + // the file on disk may no longer be the stored content, so anything that + // needs to read the artifact back has to be given these bytes instead; what + // they are then used for is the caller's business, and today they feed policy + // evaluation, which must not see the data the transformation removed. // - // nil for every crafter that stores the artifact as it found it, which is all - // but one: their content resolves from the material or the file as usual and - // nothing extra is held in memory. + // nil when the crafter did not transform the artifact (every crafter but the + // AI coding session, and that one when redaction is skipped): the content + // resolves from the material or the file as usual. Content []byte }