From 73db4f37bec78fe0faa75d78584b5d024a3db51c Mon Sep 17 00:00:00 2001 From: Javier Rodriguez Date: Wed, 12 Aug 2026 15:39:59 +0200 Subject: [PATCH] perf(crafter): stream AccessChk material instead of buffering it whole Parse now takes an io.Reader and streams the material line by line, so the crafter reads it via os.Open rather than os.ReadFile, removing the whole-file buffer held at craft time. The size-gated raw-text fallback is preserved by counting bytes as they stream and dropping the verbatim Raw/RawLines fields once RawRetentionLimit is crossed, without needing the input size upfront. Assisted-by: Claude Code Signed-off-by: Javier Rodriguez Chainloop-Trace-Sessions: 3379c15e-39dc-4bed-8b61-88f30e459908 --- .../api/attestation/v1/crafting_state.go | 2 +- .../crafter/materials/accesschk.go | 22 ++--- .../crafter/materials/accesschk/accesschk.go | 84 ++++++++++++++----- .../materials/accesschk/accesschk_test.go | 42 ++++++++-- .../crafter/materials/accesschk/gate_test.go | 13 +-- 5 files changed, 109 insertions(+), 54 deletions(-) diff --git a/pkg/attestation/crafter/api/attestation/v1/crafting_state.go b/pkg/attestation/crafter/api/attestation/v1/crafting_state.go index 1a8412883..fa1c5365d 100644 --- a/pkg/attestation/crafter/api/attestation/v1/crafting_state.go +++ b/pkg/attestation/crafter/api/attestation/v1/crafting_state.go @@ -239,7 +239,7 @@ func (m *Attestation_Material) ingestMaterialToJSON(rawMaterial []byte, value st // AccessChk emits plain text; project it to JSON so the policy engine, // which only consumes JSON, can evaluate it. The raw text is preserved // in the projection's "raw" field for string-matching fallbacks. - report, err := accesschk.Parse(rawMaterial) + report, err := accesschk.Parse(bytes.NewReader(rawMaterial)) if err != nil { return nil, fmt.Errorf("invalid accesschk material: %w", err) } diff --git a/pkg/attestation/crafter/materials/accesschk.go b/pkg/attestation/crafter/materials/accesschk.go index c9d97d249..e2b6890b3 100644 --- a/pkg/attestation/crafter/materials/accesschk.go +++ b/pkg/attestation/crafter/materials/accesschk.go @@ -44,16 +44,18 @@ func NewAccessChkCrafter(schema *schemaapi.CraftingSchema_Material, backend *cas } func (i *AccessChkCrafter) Craft(ctx context.Context, filePath string) (*api.Attestation_Material, error) { - data, err := os.ReadFile(filePath) + f, err := os.Open(filePath) if err != nil { return nil, fmt.Errorf("can't open the file: %w", err) } + defer func() { _ = f.Close() }() // Soft fingerprint: AccessChk emits free-form text, so we only require that // the input is valid text that resembles AccessChk output (a banner, at - // least one access entry, or an SDDL/descriptor marker). The raw text is - // stored unchanged; it is projected to JSON later for policy evaluation. - report, err := accesschk.Parse(data) + // least one access entry, or an SDDL/descriptor marker). The file is streamed + // rather than read whole into memory; it is stored unchanged by the uploader + // below and projected to JSON later for policy evaluation. + report, err := accesschk.Parse(f) if err != nil { return nil, fmt.Errorf("invalid accesschk output: %w", ErrInvalidMaterialType) } @@ -62,18 +64,6 @@ func (i *AccessChkCrafter) Craft(ctx context.Context, filePath string) (*api.Att return nil, fmt.Errorf("input does not look like accesschk output: %w", ErrInvalidMaterialType) } - // The material is attested by digest and stored as-is, but at policy - // evaluation time it is projected to JSON client-side on the runner. Above - // this size the parser omits the verbatim raw-text fallback to keep peak - // memory bounded; warn so an operator relying on string-matching policies is - // aware the fallback fields will not be present for this material. - if len(data) > accesschk.RawRetentionLimit { - i.logger.Warn(). - Int("size", len(data)). - Int("threshold", accesschk.RawRetentionLimit). - Msg("large AccessChk material: raw-text fallback fields are omitted from the policy input to limit memory use") - } - m, err := uploadAndCraft(ctx, i.input, i.backend, filePath, i.logger) if err != nil { return nil, err diff --git a/pkg/attestation/crafter/materials/accesschk/accesschk.go b/pkg/attestation/crafter/materials/accesschk/accesschk.go index 344d3212f..1ac6e6a26 100644 --- a/pkg/attestation/crafter/materials/accesschk/accesschk.go +++ b/pkg/attestation/crafter/materials/accesschk/accesschk.go @@ -21,17 +21,16 @@ // text is retained in Raw so a policy can fall back to string matching // regardless of the output mode used. // -// The parser streams the input line by line rather than building a normalized -// full-text copy, and it size-gates the verbatim fallback fields (Raw and the -// per-object RawLines). Both measures keep peak memory bounded: a -// several-hundred-MB material would otherwise pin multiple copies of itself in -// memory and double the JSON document handed to the policy engine, which has -// OOM-killed CI runners during client-side policy evaluation. +// Parse reads from an io.Reader and streams the input line by line rather than +// buffering the whole material in memory or building a normalized full-text +// copy, and it size-gates the verbatim fallback fields (Raw and the per-object +// RawLines). Together these keep peak memory bounded: a large material would +// otherwise pin multiple copies of itself in memory and inflate the JSON +// document handed to the policy engine to multiples of the input size. package accesschk import ( "bufio" - "bytes" "fmt" "io" "regexp" @@ -108,35 +107,45 @@ type Object struct { // Raw holds the full original text for inputs below RawRetentionLimit and is // empty otherwise; descriptorMarker records whether an SDDL/descriptor marker // was seen during parsing so LooksLikeAccessChk stays reliable even when Raw is -// omitted for oversized inputs. +// omitted for oversized inputs. rawOmitted records whether the verbatim fallback +// fields were dropped because the input exceeded RawRetentionLimit, so callers +// can warn without knowing the input size upfront (see RawOmitted). type Report struct { Tool Tool `json:"tool"` Objects []Object `json:"objects"` Raw string `json:"raw"` descriptorMarker bool + rawOmitted bool } -// Parse converts AccessChk text output into a Report. It only returns an error -// when the input is not valid UTF-8 text; well-formed text always parses, with +// RawOmitted reports whether the verbatim fallback fields (Raw and the per-object +// RawLines) were dropped because the input exceeded RawRetentionLimit. It lets a +// streaming caller warn about the omission without measuring the input itself. +func (r *Report) RawOmitted() bool { + return r.rawOmitted +} + +// Parse converts AccessChk text output read from r into a Report. It streams r +// and never buffers the whole input, so it can parse a file handle directly +// without an intermediate full-file copy. It only returns an error when the +// input is not valid UTF-8 text or r fails; well-formed text always parses, with // any unrecognized content preserved in the per-object RawLines and the // top-level Raw field for inputs below RawRetentionLimit (see the package doc). -func Parse(data []byte) (*Report, error) { - if !utf8.Valid(data) { - return nil, fmt.Errorf("input is not valid UTF-8 text") - } - - // Retain the verbatim fallback fields only for inputs small enough that the - // resulting JSON projection stays bounded; see RawRetentionLimit. - retainRaw := len(data) <= RawRetentionLimit - +func Parse(r io.Reader) (*Report, error) { report := &Report{ Tool: Tool{Name: ToolName}, Objects: []Object{}, } - if retainRaw { - report.Raw = string(data) - } + + // Retain the verbatim fallback fields (Raw, RawLines) optimistically. The + // input size is unknown when streaming, so accumulate while the running byte + // count stays within RawRetentionLimit; the moment it crosses, drop + // everything retained so far and stop, keeping peak memory bounded for + // oversized materials without needing the size upfront. + retainRaw := true + var rawBuilder strings.Builder + var total int var current *Object var entryIndent int @@ -147,12 +156,37 @@ func Parse(data []byte) (*Report, error) { // at once. bufio.Reader.ReadString grows to fit arbitrarily long lines and // returns freshly allocated strings, so stored substrings do not pin the // entire input in memory. - reader := bufio.NewReader(bytes.NewReader(data)) + reader := bufio.NewReader(r) for { raw, readErr := reader.ReadString('\n') + total += len(raw) + + // Validate UTF-8 incrementally. A '\n' (0x0A) can never be part of a + // multi-byte UTF-8 sequence, so splitting on it never splits a rune and + // per-line validation is equivalent to validating the whole input. + if !utf8.ValidString(raw) { + return nil, fmt.Errorf("input is not valid UTF-8 text") + } + line := strings.TrimSuffix(raw, "\n") line = strings.TrimSuffix(line, "\r") + // Cross the retention limit: discard everything retained so far (Raw and + // every object's RawLines) and stop retaining. This is all-or-nothing so + // oversized reports never carry a partial verbatim copy. + if retainRaw && total > RawRetentionLimit { + retainRaw = false + report.rawOmitted = true + rawBuilder.Reset() + for i := range report.Objects { + report.Objects[i].RawLines = nil + } + } + // Concatenating every ReadString chunk reproduces the input verbatim. + if retainRaw { + rawBuilder.WriteString(raw) + } + if line != "" || readErr == nil { processLine(report, line, ¤t, &entryIndent, §ion, ¤tACE, retainRaw) } @@ -165,6 +199,10 @@ func Parse(data []byte) (*Report, error) { } } + if retainRaw { + report.Raw = rawBuilder.String() + } + return report, nil } diff --git a/pkg/attestation/crafter/materials/accesschk/accesschk_test.go b/pkg/attestation/crafter/materials/accesschk/accesschk_test.go index ca4526b75..32ade9fe0 100644 --- a/pkg/attestation/crafter/materials/accesschk/accesschk_test.go +++ b/pkg/attestation/crafter/materials/accesschk/accesschk_test.go @@ -16,6 +16,7 @@ package accesschk_test import ( + "bytes" "os" "testing" @@ -28,7 +29,7 @@ func TestParse_Default(t *testing.T) { data, err := os.ReadFile("./testdata/default.txt") require.NoError(t, err) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) assert.Equal(t, "AccessChk", report.Tool.Name) @@ -60,7 +61,7 @@ func TestParse_Verbose(t *testing.T) { data, err := os.ReadFile("./testdata/verbose.txt") require.NoError(t, err) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) require.Len(t, report.Objects, 1) @@ -80,7 +81,7 @@ func TestParse_Service(t *testing.T) { data, err := os.ReadFile("./testdata/service.txt") require.NoError(t, err) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) require.Len(t, report.Objects, 1) @@ -94,7 +95,7 @@ func TestParse_SDDL(t *testing.T) { data, err := os.ReadFile("./testdata/sddl.txt") require.NoError(t, err) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) require.Len(t, report.Objects, 1) @@ -112,7 +113,7 @@ func TestParse_NoBanner(t *testing.T) { data, err := os.ReadFile("./testdata/nobanner.txt") require.NoError(t, err) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) assert.Equal(t, "AccessChk", report.Tool.Name) @@ -127,7 +128,7 @@ func TestParse_DescriptorFormat(t *testing.T) { data, err := os.ReadFile("./testdata/descriptor.txt") require.NoError(t, err) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) assert.True(t, report.LooksLikeAccessChk()) @@ -168,19 +169,42 @@ func TestParse_Garbage(t *testing.T) { data, err := os.ReadFile("./testdata/garbage.txt") require.NoError(t, err) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) assert.False(t, report.LooksLikeAccessChk()) } func TestParse_InvalidUTF8(t *testing.T) { - _, err := accesschk.Parse([]byte{0xff, 0xfe, 0x00, 0x01}) + _, err := accesschk.Parse(bytes.NewReader([]byte{0xff, 0xfe, 0x00, 0x01})) assert.Error(t, err) } func TestParse_Empty(t *testing.T) { - report, err := accesschk.Parse([]byte{}) + report, err := accesschk.Parse(bytes.NewReader([]byte{})) require.NoError(t, err) assert.False(t, report.LooksLikeAccessChk()) assert.Empty(t, report.Objects) } + +// TestParse_StreamsFromOpenFile verifies Parse consumes an *os.File directly, +// i.e. it can stream from disk without the caller reading the whole file into +// memory first. +func TestParse_StreamsFromOpenFile(t *testing.T) { + f, err := os.Open("./testdata/default.txt") + require.NoError(t, err) + defer f.Close() + + report, err := accesschk.Parse(f) + require.NoError(t, err) + + assert.Equal(t, "6.15", report.Tool.Version) + assert.False(t, report.RawOmitted()) + require.Len(t, report.Objects, 1) + assert.Equal(t, `c:\windows\system32\notepad.exe`, report.Objects[0].Name) + require.Len(t, report.Objects[0].AccessEntries, 3) + + // Streaming still reconstructs the verbatim Raw fallback for small inputs. + raw, err := os.ReadFile("./testdata/default.txt") + require.NoError(t, err) + assert.Equal(t, string(raw), report.Raw) +} diff --git a/pkg/attestation/crafter/materials/accesschk/gate_test.go b/pkg/attestation/crafter/materials/accesschk/gate_test.go index 34352efd5..0415a6861 100644 --- a/pkg/attestation/crafter/materials/accesschk/gate_test.go +++ b/pkg/attestation/crafter/materials/accesschk/gate_test.go @@ -16,6 +16,7 @@ package accesschk_test import ( + "bytes" "strings" "testing" @@ -53,9 +54,10 @@ func TestParse_LargeInputOmitsRawFields(t *testing.T) { data := buildLargeInput(accesschk.RawRetentionLimit + 1) require.Greater(t, len(data), accesschk.RawRetentionLimit) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) + assert.True(t, report.RawOmitted(), "RawOmitted must report the drop above the retention limit") assert.Empty(t, report.Raw, "Raw must be omitted above the retention limit") require.NotEmpty(t, report.Objects) for _, obj := range report.Objects { @@ -74,9 +76,10 @@ func TestParse_LargeInputOmitsRawFields(t *testing.T) { func TestParse_SmallInputRetainsRawFields(t *testing.T) { data := []byte("c:\\file\n RW BUILTIN\\Administrators\n") - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) + assert.False(t, report.RawOmitted(), "RawOmitted must be false below the retention limit") assert.Equal(t, string(data), report.Raw) require.Len(t, report.Objects, 1) assert.Equal(t, []string{" RW BUILTIN\\Administrators"}, report.Objects[0].RawLines) @@ -97,7 +100,7 @@ func TestParse_LargeSDDLStillDetected(t *testing.T) { data := []byte(b.String()) require.Greater(t, len(data), accesschk.RawRetentionLimit) - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) assert.Empty(t, report.Raw) @@ -109,7 +112,7 @@ func TestParse_LargeSDDLStillDetected(t *testing.T) { func TestParse_CRLFLineEndings(t *testing.T) { data := []byte("c:\\file\r\n RW Everyone\r\n") - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) require.Len(t, report.Objects, 1) @@ -126,7 +129,7 @@ func TestParse_VeryLongLine(t *testing.T) { principal := strings.Repeat("A", 128*1024) data := []byte("c:\\file\n RW " + principal + "\n") - report, err := accesschk.Parse(data) + report, err := accesschk.Parse(bytes.NewReader(data)) require.NoError(t, err) require.Len(t, report.Objects, 1)