fix(policies): evaluate redacted materials against the content that was stored

app/cli/internal/policydevel/eval.go

@@ -76,14 +76,19 @@ func Evaluate(opts *EvalOptions, logger zerolog.Logger) (*EvalSummary, error) {
 	}
 
 	// 2. Craft material with annotations
-	material, err := craftMaterial(opts.MaterialPath, opts.MaterialKind, &logger)
+	crafted, err := craftMaterial(opts.MaterialPath, opts.MaterialKind, &logger)
 	if err != nil {
 		return nil, err
 	}
+	material := crafted.Material
 	mergeAnnotations(material, opts.Annotations, &logger)
 
-	// 3. Verify material against policy
-	summary, err := verifyMaterial(policies, material, opts.MaterialPath, opts.Debug, opts.AllowedHostnames, opts.AttestationClient, opts.ControlPlaneConn, opts.ProjectName, opts.ProjectVersionName, &logger)
+	// 3. Verify material against policy. A crafter that transformed the artifact
+	// before storing it hands back what it stored, and that is what the policy
+	// must be evaluated against — `policy devel eval` has to reproduce what
+	// `attestation add` does, or a policy would be developed against input the
+	// real run never sees.
+	summary, err := verifyMaterial(policies, material, opts.MaterialPath, crafted.Content, opts.Debug, opts.AllowedHostnames, opts.AttestationClient, opts.ControlPlaneConn, opts.ProjectName, opts.ProjectVersionName, &logger)
 	if err != nil {
 		return nil, err
 	}
@@ -95,15 +100,15 @@ 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 what tells the policy engine to evaluate the untouched file on disk instead
-// of the sanitized copy staged for upload. Dropping it would silently feed
-// policies redacted input, which is exactly what a secret-hunting policy must
-// not see.
+// is how a policy learns that secrets were found and 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.
 //
 // The chainloop.* namespace is therefore crafter-owned and not overridable, so
