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,30 +111,57 @@ 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
 
+	// 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
+	}
+
 	artifact := m.GetArtifact()
 	if artifact == nil && m.GetSbomArtifact() != nil {
  artifact = m.GetSbomArtifact().GetArtifact()
 	}
 
 	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
- }
-
  switch {
- case useInlineContent:
+ case len(content) > 0:
+ // NOTE: ingestMaterialToJSON re-reads the artifact from `value` for
+ // the kinds it projects from a path (JUNIT_XML, HELM_CHART), so
+ // supplied content would be silently 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
+ case m.InlineCas:
  rawMaterial = artifact.GetContent()
  case value == "":
  return nil, errors.New("artifact path required")