-// that a --annotation flag cannot put back the behaviour this guards against.
-// Crafter.stageMaterial protects the equivalent invariant on `attestation add`
-// by refusing to override annotations that come from the contract.
+// that a --annotation flag cannot clear the marker. Crafter.stageMaterial
+// protects the equivalent invariant on `attestation add` by refusing to override
+// annotations that come from the contract.
 func mergeAnnotations(material *v12.Attestation_Material, annotations map[string]string, logger *zerolog.Logger) {
 	if len(annotations) == 0 {
 		return
@@ -145,7 +150,7 @@ func createPolicies(policyPath string, inputs map[string]string) (*v1.Policies,
 	}, nil
 }
 
-func verifyMaterial(pol *v1.Policies, material *v12.Attestation_Material, materialPath string, debug bool, allowedHostnames []string, attestationClient controlplanev1.AttestationServiceClient, grpcConn *grpc.ClientConn, projectName, projectVersion string, logger *zerolog.Logger) (*EvalSummary, error) {
+func verifyMaterial(pol *v1.Policies, material *v12.Attestation_Material, materialPath string, content []byte, debug bool, allowedHostnames []string, attestationClient controlplanev1.AttestationServiceClient, grpcConn *grpc.ClientConn, projectName, projectVersion string, logger *zerolog.Logger) (*EvalSummary, error) {
 	var opts []policies.PolicyVerifierOption
 	if len(allowedHostnames) > 0 {
 		opts = append(opts, policies.WithAllowedHostnames(allowedHostnames...))
@@ -159,7 +164,7 @@ func verifyMaterial(pol *v1.Policies, material *v12.Attestation_Material, materi
 	}
 
 	v := policies.NewPolicyVerifier(pol, attestationClient, logger, opts...)
-	policyEvs, err := v.VerifyMaterial(context.Background(), material, materialPath)
+	policyEvs, err := v.VerifyMaterial(context.Background(), material, materialPath, content)
 	if err != nil {
 		return nil, err
 	}
@@ -225,7 +230,7 @@ func verifyMaterial(pol *v1.Policies, material *v12.Attestation_Material, materi
 	return summary, nil
 }
 
-func craftMaterial(materialPath, materialKind string, logger *zerolog.Logger) (*v12.Attestation_Material, error) {
+func craftMaterial(materialPath, materialKind string, logger *zerolog.Logger) (*materials.CraftResult, error) {
 	backend := &casclient.CASBackend{
 		Name:     "backend",
 		MaxSize:  0,
@@ -252,15 +257,15 @@ func craftMaterial(materialPath, materialKind string, logger *zerolog.Logger) (*
 	return nil, fmt.Errorf("could not auto-detect material kind for: %s", materialPath)
 }
 
-func craft(materialPath string, kind v1.CraftingSchema_Material_MaterialType, name string, backend *casclient.CASBackend, logger *zerolog.Logger) (*v12.Attestation_Material, error) {
+func craft(materialPath string, kind v1.CraftingSchema_Material_MaterialType, name string, backend *casclient.CASBackend, logger *zerolog.Logger) (*materials.CraftResult, error) {
 	materialSchema := &v1.CraftingSchema_Material{
 		Type: kind,
 		Name: name,
 	}
 
-	m, err := materials.Craft(context.Background(), materialSchema, materialPath, backend, nil, logger, nil)
+	res, err := materials.Craft(context.Background(), materialSchema, materialPath, backend, nil, logger, nil)
 	if err != nil {
 		return nil, fmt.Errorf("failed to craft material (kind=%s): %w", kind.String(), err)
 	}
-	return m, nil
+	return res, nil
 }

app/cli/internal/policydevel/eval_test.go

@@ -252,21 +252,23 @@ func writeSessionFixture(t *testing.T) string {
 	return path
 }
 
-// Policies must be evaluated against the material as it sits on disk. The crafter
-// redacts the copy it stages for upload, and a dry run always stages inline, so
-// evaluating the staged bytes would hide from a policy the very secret it exists
-// to catch.
-func TestEvaluateReadsUnredactedMaterialFromDisk(t *testing.T) {
+// Policies must be evaluated against the copy the crafter stored, never the file
+// on disk. A policy is arbitrary user-authored Rego that may ship what it reads to
+// an allowed hostname, so handing it the credential that redaction just stripped
+// out would turn the policy engine into an exfiltration path.
+//
+// `policy devel eval` has to agree with `attestation add` on this, or a policy
+// would be developed against input the real run never produces.
+func TestEvaluateReadsRedactedMaterial(t *testing.T) {
 	testCases := []struct {
 		name        string
 		annotations map[string]string
 	}{
-		// The bug this pins: with no --annotation flags the crafter's annotation
-		// map was replaced by an empty one, so the marker that selects the file on
-		// disk was lost and policies evaluated the sanitized copy.
 		{name: "without user annotations", annotations: nil},
 		{name: "with user annotations", annotations: map[string]string{"custom": "value"}},
 		{
+			// The chainloop.* namespace is crafter-owned: clearing the marker
+			// from the command line must not be able to re-open the disk path.
 			name:        "with an attempt to override the redaction marker",
 			annotations: map[string]string{v12.AnnotationMaterialRedacted: "false"},
 		},
@@ -279,6 +281,9 @@ func TestEvaluateReadsUnredactedMaterialFromDisk(t *testing.T) {
 				MaterialKind: "CHAINLOOP_AI_CODING_SESSION",
 				MaterialPath: writeSessionFixture(t),
 				Annotations:  tc.annotations,
+				// Debug surfaces the exact bytes handed to the engine, which is
+				// what the strongest assertion below inspects.
+				Debug: true,
 			}
 
 			result, err := Evaluate(opts, zerolog.New(os.Stderr))
@@ -287,7 +292,17 @@ func TestEvaluateReadsUnredactedMaterialFromDisk(t *testing.T) {
 
 			assert.False(t, result.Result.Skipped)
 			require.Len(t, result.Result.Violations, 1)
-			assert.Contains(t, result.Result.Violations[0], "GitHub token found")
+			assert.Contains(t, result.Result.Violations[0], "secrets were found and redacted")
+
+			// The point of the whole change: the credential is nowhere in the
+			// policy input, and the placeholder that replaced it is.
+			require.NotNil(t, result.DebugInfo)
+			require.NotEmpty(t, result.DebugInfo.Inputs)
+			for _, input := range result.DebugInfo.Inputs {
+				assert.NotContains(t, string(input), fixtureGitHubPAT,
+					"the policy engine must never receive the un-redacted credential")
+				assert.Contains(t, string(input), "[REDACTED:")
+			}
 		})
 	}
 }

app/cli/internal/policydevel/testdata/ai-coding-session-no-secrets-policy.yaml

@@ -2,7 +2,7 @@ apiVersion: chainloop.dev/v1
 kind: Policy
 metadata:
   name: ai-coding-session-no-secrets
-  description: Policy that fails when a GitHub token is present in the session
+  description: Policy that fails when the session carried a credential
 spec:
   policies:
     - kind: CHAINLOOP_AI_CODING_SESSION
@@ -11,9 +11,20 @@ spec:
 
         import rego.v1
 
-        # The token is matched by shape rather than by value so that this file
-        # does not itself carry a credential-looking literal.
+        # Secrets are stripped out of the session before it is stored, and
+        # policies evaluate the stored copy, so a leaked credential is not here
+        # to be pattern-matched any more: it shows up as the placeholder that
+        # replaced it. Unanchored counterpart of
+        # internal/redaction.IsDefaultPlaceholder.
         violations contains msg if {
-            regex.match(`ghp_[A-Za-z0-9]{36}`, json.marshal(input))
-            msg := "GitHub token found in the coding session"
+            regex.match(`\[REDACTED(:[^\]\s]*)?\]`, json.marshal(input))
+            msg := "secrets were found and redacted in the coding session"
+        }
+
+        # The opt-out is visible too, so a bypass cannot pass silently: nothing
+        # was redacted, which is exactly why nothing can be concluded from the
+        # absence of placeholders above.
+        violations contains msg if {
+            input.chainloop_metadata.annotations["chainloop.material.redaction.skipped"] == "true"
+            msg := "the coding session was stored without secret redaction"
         }

pkg/attestation/crafter/api/attestation/v1/crafting_state.go

@@ -62,8 +62,9 @@ var (
 	// 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 read the untouched
-	// local file instead of the stored content (see GetEvaluableContent).
+	// 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 = CreateAnnotation("material.redacted")
 	// AnnotationMaterialRedactionCount is how many secrets were replaced.
 	AnnotationMaterialRedactionCount = CreateAnnotation("material.redaction.count")
@@ -110,41 +111,74 @@ func (m *Attestation_Material) NormalizedOutput() (*NormalizedMaterialOutput, er
 	return nil, fmt.Errorf("unknown material: %s", m.MaterialType)
 }
 
-// GetEvaluableContent returns the content to be sent to policy evaluations
-func (m *Attestation_Material) GetEvaluableContent(value string) ([]byte, error) {
+// ErrRedactedContentRequired is returned when policy evaluation is asked to
+// resolve a material whose stored copy was sanitized, without being given that
+// copy. There is no safe source left: the file on disk is the un-redacted
+// original, and for a CAS-backed material the sanitized bytes were streamed away
+// and are not on the material at all.
+var ErrRedactedContentRequired = errors.New(
+	"sanitized content is required to evaluate a redacted material: " +
+		"policies must never be handed the un-redacted original")
+
+// GetEvaluableContent returns the content to be sent to policy evaluations.
+//
+// content, when non-empty, is what policies are evaluated against, overriding
+// both the inline bytes and the file at value. Crafters that transform an artifact
+// before it leaves the machine — redacting secrets out of an AI coding session —
+// report the bytes they stored so that the policy engine sees exactly those,
+// whatever CAS backend is in use. Pass nil to resolve the content from the
+// material or the file, which is what every other material does.
+//
+// A material marked redacted with no content supplied fails closed. One
+// consequence is worth knowing: such a material's policy input cannot be
+// reconstructed from persisted crafting state alone, so any future push-time or
+// server-side material evaluation has to plumb the bytes through as well.
+func (m *Attestation_Material) GetEvaluableContent(value string, content []byte) ([]byte, error) {
 	var rawMaterial []byte
 	var err error
 
-	artifact := m.GetArtifact()
-	if artifact == nil && m.GetSbomArtifact() != nil {
-		artifact = m.GetSbomArtifact().GetArtifact()
+	// Fail closed before any source is chosen: a material whose stored copy was
+	// sanitized has exactly one valid policy input, and it is not the file on
+	// disk. Checked here rather than inside the artifact branch below so that it
+	// also covers material kinds that carry no artifact.
+	if len(content) == 0 && m.GetAnnotations()[AnnotationMaterialRedacted] == AnnotationValueTrue {
+		return nil, ErrRedactedContentRequired
 	}
 
-	if artifact != nil {
-		// Policies must evaluate the artifact as it was produced. For a redacted
-		// material the inline bytes are the sanitized copy, so the untouched file
-		// on disk wins; without a local path there is nothing better to read.
-		// This keeps the policy input identical whatever CAS backend is in use,
-		// which matters most in dry runs, where the backend is always inline and
-		// is exactly where people test their policies.
-		useInlineContent := m.InlineCas
-		if m.GetAnnotations()[AnnotationMaterialRedacted] == AnnotationValueTrue && value != "" {
-			useInlineContent = false
+	if len(content) > 0 {
+		// Chosen ahead of the artifact below so that supplied content is
+		// authoritative for every material kind, rather than only for the ones
+		// whose content happens to resolve from an artifact. A kind that carries
+		// no artifact would otherwise drop it and project "{}" instead.
+		//
+		// NOTE: ingestMaterialToJSON re-reads the artifact from `value` for the
+		// kinds it projects from a path (JUNIT_XML, HELM_CHART), so supplied
+		// content is still ignored for those. Unreachable today, since only
+		// CHAINLOOP_AI_CODING_SESSION supplies content and it is JSON-native. A
+		// crafter that starts transforming what it stores for a path-projected
+		// kind must make the projection content-based first, or it will fail open.
+		rawMaterial = content
+	} else {
+		artifact := m.GetArtifact()
+		if artifact == nil && m.GetSbomArtifact() != nil {
+			artifact = m.GetSbomArtifact().GetArtifact()
 		}
 
-		switch {
-		case useInlineContent:
-			rawMaterial = artifact.GetContent()
-		case value == "":
-			return nil, errors.New("artifact path required")
-		case m.MaterialType != v1.CraftingSchema_Material_HELM_CHART &&
-			m.MaterialType != v1.CraftingSchema_Material_JUNIT_XML &&
-			m.MaterialType != v1.CraftingSchema_Material_RADAMSA_CRASHES:
-			// read content from local filesystem (except for tgz charts and
-			// metadata-only materials like radamsa crashes)
-			rawMaterial, err = os.ReadFile(value)
-			if err != nil {
-				return nil, fmt.Errorf("failed to read material content: %w", err)
+		if artifact != nil {
+			switch {
+			case m.InlineCas:
+				rawMaterial = artifact.GetContent()
+			case value == "":
+				return nil, errors.New("artifact path required")
+			case m.MaterialType != v1.CraftingSchema_Material_HELM_CHART &&
+				m.MaterialType != v1.CraftingSchema_Material_JUNIT_XML &&
+				m.MaterialType != v1.CraftingSchema_Material_RADAMSA_CRASHES:
+				// read content from local filesystem (except for tgz charts and
+				// metadata-only materials like radamsa crashes)
+				rawMaterial, err = os.ReadFile(value)
+				if err != nil {
+					return nil, fmt.Errorf("failed to read material content: %w", err)
+				}
 			}
 		}
 	}