diff --git a/.gitignore b/.gitignore index 2e97af2..0fed448 100644 --- a/.gitignore +++ b/.gitignore @@ -35,3 +35,6 @@ coverage.* *.swp .idea/ .vscode/ + +# Local files test snapshots +local/ \ No newline at end of file diff --git a/.kiro/specs/piace/design.md b/.kiro/specs/piace/design.md index 2d022de..61f36c0 100644 --- a/.kiro/specs/piace/design.md +++ b/.kiro/specs/piace/design.md @@ -5,8 +5,11 @@ PIACE is a dependency-free, CGO-free Go CLI that compares a target's selected **baseline catalog** against a **candidate catalog** requested from an existing Puppet Server or OpenVox **compiler**. CI deploys the candidate environment -before PIACE runs. PIACE never embeds a Puppet runtime, runs agents, or writes -facts or catalogs to PuppetDB. +before PIACE runs. PIACE never embeds a Puppet runtime, runs agents, or issues +a write to PuppetDB. With `catalog_api: v4` the candidate compilation writes +nothing to PuppetDB either, because every v4 request disables fact and catalog +persistence; with `catalog_api: v3` the compiler stores the candidate facts and +catalog regardless, which is the constraint that shapes the v3 rules below. This design fixes the contracts deliberately left open in the requirements: configuration merge rules, snapshot integrity, compiler compatibility, @@ -20,7 +23,8 @@ precedence. The implementation must use the terminology in `CONTEXT.md`. - Strict source and target validation before a catalog is diffed. - Independent least-privilege mTLS configuration for the compiler and PuppetDB; no secret material in output. -- An explicit degraded-compatibility path for compiler catalog API v3. +- An explicit degraded-compatibility path for compiler catalog API v3, bounded + by what v3 cannot do: suppress persistence. - Reusable, verifiable snapshots for environment-stable baselines. ### 1.2 Explicit non-goals @@ -84,11 +88,25 @@ fail_on_diff `catalog_api` is `v3` or `v4`; `allow_v3_fallback` defaults to `false` and is valid only with `v4`. This explicit opt-in prevents a server capability error -from silently degrading trusted-fact semantics. A v4 request may fall back only -for a documented unsupported-endpoint or unsupported-version response. It must -not fall back after authentication, authorization, timeout, malformed response, -or candidate identity/environment mismatch. Any fallback is recorded as a -warning and is subject to the v3 warning rules. +from silently degrading both trusted-fact semantics and PuppetDB integrity. A +v4 request may fall back only for a documented unsupported-endpoint or +unsupported-version response. It must not fall back after authentication, +authorization, timeout, malformed response, or candidate identity/environment +mismatch. Any fallback is recorded as a warning and is subject to every v3 rule +below, including the persistence consequences — an operator who enables the +fallback is accepting that a compiler without v4 will have the target's stored +factset and catalog overwritten. + +Any target that can compile over v3 requires `baseline.source: file` +(requirements.md 1.8) — `catalog_api: v3`, and equally `catalog_api: v4` with +`allow_v3_fallback: true`. A v3 compilation overwrites the target's stored +PuppetDB catalog and factset with the candidate's, so a `puppetdb` baseline is +not merely inaccurate here: the candidate request destroys the baseline the run +reads, and the failure surfaces on the *next* target or the *next* run as a +baseline-environment mismatch. Both combinations are configuration errors +rejected during resolution rather than runtime warnings, because a fallback +that has already fired cannot be un-fired. Pairing a file baseline with v3 +still leaves the compilation itself mutating: see section 5. ### 3.2 Merge and validation rules @@ -180,13 +198,30 @@ PIACE fails compilation rather than inventing trusted facts. The response provenance records `provided` or `compiler_lookup` but never trusted-fact values. +Every v4 request carries `persistence: {facts: false, catalog: false}`. This is +not configurable. It is what allows requirement 1.6 to hold and what keeps a +PuppetDB baseline meaningful: the compiler returns the candidate catalog and +writes nothing, so the target's stored factset and catalog remain those of its +last real agent run. + For API v3, and every permitted v4-to-v3 fallback, PIACE attaches a prominent, -non-suppressible warning to the target: the catalog-reader certificate can make -`$trusted` reflect the service identity rather than the target. The same -warning appears in the shared result, text, JSON, and HTML. OpenVox is -configured v3 only unless an operator explicitly selects an implementation -with a documented v4 contract; PIACE does not claim v4 trusted-fact equivalence -for OpenVox. +non-suppressible warning to the target, covering both v3 consequences: the +catalog-reader certificate can make `$trusted` reflect the service identity +rather than the target, and the compilation writes the candidate facts and +candidate catalog into PuppetDB under the candidate environment. The same +warning appears in the shared result, text, JSON, and HTML. + +The v3 endpoint offers no persistence control, so neither consequence is +avoidable from the client: the compiler saves the facts submitted with the +request, and stores the compiled catalog through its PuppetDB catalog cache +terminus. PIACE contains v3's blast radius through configuration +(`baseline.source: file` is mandatory, section 3.1) and reports it through the +warning; it cannot prevent it. + +Puppet Server and OpenVox are treated identically. Both serve v3 and v4, both +authorize a catalog-reader certificate through `auth.conf`, and both honour the +v4 `persistence` field, so `catalog_api` alone determines PIACE's guarantees. +There is no implementation-specific branch anywhere in the adapter. PIACE does not probe alternate API versions speculatively. Capture catalog uses the exact same adapter and policy as comparison and records the requested API, @@ -398,18 +433,23 @@ assets, and user-controlled template execution are excluded. | Decision | Rationale | Requirements | | --- | --- | --- | | Separate target and service files | Keeps reviewable scope/policy distinct from mTLS locations. | 3, 4 | -| Explicit v3 fallback opt-in | Prevents silent loss of v4 trusted-fact behavior. | 2, 7 | +| Explicit v3 fallback opt-in | Prevents silent loss of v4 trusted-fact behavior and silent PuppetDB mutation. | 2, 7 | +| v4 persistence always disabled | The only client-side control that keeps a candidate compilation out of PuppetDB. | 1, 7 | +| v3 requires a file baseline | A v3 candidate compilation overwrites a PuppetDB baseline, including its own run's. | 1, 7 | | SHA-256 canonical payload envelope | Detects snapshot corruption without a Puppet runtime. | 11 | | Continue valid targets after local errors | Produces actionable CI evidence without hiding failures. | 8, 10 | | Redact after equality, before results | Maintains correct diff semantics and prevents disclosure. | 3, 8 | | Enabled impact failure is operational | A requested bounded analysis must not be silently omitted. | 9, 10 | -The unresolved external protocol details are isolated behind the compiler and -PuppetDB adapters. Before production implementation, fixture captures from the -specific Puppet Server/OpenVox and PuppetDB versions in use must verify request -fields, response shapes, checksum semantics, file-content endpoints, and PQL -options; adapter support is not enabled merely because another implementation -accepts a similar endpoint. +The external protocol details are isolated behind the compiler and PuppetDB +adapters. The v3/v4 request shapes, the v3 `Accept` requirement, the v4 +response envelope, and the v3/v4 persistence behaviour were verified against a +deployed OpenVox compiler and PuppetDB on 2026-08-25 (requirements.md section +7). For any other compiler or PuppetDB version, fixture captures must verify +request fields, response shapes, checksum semantics, file-content endpoints, +and PQL options before that combination is declared supported; adapter support +is not enabled merely because another implementation accepts a similar +endpoint. ## Correctness Properties @@ -475,6 +515,6 @@ normalization, exclusions, redaction, aggregate equivalence, PQL quoting, and outcome precedence. Adapter contract fixtures must represent every supported PuppetDB, Puppet Server, and OpenVox response variant. Integration validation uses an mTLS test service to prove authority isolation, no secret disclosure, -v3 warning behavior, v4 handling, fallback limits, and deterministic -self-contained report generation. Release validation proves the CGO-free +v3 warning behavior, v4 handling including the always-disabled persistence +fields, fallback limits, and deterministic self-contained report generation. Release validation proves the CGO-free artifact and checksum/signature workflow. diff --git a/.kiro/specs/piace/requirements.md b/.kiro/specs/piace/requirements.md index bda5c81..ff52d60 100644 --- a/.kiro/specs/piace/requirements.md +++ b/.kiro/specs/piace/requirements.md @@ -124,6 +124,17 @@ candidate environment, so that I can review changes before release. target identity and environment agree with the request, or report a compilation error. 6. THE CLI SHALL not persist candidate facts or candidate catalogs to PuppetDB. +7. WHEN catalog API v4 is selected, THE CLI SHALL request compilation with + fact persistence and catalog persistence explicitly disabled, which is how + criterion 6 is satisfied. +8. WHEN a target can compile over v3 — `catalog_api: v3`, or `catalog_api: v4` + with `allow_v3_fallback: true` — THE CLI SHALL require `baseline.source: + file` for that target. A v3 compilation cannot satisfy criterion 6: the + compiler stores the submitted facts and the compiled catalog under the + candidate environment, which overwrites exactly the PuppetDB baseline the + comparison would read. A permitted fallback reaches that state at runtime, + when it is too late to reject the configuration, so the requirement is + keyed on what the target *may* do, not on what it did. See section 7.2. ### Requirement 2: Target facts and trusted identity @@ -139,13 +150,20 @@ unrelated client identity or stale data. 2. THE CLI SHALL identify the fact source and factset identity used for each candidate result. 3. THE CLI SHALL allow CI configuration to select the compiler catalog API v3 - or v4 for candidate compilation. -4. WHEN v4 is selected and supports target trusted facts, THE CLI SHALL use - that mechanism for the candidate request. -5. WHEN v3 is selected or used as a fallback, THE CLI SHALL emit a prominent - trusted-fact compatibility warning in every output format. + or v4 for candidate compilation. v4 is the supported path; v3 is a degraded + path constrained by section 7.2. +4. WHEN v4 is selected, THE CLI SHALL send the target's own trusted facts in + the request, or use the compiler's PuppetDB trusted-fact lookup when the + target is explicitly configured for it, and SHALL fail compilation when + neither source is available rather than compiling with substituted trusted + facts. +5. WHEN v3 is selected or used as a fallback, THE CLI SHALL emit a prominent, + non-suppressible v3 compatibility warning in every output format. 6. THE v3 warning SHALL explain that `$trusted` can reflect the catalog-reader certificate rather than the target identity. +7. THE v3 warning SHALL also explain that the compilation writes the candidate + facts and the candidate catalog into PuppetDB under the candidate + environment, overwriting the target's stored factset and catalog. ### Requirement 3: Service authentication and authorization @@ -361,20 +379,77 @@ package resolution. ## 7. Compatibility and known constraints -| Capability | Puppet Server | OpenVox | -| --- | --- | --- | -| Shared catalog API | `POST /puppet/v3/catalog/:certname` | `POST /puppet/v3/catalog/:certname` | -| Dedicated catalog-reader certificate | Supported through `auth.conf` | Expected through `auth.conf` | -| v4 catalog API with explicit trusted facts | Documented | Not documented | -| v3 trusted-fact caveat with service identity | Warning required | Warning required | - -Puppet's v3 catalog request uses the TLS client identity. When the -catalog-reader certificate is not the target's certificate, `$trusted` use in -manifests or Hiera can yield a non-equivalent catalog. `puppet-catalog_diff` -documents this exact limitation; PIACE therefore allows an explicit v3/v4 -selection and emits the v3 compatibility warning. See +Puppet Server and OpenVox present PIACE with the same catalog contract. Both +serve `POST /puppet/v3/catalog/:certname` and `POST /puppet/v4/catalog`, both +authorize a dedicated catalog-reader certificate through `auth.conf`, and both +honour the v4 request's `persistence` field. PIACE therefore makes no +implementation-specific distinction: the API version selected in the target +file, not the compiler product, determines what PIACE can guarantee. + +### 7.1 v4 is the supported path + +A v4 request carries `persistence: {facts: false, catalog: false}` and the +target's own trusted facts. The compiler returns the catalog and writes nothing +to PuppetDB, which is what makes requirement 1.6 satisfiable and what makes a +PuppetDB baseline usable: the node's stored catalog and factset are exactly +what its last real agent run produced, both before and after PIACE runs. + +### 7.2 v3 is a degraded path that mutates PuppetDB + +The v3 catalog endpoint has no persistence control, and this is a property of +the endpoint, not of a particular compiler or configuration: + +- the compiler saves the facts submitted in the request, rewriting the target's + stored factset and its `facts_environment` to the candidate environment; +- the compiled catalog is stored through the master's PuppetDB catalog cache + terminus, rewriting the target's stored catalog, `catalog_environment`, and + `transaction_uuid` to the candidate compilation's. + +Two consequences follow, and both are contractual: + +1. **A PuppetDB baseline is impossible with v3.** PIACE reads the baseline, + then compiles the candidate — and the candidate compilation overwrites the + baseline that the next target, or the next run, would read. A v3 target + requires `baseline.source: file` (requirement 1.8), captured while the + baseline environment's catalog was the stored one. So does a v4 target with + `allow_v3_fallback: true`: enabling the fallback is accepting a v3 + compilation, and by the time one happens the configuration can no longer be + rejected. +2. **`baseline.source: file` does not make v3 non-mutating.** It stops PIACE + from destroying its own input; it does not stop the compiler from writing + the candidate facts and catalog into PuppetDB. Any consumer of PuppetDB + state — reporting, exported resources, inventory, node classification that + reads `facts_environment` — sees the candidate values until the target's + next agent run restores them. + +v3 additionally carries the trusted-fact caveat that motivated PIACE's explicit +API selection in the first place: the request is authenticated by the TLS +client identity, so when the catalog-reader certificate is not the target's +certificate, `$trusted` in manifests or Hiera can yield a non-equivalent +catalog. `puppet-catalog_diff` documents this same limitation. See [trusted-facts research](../../../docs/research/trusted-facts-in-existing-catalog-diff-tools.md). +### 7.3 Wire requirements shared by both implementations + +- Every `/puppet/v3/` request requires an explicit `Accept` header. The v3 + routes are served by the compiler's embedded Ruby Puppet request handler, + which rejects a request without one before doing any work, with HTTP 400 and + `"Missing required Accept header"`. The acceptable value is endpoint-specific: + `application/json` for `/puppet/v3/catalog/:certname`, and + `application/octet-stream` for `/puppet/v3/file_content/` — which rejects + `application/json` with HTTP 406. `POST /puppet/v4/catalog` has no such + requirement, being served directly rather than through that handler. +- A v3 catalog response is the catalog document itself. A v4 catalog response + wraps it as `{"catalog": {...}}`. +- The `Accept` header does not select the catalog's rich-data encoding; + `__ptype`-tagged values are returned or not according to the compiler's own + `rich_data` setting, independent of the requested format. + +The behaviour in sections 7.1-7.3 was verified against a deployed OpenVox +compiler and PuppetDB on 2026-08-25. Fixture captures from the specific +compiler and PuppetDB versions in use remain the condition for declaring any +other combination supported. + ## 8. Target-file shape The following schema illustrates the required global defaults and per-target @@ -428,7 +503,23 @@ targets: For a development branch, `baseline.source: file` points to a snapshot captured after the target's main/production environment was deployed. A `baseline.source: puppetdb` request intentionally means PuppetDB's current -latest catalog, regardless of its environment. +latest catalog, regardless of its environment, and is available only with +`catalog_api: v4` (requirement 1.8, section 7.2). + +The two baseline sources answer different questions, and in CI the difference +matters more than the convenience: + +- **`baseline.source: puppetdb`** compares *what the target last received* + against *what it would receive now*. It is the right baseline for asking + whether a node has drifted from what the deployed code produces, but it + depends on the target having run recently in the baseline environment, and + the comparison mixes code changes with fact changes since that run. +- **`baseline.source: file`**, captured with `piace capture catalog + --environment ` from the same factset, compares + *baseline code now* against *candidate code now* against *identical facts*. + Nothing but the environment differs, so a difference is attributable to the + change under review. This is the better baseline for a CI gate on a code + change, and it does not depend on the target's agent-run schedule. ## 9. Source material @@ -437,4 +528,5 @@ latest catalog, regardless of its environment. - [Trusted-facts research](../../../docs/research/trusted-facts-in-existing-catalog-diff-tools.md) - [Puppet Server v4 catalog API](https://help.puppet.com/core/current/Content/PuppetCore/server/http_api/puppet-api/v4/catalog.htm) - [OpenVox v3 catalog API](https://github.com/openvoxproject/openvox/blob/main/api/docs/http_catalog.md) +- [Puppet v3 file_content API](https://github.com/puppetlabs/puppet/blob/main/api/docs/http_file_content.md) - [PuppetDB resources query API](https://github.com/puppetlabs/puppetdb/blob/main/documentation/api/query/v4/resources.markdown) diff --git a/.kiro/specs/piace/tasks.md b/.kiro/specs/piace/tasks.md index 71e3f67..4457585 100644 --- a/.kiro/specs/piace/tasks.md +++ b/.kiro/specs/piace/tasks.md @@ -65,12 +65,17 @@ and deterministic-output invariants. Requirement references point to returned target and candidate environment, and collect compiler provenance. - Implement explicit v4 target trusted-fact handling and fail a v4 request when neither a validated input nor configured compiler lookup is available. + - Send `persistence: {facts: false, catalog: false}` on every v4 request, + unconditionally, and send an endpoint-appropriate `Accept` header on every + v3 request. - Permit v4-to-v3 fallback only when explicitly enabled and only for a verified unsupported-v4 response; emit the non-suppressible service- identity trusted-fact warning for every v3 catalog. - - Reuse this path for catalog snapshot capture; do not claim v4 behavior for - OpenVox without an operator-selected, documented compatibility contract. - - _Requirements: 1.4-1.5, 2.3-2.5, 7 compatibility constraints, 11.2-11.5_ + - Reuse this path for catalog snapshot capture. Treat Puppet Server and + OpenVox identically: both serve v3 and v4 and both honour the v4 + `persistence` field, so the adapter carries no implementation-specific + branch. + - _Requirements: 1.4-1.8, 2.3-2.5, 7 compatibility constraints, 11.2-11.5_ - [x] 7. Normalize Puppet catalogs into a deterministic semantic graph - Validate catalog resource and edge structures; construct exact @@ -199,7 +204,28 @@ and deterministic-output invariants. Requirement references point to - Protocol adapters remain the compatibility boundary. Their exact requests and responses must be demonstrated with fixtures from the deployed service versions before declaring a compiler/PuppetDB combination supported. -- No task authorizes candidate facts or catalogs to be persisted to PuppetDB. +- No task authorizes PIACE to write to PuppetDB, and no task authorizes a + candidate compilation to be persisted where the API version allows that to be + suppressed. v4 suppresses it (`persistence: {facts: false, catalog: false}`); + v3 has no such control, and the compiler stores the candidate facts and + catalog on every v3 request. That is why v3 is a degraded path requiring a + file baseline (requirements.md 1.8, 7.2), not a second supported one. + +- [ ] 13. Enforce and disclose the v3 persistence constraint + - Reject `baseline.source: puppetdb` for any target that can compile over + v3 — `catalog_api: v3`, and `catalog_api: v4` with `allow_v3_fallback: + true` — during configuration resolution, with a diagnostic naming the + reason: a v3 candidate compilation overwrites the stored catalog the + baseline reads. The rule belongs beside the existing `allow_v3_fallback` + validation. + - The existing v3 acceptance tests baseline from the fake PuppetDB, which + this rule forbids; move `TestAcceptance_V3WarningAppearsInEveryFormat` and + `TestAcceptance_V4ToV3Fallback` to a file-backed baseline as part of the + change rather than treating their failure as a regression. + - Extend the non-suppressible v3 warning so it states the PuppetDB mutation + as well as the `$trusted` caveat, and update the outcome/renderer fixtures + that pin the exact warning text. + - _Requirements: 1.6-1.8, 2.5-2.7, 7.2_ ## Task Dependency Graph @@ -215,7 +241,8 @@ and deterministic-output invariants. Requirement references point to {"wave": 7, "tasks": [9]}, {"wave": 8, "tasks": [10]}, {"wave": 9, "tasks": [11]}, - {"wave": 10, "tasks": [12]} + {"wave": 10, "tasks": [12]}, + {"wave": 11, "tasks": [13]} ] } ``` @@ -223,7 +250,7 @@ and deterministic-output invariants. Requirement references point to ```text 1 -> 2 -> 3 -> 4 -> 5 \-> 6 -4 + 5 + 6 -> 7 -> 8 -> 9 -> 10 -> 11 -> 12 +4 + 5 + 6 -> 7 -> 8 -> 9 -> 10 -> 11 -> 12 -> 13 ``` Tasks 4, 5, and 6 may proceed in parallel after task 3. Task 7 depends on their diff --git a/CONTEXT.md b/CONTEXT.md index 259da2e..f4f38eb 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -13,7 +13,9 @@ _Avoid_: old catalog, stored catalog, production catalog **Candidate catalog**: The catalog compiled by the configured compiler for a target certname in the CI -environment under test, using the selected v3 or v4 compiler catalog API. +environment under test, using the selected v3 or v4 compiler catalog API. A v4 +candidate compilation is not persisted; a v3 one is stored by the compiler in +PuppetDB and overwrites the target's stored catalog and factset. _Avoid_: CI catalog, new catalog **Compiler**: diff --git a/README.md b/README.md index d4ce00c..ee511eb 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,34 @@ the production/default environment, captured after merge, so development-branch runs baseline against a frozen catalog rather than a later one from another environment. +### Debugging a service request + +Every subcommand accepts two options for inspecting what PIACE actually sent +and received. They are separate because they sit on opposite sides of the +redaction boundary in [Output and secrecy](#output-and-secrecy). + +``` +--debug print one line per compiler/PuppetDB request to stderr +--debug-dump-dir DIR additionally write raw request/response bodies to DIR +``` + +`--debug` prints metadata only — method, URL, status, duration, body sizes, +content type, and the response body's top-level JSON *member names*: + +``` +piace capture catalog: debug #002 POST https://compiler.example.test:8140/puppet/v4/catalog -> 200 in 1.069s (request 24580 B, response 18362 B, content-type application/json, body object, top-level keys: catalog) +``` + +Those top-level keys are the fastest way to spot a wire-shape mismatch between +PIACE and a compiler or PuppetDB version, and they contain no catalog values, +so the output is safe for a CI log. + +`--debug-dump-dir` writes the verbatim request and response bodies to `0600` +files in a `0700` directory, never to stdout or stderr. Those bodies are +**unredacted**: they can contain Puppet `Sensitive` values and managed file +content. Use it on a workstation, not in CI, and delete the directory +afterwards. + ## Configuration Two files, deliberately separate: the reviewable selection/policy file, and the @@ -95,7 +123,7 @@ version: 1 defaults: candidate: environment: feature-123 - catalog_api: v4 # v3 | v4 + catalog_api: v4 # v3 | v4 — v4 unless the compiler lacks it allow_v3_fallback: false # valid only with v4; opt-in, never implicit facts: source: puppetdb # puppetdb | file @@ -153,6 +181,66 @@ The compiler identity is a dedicated **catalog-reader certificate** whose `auth.conf` rule grants it catalog reads for target certnames other than its own. +### Authorizing the catalog-reader certificate + +A stock compiler lets nobody use the v4 endpoint, so PIACE gets HTTP 403 until +one rule in `/etc/puppetlabs/puppetserver/conf.d/auth.conf` names the +catalog-reader certificate's **subject CN** — not the filename in +`services.yaml`. Edit the stock rule in place rather than appending a new one: +`name` and `sort-order` identify a rule, and a duplicate is a configuration +error. + +```hocon + { + # Stock ships this rule as `deny: "*"`. Replace that deny with + # an allow list; do not leave both in place. + match-request: { + path: "^/puppet/v4/catalog/?$" + type: regex + method: post + } + allow: [ "catalog-reader" ] + sort-order: 500 + name: "puppetlabs v4 catalog for services" + }, +``` + +That is the whole requirement for a v4 setup. The v3 rule below is needed +**only** if you have opted into `catalog_api: v3` or `allow_v3_fallback: true` +— see [Use `catalog_api: v4`](#use-catalog_api-v4) for why that is a degraded +path: + +```hocon + { + # Allow nodes to retrieve their own catalog, and the + # catalog-reader certificate to retrieve anyone's. + match-request: { + path: "^/puppet/v3/catalog/([^/]+)$" + type: regex + method: [get, post] + } + allow: [ "$1", "catalog-reader" ] + sort-order: 500 + name: "puppetlabs v3 catalog from agents" + }, +``` + +`$1` is the certname captured from the request path, and it keeps working +alongside a second entry — ordinary agents still fetch their own catalogs. +Adding a CN beside it grants that certificate *every* target's catalog, which +is the point of a dedicated identity and the reason it should be a certificate +used for nothing else. It is also exactly what makes `$trusted` in a v3 catalog +potentially reflect the reader rather than the target. + +Reload the compiler after editing (`systemctl reload puppetserver`). No rule +change is needed for managed-File content evidence: the stock +`"puppetlabs file"` rule already covers `/puppet/v3/file_content/`, which a +catalog-reader certificate can therefore use for any target's files. + +PuppetDB is authorized separately, by its own certificate allowlist or by +accepting any certificate signed by the CA, depending on how the installation +is configured. + ## Exit codes | Code | Outcome | Meaning | @@ -166,16 +254,48 @@ Precedence is `30 > 20 > 10 > differences_allowed > clean`. A run is never `clean` while any target has an unreported retrieval, compilation, or normalization failure — an indeterminate File-content comparison included. +## Use `catalog_api: v4` + +v4 is the supported path, and the reason is not trusted facts alone. Every v4 +request PIACE makes carries `persistence: {facts: false, catalog: false}`: the +compiler returns the candidate catalog and writes nothing. The target's stored +factset and catalog stay exactly as its last real agent run left them. + +The v3 catalog endpoint has no equivalent control, and the consequence is not +cosmetic. On every v3 request the compiler saves the facts you submitted — +rewriting the target's stored factset and its `facts_environment` to the +candidate environment — and stores the compiled catalog through its PuppetDB +catalog cache terminus, rewriting the target's stored catalog, +`catalog_environment`, and `transaction_uuid`. That is a property of the +endpoint. Nothing PIACE sends can turn it off. + +So with `catalog_api: v3`: + +- **`baseline.source: puppetdb` cannot work.** PIACE reads the baseline, then + compiles the candidate, and the candidate compilation overwrites the baseline + — for the next target in the same run, and for every later run. The symptom + is a baseline-environment mismatch that names the candidate environment. A v3 + target needs `baseline.source: file`, captured while the baseline + environment's catalog was the stored one. +- **A file baseline does not make v3 harmless.** It stops PIACE from destroying + its own input. It does not stop the compiler from writing the candidate facts + and catalog into PuppetDB, where anything reading PuppetDB state — reporting, + exported resources, inventory, classification keyed on `facts_environment` — + sees candidate values until the target's next agent run. + +Puppet Server and OpenVox behave identically here: both serve v3 and v4, and +both honour the v4 `persistence` field. `catalog_api` is the only thing that +decides. + ## Two things the reports say, and mean literally -**The v3 trusted-fact warning.** With `catalog_api: v3` — or any permitted -v4→v3 fallback — `$trusted` in the compiled catalog can reflect the -catalog-reader certificate rather than the target. The warning is -non-suppressible and appears in all three formats. It does not change the exit -status; it makes the trust semantics reviewable. v4 uses the compiler's target -trusted-fact mechanism, and fails compilation rather than inventing trusted -facts when neither a validated input nor a configured compiler lookup is -available. +**The v3 warning.** With `catalog_api: v3` — or any permitted v4→v3 fallback — +`$trusted` in the compiled catalog can reflect the catalog-reader certificate +rather than the target. The warning is non-suppressible and appears in all +three formats. It does not change the exit status; it makes the trust semantics +reviewable. v4 sends the target's own trusted facts, and fails compilation +rather than inventing them when neither a validated input nor a configured +compiler lookup is available. **The impact estimate.** It reports only that a node's latest *stored* catalog contains the exact `Type[title]`. It is not proof those nodes would change, and @@ -199,6 +319,11 @@ exact type and parameter name. No report carries credentials, private key material, managed file content bytes, or unredacted sensitive values — asserted end to end over all three formats in `cmd/piace/acceptance_disclosure_test.go`. +That boundary holds for `--debug` too, which reports only request metadata and +response top-level member names. `--debug-dump-dir` is the one deliberate +exception: an operator-requested dump of verbatim bodies to `0600` files, never +to a console or a report. + ## Snapshots `piace capture` writes PIACE envelopes, not bare Puppet payloads: format diff --git a/cmd/piace/acceptance_debug_test.go b/cmd/piace/acceptance_debug_test.go new file mode 100644 index 0000000..543648d --- /dev/null +++ b/cmd/piace/acceptance_debug_test.go @@ -0,0 +1,135 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestAcceptance_DebugPrintsSafeRequestMetadata covers the --debug +// option's contract: one stderr line per service request, carrying +// enough to diagnose a wire-shape mismatch (status, timing, sizes, and +// the response body's top-level JSON keys) and nothing that +// requirements.md 3.5 forbids in a log. +func TestAcceptance_DebugPrintsSafeRequestMetadata(t *testing.T) { + h := newHarness(t) + h.seedTarget("web-01.example.test", baseResources(), baseResources(), baseEdges()) + h.writeConfigs(t, targetsYAML(defaultDefaults, target("web-01.example.test"))) + + got := h.compare(t, "--debug") + + for _, want := range []string{ + "POST https://" + h.compilerServer.Listener.Addr().String() + "/puppet/v4/catalog", + "/pdb/query/v4/catalogs/web-01.example.test", + "-> 200 in ", + "top-level keys: catalog", + } { + if !strings.Contains(got.stderr, want) { + t.Errorf("--debug stderr does not contain %q:\n%s", want, got.stderr) + } + } + + // The debug lines are metadata only: no catalog member below the top + // level, and no resource content, reaches stderr. + for _, forbidden := range []string{"Service[nginx]", "\"resources\"", "ensure"} { + if strings.Contains(got.stderr, forbidden) { + t.Errorf("--debug stderr leaked body content %q:\n%s", forbidden, got.stderr) + } + } +} + +// TestAcceptance_DebugLeavesStdoutUnchanged asserts observation is inert: +// enabling --debug must not alter the report or the exit code, since an +// operator turns it on precisely to inspect a run they need to reproduce. +func TestAcceptance_DebugLeavesStdoutUnchanged(t *testing.T) { + h := newHarness(t) + h.seedTarget("web-01.example.test", baseResources(), baseResources(), baseEdges()) + h.writeConfigs(t, targetsYAML(defaultDefaults, target("web-01.example.test"))) + + plain := h.compare(t) + debugged := h.compare(t, "--debug") + + if plain.code != debugged.code { + t.Errorf("exit code changed with --debug: %d -> %d", plain.code, debugged.code) + } + if plain.stdout != debugged.stdout { + t.Errorf("stdout changed with --debug:\n%s\n---\n%s", plain.stdout, debugged.stdout) + } + if plain.stderr != "" { + t.Errorf("stderr is not empty without --debug:\n%s", plain.stderr) + } +} + +// TestAcceptance_DebugDumpDirWritesRestrictedFiles covers the other side +// of the redaction boundary: raw bodies land in 0600 files under an +// operator-named directory, never on stdout or stderr, and the command +// says out loud that they can contain sensitive values. +func TestAcceptance_DebugDumpDirWritesRestrictedFiles(t *testing.T) { + h := newHarness(t) + h.seedTarget("web-01.example.test", baseResources(), baseResources(), baseEdges()) + h.writeConfigs(t, targetsYAML(defaultDefaults, target("web-01.example.test"))) + + dumpDir := h.path("dump") + got := h.compare(t, "--debug-dump-dir", dumpDir) + + if !strings.Contains(got.stderr, "may contain sensitive catalog values") { + t.Errorf("no warning printed for --debug-dump-dir:\n%s", got.stderr) + } + + entries, err := os.ReadDir(dumpDir) + if err != nil { + t.Fatalf("ReadDir(%s): %v", dumpDir, err) + } + if len(entries) == 0 { + t.Fatal("--debug-dump-dir produced no files") + } + + dirInfo, err := os.Stat(dumpDir) + if err != nil { + t.Fatalf("Stat(%s): %v", dumpDir, err) + } + if perm := dirInfo.Mode().Perm(); perm != 0o700 { + t.Errorf("dump directory mode = %04o, want 0700", perm) + } + + sawV4Response := false + for _, e := range entries { + info, err := e.Info() + if err != nil { + t.Fatalf("Info(%s): %v", e.Name(), err) + } + if perm := info.Mode().Perm(); perm != 0o600 { + t.Errorf("%s mode = %04o, want 0600", e.Name(), perm) + } + if !strings.HasSuffix(e.Name(), "puppet-v4-catalog.response.json") { + continue + } + sawV4Response = true + body, err := os.ReadFile(filepath.Join(dumpDir, e.Name())) + if err != nil { + t.Fatalf("ReadFile(%s): %v", e.Name(), err) + } + // The dump is the verbatim response, envelope included — that is + // the whole point of having it. + if !strings.HasPrefix(strings.TrimSpace(string(body)), `{"catalog":`) { + t.Errorf("v4 response dump is not the raw enveloped body:\n%s", body) + } + } + if !sawV4Response { + t.Errorf("no v4 catalog response dump written; got %v", names(entries)) + } + + // Nothing raw reaches the console. + if strings.Contains(got.stderr, "Service") || strings.Contains(got.stdout, `{"catalog":`) { + t.Error("raw body content reached stdout/stderr") + } +} + +func names(entries []os.DirEntry) []string { + out := make([]string, 0, len(entries)) + for _, e := range entries { + out = append(out, e.Name()) + } + return out +} diff --git a/cmd/piace/acceptance_fixture_test.go b/cmd/piace/acceptance_fixture_test.go index 97f3498..f9d2497 100644 --- a/cmd/piace/acceptance_fixture_test.go +++ b/cmd/piace/acceptance_fixture_test.go @@ -264,12 +264,17 @@ func compilerCatalog(certname, environment string, resources []resourceSpec, edg "file": "/etc/puppetlabs/code/site.pp", "line": 42, "exported": false, }) } + // A compiler serializes each edge vertex as a `Type[title]` reference + // string (Puppet::Relationship#to_data_hash calls source.to_s / + // target.to_s), not as the `{type, title}` object of PuppetDB's wire + // format v8. Emitting the object form here — as this fixture + // originally did — exercises a shape no compiler ever sends. See + // internal/normalize/wire.go's resourceSpecWire. edgeList := make([]map[string]any, 0, len(edges)) for _, e := range edges { edgeList = append(edgeList, map[string]any{ - "source": map[string]any{"type": e.SourceType, "title": e.SourceTitle}, - "target": map[string]any{"type": e.TargetType, "title": e.TargetTitle}, - "relationship": "contains", + "source": e.SourceType + "[" + e.SourceTitle + "]", + "target": e.TargetType + "[" + e.TargetTitle + "]", }) } return map[string]any{ diff --git a/cmd/piace/acceptance_service_test.go b/cmd/piace/acceptance_service_test.go index d9388b1..19663c9 100644 --- a/cmd/piace/acceptance_service_test.go +++ b/cmd/piace/acceptance_service_test.go @@ -147,8 +147,15 @@ type fakeCompiler struct { // v3Status behaves the same way for POST /puppet/v3/catalog/:certname. v3Status int // catalogs is keyed by certname and holds the compiler wire-format - // catalog body to return from whichever endpoint is used. + // catalog *document* to return. serveCatalog wraps it in whichever + // envelope the answering endpoint uses. catalogs map[string]any + // rawBodies is keyed by certname and, when set, is returned verbatim + // with status 200 in place of any catalog — no endpoint envelope + // applied. It exists so a test can exercise the adapter's + // semantic-rejection probe, which reads the outer response body + // before any envelope is unwrapped. + rawBodies map[string]any // fileContent is keyed by the mount path segment the resolver builds // from a `puppet://` source reference. fileContent map[string]string @@ -159,7 +166,7 @@ type fakeCompiler struct { } func newFakeCompiler() *fakeCompiler { - return &fakeCompiler{catalogs: map[string]any{}, fileContent: map[string]string{}} + return &fakeCompiler{catalogs: map[string]any{}, rawBodies: map[string]any{}, fileContent: map[string]string{}} } func (f *fakeCompiler) handler() http.Handler { @@ -178,14 +185,14 @@ func (f *fakeCompiler) handler() http.Handler { return } certname, _ := body["certname"].(string) - f.serveCatalog(w, certname) + f.serveCatalog(w, certname, apiV4) case strings.HasPrefix(r.URL.Path, "/puppet/v3/catalog/"): if f.v3Status != 0 { w.WriteHeader(f.v3Status) writeJSON(w, map[string]any{"error": "forced status"}) return } - f.serveCatalog(w, strings.TrimPrefix(r.URL.Path, "/puppet/v3/catalog/")) + f.serveCatalog(w, strings.TrimPrefix(r.URL.Path, "/puppet/v3/catalog/"), apiV3) case strings.HasPrefix(r.URL.Path, "/puppet/v3/file_content/"): content, ok := f.fileContent[strings.TrimPrefix(r.URL.Path, "/puppet/v3/file_content/")] if !ok { @@ -200,12 +207,33 @@ func (f *fakeCompiler) handler() http.Handler { }) } -func (f *fakeCompiler) serveCatalog(w http.ResponseWriter, certname string) { +// catalogAPI selects which endpoint's response envelope serveCatalog +// wraps a catalog document in. The two endpoints differ: v3 returns the +// document as the whole body, v4 returns `{"catalog": }` (see +// internal/compiler/doc.go for the primary sources). Serving one shape +// from both endpoints, as this fixture originally did, hides that +// difference from every acceptance test that exercises the v4 path. +type catalogAPI int + +const ( + apiV3 catalogAPI = iota + apiV4 +) + +func (f *fakeCompiler) serveCatalog(w http.ResponseWriter, certname string, api catalogAPI) { + if raw, ok := f.rawBodies[certname]; ok { + writeJSON(w, raw) + return + } catalog, ok := f.catalogs[certname] if !ok { http.Error(w, "no catalog", http.StatusNotFound) return } + if api == apiV4 { + writeJSON(w, map[string]any{"catalog": catalog}) + return + } writeJSON(w, catalog) } diff --git a/cmd/piace/acceptance_test.go b/cmd/piace/acceptance_test.go index 88cee94..035a68d 100644 --- a/cmd/piace/acceptance_test.go +++ b/cmd/piace/acceptance_test.go @@ -233,7 +233,10 @@ func TestAcceptance_OutcomePrecedence(t *testing.T) { // override for the whole compiler when that target is in play. for _, certname := range tc.targets { if certname == "compilefail.example.test" { - h.compiler.catalogs["compilefail.example.test"] = map[string]any{ + // A semantic rejection is served as the whole + // response body, with no catalog envelope around it: + // the adapter's error probe reads the outer body. + h.compiler.rawBodies["compilefail.example.test"] = map[string]any{ "error": "Evaluation Error: Unknown class site::missing", } } diff --git a/cmd/piace/debug.go b/cmd/piace/debug.go new file mode 100644 index 0000000..10f83a3 --- /dev/null +++ b/cmd/piace/debug.go @@ -0,0 +1,201 @@ +package main + +import ( + "encoding/json" + "fmt" + "io" + "os" + "path/filepath" + "strings" + "sync" + + "github.com/example42/piace/internal/transport" +) + +// debugFlags holds the two observation options every subcommand accepts. +// +// They are deliberately separate because they sit on opposite sides of +// requirements.md 3.5 ("SHALL NOT log private keys, certificate private +// material, request authorization headers, or unredacted sensitive +// catalog parameter values"): +// +// - --debug prints safe metadata only — method, URL, status, timing, +// body sizes, content type, and the response body's top-level JSON +// *member names*. That is enough to diagnose a wire-shape mismatch +// (a v4 response whose only top-level key is "catalog", say) and +// contains no catalog values, so it is safe for a CI log. +// - --debug-dump-dir writes the raw request and response bodies to +// 0600 files in an operator-named directory. Those bodies can carry +// Puppet Sensitive values and unredacted catalog parameters, so they +// never go to stdout or stderr: this is an operator-requested dump +// to a restricted path, not logging, and the command says so out +// loud when it is enabled. +type debugFlags struct { + debug bool + dumpDir string +} + +// register adds both options to fs. Every subcommand calls this so the +// observation seam is uniform across compare and both captures. +func (d *debugFlags) register(fs interface { + BoolVar(*bool, string, bool, string) + StringVar(*string, string, string, string) +}) { + fs.BoolVar(&d.debug, "debug", false, + "print one line per service request to stderr: method, URL, status, duration, body sizes, and the response's top-level JSON keys (no body content)") + fs.StringVar(&d.dumpDir, "debug-dump-dir", "", + "write raw request/response bodies to 0600 files in this directory; bodies may contain sensitive catalog values and are never printed to stdout/stderr") +} + +// enabled reports whether any observation was requested. +func (d debugFlags) enabled() bool { return d.debug || d.dumpDir != "" } + +// transportOptions builds the transport.Option list every service client +// in this invocation is constructed with. It returns nil when neither +// option was requested, so the default code path is untouched. +// +// label is the subcommand name, used to prefix the stderr lines and to +// name the dump directory's files. +func (d debugFlags) transportOptions(label string, stderr io.Writer) ([]transport.Option, error) { + if !d.enabled() { + return nil, nil + } + sink := &debugSink{label: label, stderr: stderr, printMetadata: d.debug, dumpDir: d.dumpDir} + if d.dumpDir != "" { + // 0700: the dump directory holds unredacted request/response + // bodies, so it is created no more readable than the 0600 files + // inside it. An existing directory's mode is left alone — that is + // the operator's choice, not this command's to override. + if err := os.MkdirAll(d.dumpDir, 0o700); err != nil { + return nil, fmt.Errorf("creating --debug-dump-dir: %w", err) + } + fmt.Fprintf(stderr, "piace %s: writing raw request/response bodies to %s; they may contain sensitive catalog values\n", label, d.dumpDir) + } + opts := []transport.Option{transport.WithObserver(sink.observe)} + if d.dumpDir != "" { + opts = append(opts, transport.WithBodyCapture(true)) + } + return opts, nil +} + +// debugSink renders transport.Event values. One sink is shared by every +// client in an invocation so the dump-file sequence numbers reflect the +// real request order across both services. +type debugSink struct { + label string + stderr io.Writer + printMetadata bool + dumpDir string + + mu sync.Mutex + seq int +} + +// observe implements transport.Observer. It is called synchronously from +// transport.Client.Do. +func (s *debugSink) observe(ev transport.Event) { + s.mu.Lock() + s.seq++ + seq := s.seq + s.mu.Unlock() + + if s.printMetadata { + fmt.Fprintf(s.stderr, "piace %s: debug #%03d %s\n", s.label, seq, describeEvent(ev)) + } + if s.dumpDir == "" { + return + } + base := fmt.Sprintf("%03d-%s-%s", seq, strings.ToLower(ev.Method), slugPath(ev.URL)) + s.dump(base+".request", ev.RequestBody) + s.dump(base+".response", ev.ResponseBody) +} + +// dump writes one body to a 0600 file. A dump failure is reported to +// stderr but never fails the run: observation must not change a +// comparison's or capture's outcome. +func (s *debugSink) dump(base string, body []byte) { + if len(body) == 0 { + return + } + name := base + ".bin" + if json.Valid(body) { + name = base + ".json" + } + path := filepath.Join(s.dumpDir, name) + if err := os.WriteFile(path, body, 0o600); err != nil { + fmt.Fprintf(s.stderr, "piace %s: debug: writing %s: %v\n", s.label, path, err) + } +} + +// describeEvent renders one Event as a single safe line. Every field it +// prints is metadata; no body content reaches it (transport.Event's +// TopLevelKeys carries member names only — see internal/transport/debug.go). +func describeEvent(ev transport.Event) string { + var b strings.Builder + fmt.Fprintf(&b, "%s %s", ev.Method, ev.URL) + if ev.Err != nil { + fmt.Fprintf(&b, " -> no response after %s: %s", ev.Duration, transport.SafeMessage(ev.Err)) + return b.String() + } + fmt.Fprintf(&b, " -> %d in %s", ev.StatusCode, ev.Duration) + if ev.RequestBodyBytes >= 0 { + fmt.Fprintf(&b, " (request %d B", ev.RequestBodyBytes) + } else { + b.WriteString(" (request unknown size") + } + fmt.Fprintf(&b, ", response %d B", ev.ResponseBodyBytes) + if ev.ContentType != "" { + fmt.Fprintf(&b, ", content-type %s", ev.ContentType) + } + fmt.Fprintf(&b, ", body %s", ev.Shape) + if ev.Shape == transport.ShapeObject { + keys := strings.Join(ev.TopLevelKeys, ",") + if ev.KeysTruncated { + keys += ",..." + } + fmt.Fprintf(&b, ", top-level keys: %s", keys) + } + b.WriteString(")") + return b.String() +} + +// slugPath reduces a request URL to a short filesystem-safe fragment for +// a dump file name: its path only, with separators and any other +// non-alphanumeric character collapsed to "-". The query string is +// dropped so a PuppetDB query never lands in a file name. +func slugPath(rawURL string) string { + path := rawURL + if i := strings.Index(path, "://"); i >= 0 { + path = path[i+3:] + if j := strings.Index(path, "/"); j >= 0 { + path = path[j:] + } else { + path = "/" + } + } + if i := strings.IndexAny(path, "?#"); i >= 0 { + path = path[:i] + } + var b strings.Builder + lastDash := true + for _, r := range path { + switch { + case (r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9'): + b.WriteRune(r) + lastDash = false + default: + if !lastDash { + b.WriteByte('-') + lastDash = true + } + } + } + slug := strings.Trim(b.String(), "-") + if slug == "" { + slug = "request" + } + if len(slug) > 80 { + slug = slug[:80] + } + return slug +} diff --git a/cmd/piace/main.go b/cmd/piace/main.go index bb61e98..a5bbddb 100644 --- a/cmd/piace/main.go +++ b/cmd/piace/main.go @@ -75,7 +75,14 @@ func usage() string { [--text-out PATH] [--json-out PATH] [--html-out PATH] piace capture facts --targets TARGETS.yaml --services SERVICES.yaml piace capture catalog --targets TARGETS.yaml --services SERVICES.yaml \ - --environment ENVIRONMENT` + --environment ENVIRONMENT + +Every subcommand also accepts: + --debug print one line per service request to stderr (method, + URL, status, duration, body sizes, response top-level + JSON keys); no body content is printed + --debug-dump-dir DIR additionally write raw request/response bodies to 0600 + files in DIR; they may contain sensitive catalog values` } // compareFlags holds the parsed --compare flags. Kept as a struct so tests @@ -86,6 +93,7 @@ type compareFlags struct { textOut string jsonOut string htmlOut string + debug debugFlags } func runCompare(args []string, stdout, stderr *os.File) exitcode.Code { @@ -97,6 +105,7 @@ func runCompare(args []string, stdout, stderr *os.File) exitcode.Code { fs.StringVar(&f.textOut, "text-out", "", "path to write the text report (default: stdout)") fs.StringVar(&f.jsonOut, "json-out", "", "path to write the versioned JSON report") fs.StringVar(&f.htmlOut, "html-out", "", "path to write the static HTML report") + f.debug.register(fs) if err := fs.Parse(args); err != nil { return exitcode.OperationalError } @@ -111,7 +120,13 @@ func runCompare(args []string, stdout, stderr *os.File) exitcode.Code { return exitcode.OperationalError } - workflow, err := newCompareWorkflow(cfg) + debugOpts, err := f.debug.transportOptions("compare", stderr) + if err != nil { + fmt.Fprintf(stderr, "piace compare: %s\n", err) + return exitcode.OperationalError + } + + workflow, err := newCompareWorkflow(cfg, debugOpts) if err != nil { fmt.Fprintf(stderr, "piace compare: %s\n", err) return exitcode.OperationalError @@ -139,20 +154,20 @@ func runCompare(args []string, stdout, stderr *os.File) exitcode.Code { // The compiler and PuppetDB clients are built independently from their // own resolved endpoints, per design.md section 2.2, so neither service's // credentials can reach the other. -func newCompareWorkflow(cfg resolve.Config) (*compare.Workflow, error) { - puppetDBAdapter, err := newPuppetDBAdapter(cfg) +func newCompareWorkflow(cfg resolve.Config, debugOpts []transport.Option) (*compare.Workflow, error) { + puppetDBAdapter, err := newPuppetDBAdapter(cfg, debugOpts) if err != nil { return nil, err } - compilerAdapter, err := newCompilerAdapter(cfg) + compilerAdapter, err := newCompilerAdapter(cfg, debugOpts) if err != nil { return nil, err } - compilerClient, err := transport.NewClient(cfg.Services.Compiler) + compilerClient, err := transport.NewClient(cfg.Services.Compiler, debugOpts...) if err != nil { return nil, fmt.Errorf("building compiler content client: %w", err) } - puppetDBClient, err := transport.NewClient(cfg.Services.PuppetDB) + puppetDBClient, err := transport.NewClient(cfg.Services.PuppetDB, debugOpts...) if err != nil { return nil, fmt.Errorf("building puppetdb impact client: %w", err) } @@ -233,6 +248,7 @@ type captureFlags struct { services string environment string replace bool + debug debugFlags } func runCapture(args []string, stdout, stderr *os.File) exitcode.Code { @@ -259,6 +275,7 @@ func runCaptureFacts(args []string, stdout, stderr *os.File) exitcode.Code { fs.StringVar(&f.targets, "targets", "", "path to the target YAML file (required)") fs.StringVar(&f.services, "services", "", "path to the services YAML file (required)") fs.BoolVar(&f.replace, "replace", false, "overwrite an existing snapshot") + f.debug.register(fs) if err := fs.Parse(args); err != nil { return exitcode.OperationalError } @@ -273,7 +290,13 @@ func runCaptureFacts(args []string, stdout, stderr *os.File) exitcode.Code { return exitcode.OperationalError } - puppetDBFacts, err := newPuppetDBAdapter(cfg) + debugOpts, err := f.debug.transportOptions("capture facts", stderr) + if err != nil { + fmt.Fprintf(stderr, "piace capture facts: %s\n", err) + return exitcode.OperationalError + } + + puppetDBFacts, err := newPuppetDBAdapter(cfg, debugOpts) if err != nil { fmt.Fprintf(stderr, "piace capture facts: %s\n", err) return exitcode.OperationalError @@ -297,6 +320,7 @@ func runCaptureCatalog(args []string, stdout, stderr *os.File) exitcode.Code { fs.StringVar(&f.services, "services", "", "path to the services YAML file (required)") fs.StringVar(&f.environment, "environment", "", "candidate environment to request the catalog from (required)") fs.BoolVar(&f.replace, "replace", false, "overwrite an existing snapshot") + f.debug.register(fs) if err := fs.Parse(args); err != nil { return exitcode.OperationalError } @@ -311,13 +335,19 @@ func runCaptureCatalog(args []string, stdout, stderr *os.File) exitcode.Code { return exitcode.OperationalError } - puppetDBFacts, err := newPuppetDBAdapter(cfg) + debugOpts, err := f.debug.transportOptions("capture catalog", stderr) + if err != nil { + fmt.Fprintf(stderr, "piace capture catalog: %s\n", err) + return exitcode.OperationalError + } + + puppetDBFacts, err := newPuppetDBAdapter(cfg, debugOpts) if err != nil { fmt.Fprintf(stderr, "piace capture catalog: %s\n", err) return exitcode.OperationalError } - compilerAdapter, err := newCompilerAdapter(cfg) + compilerAdapter, err := newCompilerAdapter(cfg, debugOpts) if err != nil { fmt.Fprintf(stderr, "piace capture catalog: %s\n", err) return exitcode.OperationalError @@ -337,8 +367,8 @@ func runCaptureCatalog(args []string, stdout, stderr *os.File) exitcode.Code { // newPuppetDBAdapter builds the PuppetDB-backed fact/baseline source // adapter (task 4) from cfg's resolved PuppetDB service endpoint, per // task 3's hardened mTLS transport construction. -func newPuppetDBAdapter(cfg resolve.Config) (*puppetdb.Adapter, error) { - client, err := transport.NewClient(cfg.Services.PuppetDB) +func newPuppetDBAdapter(cfg resolve.Config, debugOpts []transport.Option) (*puppetdb.Adapter, error) { + client, err := transport.NewClient(cfg.Services.PuppetDB, debugOpts...) if err != nil { return nil, fmt.Errorf("building puppetdb client: %w", err) } @@ -352,8 +382,8 @@ func newPuppetDBAdapter(cfg resolve.Config) (*puppetdb.Adapter, error) { // way, so they share the exact same request/policy implementation // (design.md section 5: "Capture catalog uses the exact same adapter and // policy as comparison"). -func newCompilerAdapter(cfg resolve.Config) (*compiler.Adapter, error) { - client, err := transport.NewClient(cfg.Services.Compiler) +func newCompilerAdapter(cfg resolve.Config, debugOpts []transport.Option) (*compiler.Adapter, error) { + client, err := transport.NewClient(cfg.Services.Compiler, debugOpts...) if err != nil { return nil, fmt.Errorf("building compiler client: %w", err) } diff --git a/docs/adr/0001-request-candidate-catalogs-from-an-existing-compiler.md b/docs/adr/0001-request-candidate-catalogs-from-an-existing-compiler.md index ff7cc8b..fdf5db7 100644 --- a/docs/adr/0001-request-candidate-catalogs-from-an-existing-compiler.md +++ b/docs/adr/0001-request-candidate-catalogs-from-an-existing-compiler.md @@ -10,6 +10,14 @@ catalog snapshots as baselines, allowing CI to compare a development candidate with an intentional capture from each target's default environment rather than with PuppetDB's latest catalog regardless of environment. +Borrowing the deployed compiler means accepting its persistence behaviour. The +v4 catalog API lets a request disable fact and catalog persistence, so a v4 +candidate compilation leaves PuppetDB untouched; PIACE sets those fields on +every v4 request. The v3 catalog API has no such control: the compiler saves +the submitted facts and stores the compiled catalog under the candidate +environment. v3 therefore remains available as a degraded path, constrained to +a file-backed baseline, rather than an equivalent one. + Local fact and catalog snapshots are PIACE envelopes rather than bare Puppet payloads: they record source, target, environment where applicable, capture metadata, input identity, and an integrity checksum. A PuppetDB baseline whose diff --git a/docs/research/trusted-facts-in-existing-catalog-diff-tools.md b/docs/research/trusted-facts-in-existing-catalog-diff-tools.md index 09ff2b8..727a997 100644 --- a/docs/research/trusted-facts-in-existing-catalog-diff-tools.md +++ b/docs/research/trusted-facts-in-existing-catalog-diff-tools.md @@ -46,18 +46,46 @@ Sources: [upstream project README](https://github.com/github/octocatalog-diff/bl ### OpenVox compatibility -OpenVox currently documents the v3 catalog endpoint only. It does not document -Puppet Server's v4 catalog endpoint or v4 `trusted_facts` behaviour. Therefore -the `puppet-catalog_diff` mitigation cannot be assumed to work against OpenVox. - -Source: [OpenVox catalog HTTP API](https://github.com/openvoxproject/openvox/blob/main/api/docs/http_catalog.md). +OpenVox's published HTTP API docs describe the v3 catalog endpoint and do not +describe a v4 catalog endpoint or v4 `trusted_facts` behaviour. Undocumented is +not the same as unimplemented, and here the distinction decides whether the +`puppet-catalog_diff` mitigation is available at all: measured against a +deployed OpenVox compiler on 2026-08-25, `POST /puppet/v4/catalog` returns 200 +with a `{"catalog": ...}` envelope, accepts `trusted_facts`, and honours +`persistence: {facts: false, catalog: false}` — a request with persistence +disabled left no factset, no catalog, and no node in PuppetDB for a certname +that had none before. The v4 mitigation works against OpenVox. + +Sources: [OpenVox catalog HTTP API](https://github.com/openvoxproject/openvox/blob/main/api/docs/http_catalog.md); +direct measurement against a deployed OpenVox compiler and PuppetDB. + +### The v3 endpoint's second problem: persistence + +The trusted-fact caveat is the one the existing tools document. Measurement +against the same deployed compiler surfaced a second, independent one: a v3 +catalog request has no persistence control. One `POST +/puppet/v3/catalog/:certname` for a previously unknown certname created, in +PuppetDB, a factset and a catalog under the requested environment — the stored +catalog carrying the `transaction_uuid` the request had supplied. The compiler +saves the facts submitted with the request and stores the compiled catalog +through its PuppetDB catalog cache terminus. Neither is suppressible from the +client, and both apply to any tool that compiles a candidate catalog over v3, +not only to PIACE. ## Consequence for PIACE PIACE allows the CI configuration to select the compiler's v3 or v4 catalog -API. A v3 candidate compilation made with the catalog-reader certificate must -emit a prominent trusted-fact compatibility warning: any target code that uses -`$trusted` can observe that service identity instead of the target identity. -Puppet Server v4 is the documented path that can explicitly provide, or obtain -from PuppetDB, target trusted facts. OpenVox's currently documented v3 API does -not establish an equivalent behaviour. +API, and treats v4 as the supported one for two reasons rather than one. + +v4 explicitly provides the target's trusted facts, or obtains them from +PuppetDB, and disables persistence so the candidate compilation leaves PuppetDB +untouched. It behaves this way on both Puppet Server and OpenVox. + +A v3 candidate compilation made with the catalog-reader certificate must emit a +prominent, non-suppressible warning covering both consequences: any target code +that uses `$trusted` can observe the service identity instead of the target +identity, and the compilation overwrites the target's stored factset and +catalog under the candidate environment. The second consequence also makes a +PuppetDB baseline unusable with v3 — the candidate compilation destroys the +baseline the comparison reads — so a v3 target is constrained to +`baseline.source: file`. diff --git a/internal/compiler/adapter.go b/internal/compiler/adapter.go index ca40f40..ff1f9e9 100644 --- a/internal/compiler/adapter.go +++ b/internal/compiler/adapter.go @@ -94,7 +94,7 @@ func (a *Adapter) requestV3(ctx context.Context, target resolve.Target, flatFact return puppetdb.Catalog{}, model.CandidateProvenance{}, &diag } - cat, diag := processResponse(resp, a.client.Host(), target.Certname, target.Candidate.Environment) + cat, diag := processResponse(resp, a.client.Host(), target.Certname, target.Candidate.Environment, config.CatalogAPIv3) if diag != nil { return puppetdb.Catalog{}, model.CandidateProvenance{}, diag } @@ -151,7 +151,7 @@ func (a *Adapter) requestV4WithFallback(ctx context.Context, target resolve.Targ return cat, prov, nil, nil } - cat, diag := processResponse(resp, host, target.Certname, target.Candidate.Environment) + cat, diag := processResponse(resp, host, target.Certname, target.Candidate.Environment, config.CatalogAPIv4) if diag != nil { return puppetdb.Catalog{}, model.CandidateProvenance{}, nil, diag } diff --git a/internal/compiler/adapter_test.go b/internal/compiler/adapter_test.go index 039fed8..6d2d813 100644 --- a/internal/compiler/adapter_test.go +++ b/internal/compiler/adapter_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "strings" "testing" "time" @@ -60,8 +61,26 @@ func v3Target(certname, environment string) resolve.Target { } } +// wireCatalogBody is a v3 response body: the catalog document, with no +// envelope around it. func wireCatalogBody(name, environment string) []byte { - body, _ := json.Marshal(map[string]any{ + body, _ := json.Marshal(catalogDocumentFixture(name, environment)) + return body +} + +// v4CatalogBody is a v4 response body: the same catalog document wrapped +// in the endpoint's `{"catalog": ...}` envelope. The two helpers are +// deliberately separate rather than one shape reused for both endpoints +// — that conflation is what +// TestAdapter_RequestCandidate_V4RejectsUnwrappedCatalogBody guards +// against reappearing. +func v4CatalogBody(name, environment string) []byte { + body, _ := json.Marshal(map[string]any{"catalog": catalogDocumentFixture(name, environment)}) + return body +} + +func catalogDocumentFixture(name, environment string) map[string]any { + return map[string]any{ "name": name, "version": "1", "environment": environment, @@ -70,8 +89,7 @@ func wireCatalogBody(name, environment string) []byte { "transaction_uuid": "aff261a2-1a34-4647-8c20-ff662ec11c4c", "resources": []any{}, "edges": []any{}, - }) - return body + } } func TestAdapter_RequestCandidate_V3Success(t *testing.T) { @@ -107,6 +125,39 @@ func TestAdapter_RequestCandidate_V3Success(t *testing.T) { } } +// TestAdapter_RequestCandidate_V3RequiresAcceptHeader pins the one v3 +// request detail no other test in this file covers: the endpoint is +// served by the compiler's embedded Ruby Puppet request handler, which +// rejects a request carrying no Accept header ("Missing required Accept +// header", logged as a 400) before compiling anything. The handler below +// mirrors that behavior, so the test fails against a builder that omits +// the header rather than passing either way. See doc.go's v3 request +// Accept bullet. +func TestAdapter_RequestCandidate_V3RequiresAcceptHeader(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + var gotAccept string + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { + gotAccept = r.Header.Get("Accept") + if gotAccept == "" { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte("Bad Request: Missing required Accept header")) + return + } + w.WriteHeader(http.StatusOK) + w.Write(wireCatalogBody("web-01.example.test", "production")) + }) + adapter := newAdapter(t, fixture, srv) + + fs := factsetWithTrusted("web-01.example.test", "production", false) + _, _, _, diag := adapter.RequestCandidate(context.Background(), v3Target("web-01.example.test", "production"), fs) + if diag != nil { + t.Fatalf("RequestCandidate returned diagnostic: %+v", diag) + } + if gotAccept != "application/json" { + t.Errorf("Accept = %q, want application/json", gotAccept) + } +} + func TestAdapter_RequestCandidate_V4Success_ProvidedTrustedFacts(t *testing.T) { fixture := newTLSFixture(t, "127.0.0.1") var gotBody v4Request @@ -118,7 +169,7 @@ func TestAdapter_RequestCandidate_V4Success_ProvidedTrustedFacts(t *testing.T) { t.Fatalf("decoding request body: %v", err) } w.WriteHeader(http.StatusOK) - w.Write(wireCatalogBody("web-01.example.test", "production")) + w.Write(v4CatalogBody("web-01.example.test", "production")) }) adapter := newAdapter(t, fixture, srv) @@ -152,7 +203,7 @@ func TestAdapter_RequestCandidate_V4Success_CompilerLookupOmitsField(t *testing. t.Fatalf("decoding request body: %v", err) } w.WriteHeader(http.StatusOK) - w.Write(wireCatalogBody("web-01.example.test", "production")) + w.Write(v4CatalogBody("web-01.example.test", "production")) }) adapter := newAdapter(t, fixture, srv) @@ -318,7 +369,7 @@ func TestAdapter_RequestCandidate_IdentityMismatchNeverFallsBack(t *testing.T) { t.Error("v3 fallback request made after a v4 identity mismatch") } w.WriteHeader(http.StatusOK) - w.Write(wireCatalogBody("some-other-node.example.test", "production")) + w.Write(v4CatalogBody("some-other-node.example.test", "production")) }) adapter := newAdapter(t, fixture, srv) @@ -374,7 +425,7 @@ func TestAdapter_RequestCandidate_ProvenanceNeverCarriesTrustedFactValues(t *tes fixture := newTLSFixture(t, "127.0.0.1") srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - w.Write(wireCatalogBody("web-01.example.test", "production")) + w.Write(v4CatalogBody("web-01.example.test", "production")) }) adapter := newAdapter(t, fixture, srv) @@ -429,3 +480,105 @@ func TestAdapter_RequestCandidate_UnsupportedCatalogAPIFails(t *testing.T) { t.Fatal("expected a diagnostic for an unsupported catalog_api, got nil") } } + +// TestAdapter_RequestCandidate_V4RejectsUnwrappedCatalogBody pins the +// v4 response envelope. A bare catalog document (the v3 shape) decodes +// into wireCatalog with every field absent rather than failing, so +// reading a v4 response unwrapped produced a "malformed response" +// diagnostic against a compiler that had just logged a successful +// compilation. This asserts the adapter requires the documented +// `{"catalog": ...}` envelope on v4 instead of tolerating either shape. +func TestAdapter_RequestCandidate_V4RejectsUnwrappedCatalogBody(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/puppet/v3/catalog/web-01.example.test" { + t.Error("v3 fallback request made after an unwrapped v4 200 response") + } + w.WriteHeader(http.StatusOK) + w.Write(wireCatalogBody("web-01.example.test", "production")) + }) + adapter := newAdapter(t, fixture, srv) + + fs := factsetWithTrusted("web-01.example.test", "production", true) + _, _, _, diag := adapter.RequestCandidate(context.Background(), v4Target("web-01.example.test", "production", true, false), fs) + if diag == nil { + t.Fatal("expected a diagnostic for a v4 response with no catalog envelope, got nil") + } + if diag.Operation != model.OperationRequestCandidateTransport { + t.Errorf("Operation = %q, want %q", diag.Operation, model.OperationRequestCandidateTransport) + } +} + +// TestAdapter_RequestCandidate_V3RejectsWrappedCatalogBody is the mirror +// of the above: v3 has no envelope, so a v4-shaped body from the v3 +// endpoint must fail rather than be unwrapped opportunistically. +func TestAdapter_RequestCandidate_V3RejectsWrappedCatalogBody(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write(v4CatalogBody("web-01.example.test", "production")) + }) + adapter := newAdapter(t, fixture, srv) + + fs := factsetWithTrusted("web-01.example.test", "production", false) + _, _, _, diag := adapter.RequestCandidate(context.Background(), v3Target("web-01.example.test", "production"), fs) + if diag == nil { + t.Fatal("expected a diagnostic for a v3 response carrying a v4 envelope, got nil") + } +} + +// TestAdapter_RequestCandidate_V4FallbackReadsV3ShapeNotV4 covers the +// case the envelope selection is easiest to get wrong: on the permitted +// v4-to-v3 fallback the target is configured for v4, but the response in +// hand came from the v3 endpoint and carries no envelope. The unwrap +// must key on the API that served the response, not on the target's +// configured candidate.catalog_api. +func TestAdapter_RequestCandidate_V4FallbackReadsV3ShapeNotV4(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/puppet/v4/catalog": + w.WriteHeader(http.StatusNotFound) + case "/puppet/v3/catalog/web-01.example.test": + w.WriteHeader(http.StatusOK) + w.Write(wireCatalogBody("web-01.example.test", "production")) + default: + t.Errorf("unexpected path: %s", r.URL.Path) + } + }) + adapter := newAdapter(t, fixture, srv) + + fs := factsetWithTrusted("web-01.example.test", "production", true) + cat, prov, _, diag := adapter.RequestCandidate(context.Background(), v4Target("web-01.example.test", "production", true, false), fs) + if diag != nil { + t.Fatalf("RequestCandidate returned diagnostic: %+v", diag) + } + if cat.Certname != "web-01.example.test" { + t.Errorf("catalog = %+v", cat) + } + if !prov.FellBackFromV4 || prov.EffectiveAPI != config.CatalogAPIv3 { + t.Errorf("provenance = %+v, want a v3 fallback", prov) + } +} + +// TestAdapter_RequestCandidate_V4NullCatalogMember pins the null-member +// case: `{"catalog": null}` is a present member whose raw JSON is four +// non-empty bytes, so it must be rejected by the envelope check rather +// than slipping through to catalog decoding. +func TestAdapter_RequestCandidate_V4NullCatalogMember(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"catalog": null}`)) + }) + adapter := newAdapter(t, fixture, srv) + + fs := factsetWithTrusted("web-01.example.test", "production", true) + _, _, _, diag := adapter.RequestCandidate(context.Background(), v4Target("web-01.example.test", "production", false, false), fs) + if diag == nil { + t.Fatal(`expected a diagnostic for a v4 response with a null "catalog" member, got nil`) + } + if !strings.Contains(diag.Message, `no "catalog" member`) { + t.Errorf("Message = %q, want the envelope-specific message", diag.Message) + } +} diff --git a/internal/compiler/doc.go b/internal/compiler/doc.go index ff56117..9ce15ef 100644 --- a/internal/compiler/doc.go +++ b/internal/compiler/doc.go @@ -24,6 +24,11 @@ // environment verification, per design.md section 5's "Its contract // requires that the returned catalog identify the requested certname // and candidate environment exactly"; +// - v4 persistence suppression: every v4 request carries +// `persistence: {facts: false, catalog: false}`, unconditionally. +// This is the only client-side control that keeps a candidate +// compilation out of PuppetDB, and it is why requirements.md 1.6 +// holds for v4 and cannot hold for v3 (see "# Persistence" below); // - v4 target trusted-fact handling: sending a factset's own valid // trusted-fact structure, using the documented v4 omitted-field/ // compiler-lookup behavior only when the target has opted into that @@ -35,28 +40,24 @@ // permitted fallback), per design.md section 5's fourth paragraph and // requirements.md 2.5-2.6. // -// It does not normalize a catalog into model.NormalizedCatalog (task 7) -// and does not decide policy about OpenVox beyond what design.md section -// 5 states explicitly ("OpenVox is configured v3 only unless an operator -// explicitly selects an implementation with a documented v4 contract; -// PIACE does not claim v4 trusted-fact equivalence for OpenVox") — this -// package has no OpenVox-specific branch at all: it implements exactly -// the v3 and v4 request/response contracts documented for Puppet Server -// (which OpenVox's own documented v3 endpoint is wire-compatible with, -// per requirements.md section 7's "Shared catalog API" row), and an -// operator who selects catalog_api: v4 for an OpenVox compiler is relying -// on their own documented compatibility contract with that compiler, not -// on any OpenVox-specific behavior this package invents. -// -// # Documented assumptions: wire shapes (unverified against a live -// service; see tasks.md's Notes section) +// It does not normalize a catalog into model.NormalizedCatalog (task 7). +// +// This package has no implementation-specific branch: Puppet Server and +// OpenVox serve the same v3 and v4 catalog contracts, both authorize a +// catalog-reader certificate through auth.conf, and both honour the v4 +// request's persistence field (verified against a deployed OpenVox +// compiler on 2026-08-25; see requirements.md section 7). The configured +// catalog_api, not the compiler product, decides what this package can +// guarantee. +// +// # Wire shapes // // Per tasks.md's Notes ("Protocol adapters remain the compatibility // boundary. Their exact requests and responses must be demonstrated with // fixtures from the deployed service versions before declaring a // compiler/PuppetDB combination supported"), this package is built from -// the publicly documented v3/v4 catalog HTTP APIs, not from a fixture -// capture against a live Puppet Server/OpenVox instance: +// the v3/v4 catalog HTTP APIs as documented and as implemented in the +// compilers' own source: // // - v3 request: POST /puppet/v3/catalog/, form-encoded body with // `environment`, `facts_format=application/json`, a JSON-encoded @@ -65,20 +66,81 @@ // catalog API (api/docs/http_catalog.md in openvoxproject/openvox), // which Puppet Server's v3 endpoint is wire-compatible with per // requirements.md section 7. -// - v3/v4 response: both endpoints return the same catalog document -// shape directly (not wrapped): `{"name": , "environment": -// ..., "code_id": ..., "catalog_uuid": ..., "resources": [...], -// "edges": [...], ...}`. Critically, this uses `name`, not -// `certname` — unlike internal/puppetdb's query-API responses. This -// package's wireCatalog type reflects that; RequestCandidate maps +// - v3 request Accept header: `Accept: application/json`, and it is +// mandatory, not a nicety. Unlike v4 (a pure Clojure route in +// master_core.clj), the v3 catalog endpoint dispatches into the +// compiler's embedded Ruby Puppet request handler, whose +// Puppet::Network::HTTP::Request#response_formatters_for raises +// "Missing required Accept header" when the header is absent — the +// request is rejected before any compilation happens. Verified +// against a deployed OpenVox server (2026-08-25): the same POST, +// with real PuppetDB-sourced facts, returns +// `{"message":"Bad Request: Missing required Accept header", +// "issue_kind":"MISSING_HEADER_FIELD"}` with HTTP 400 when the +// header is omitted and HTTP 200 with a complete catalog when it is +// `application/json`. The same request against /puppet/v4/catalog +// succeeds with no Accept header at all, confirming the asymmetry. +// buildV3Request therefore sets the header and buildV4Request +// deliberately does not. +// - v3 rich-data encoding is not selected by the Accept header, at +// least on the compiler this was measured against. A Puppet agent +// requests `application/vnd.puppet.rich+json, application/json, +// text/pson`, which raises a fair question for PIACE: a +// PuppetDB-sourced baseline was stored from a real agent's +// submission, so a candidate fetched with a *less* capable Accept +// could differ from it in encoding alone (rich types — Sensitive, +// Timestamp, Binary, Regexp, Deferred — degrading to plain strings) +// and produce diffs that are pure artifacts. Measured on the same +// deployed OpenVox server, it does not: the two responses differ +// only in `Content-Type` (application/json vs +// application/vnd.puppet.rich+json) and in the per-compilation +// `catalog_uuid`/`version`; the catalog documents are structurally +// identical, and `__ptype`-tagged rich values (a Regexp parameter) +// appear in *both*. The isolating case was run too — +// `Accept: application/vnd.puppet.rich+json` alone, with no +// application/json fallback for the server to select instead — +// and returns the same structurally identical document, so the +// result is not an artifact of the agent list's json fallback +// matching first. Rich encoding is a server-side property +// (Puppet's `rich_data` setting), not something the client's Accept +// header negotiates. Caveat on the sample: the catalog used carried +// rich values of one type only (Regexp), so this is evidence that +// the converter's rich flag is on regardless of requested format, +// not a per-type enumeration. Requesting bare `application/json` +// keeps this package's Accept header minimal and honest about what +// response.go actually decodes; if a future deployment is found +// where the header does select the encoding, this constant — not +// normalization — is the place to change it. +// - catalog document: `{"name": , "environment": ..., "code_id": +// ..., "catalog_uuid": ..., "resources": [...], "edges": [...], +// ...}`. Critically, this uses `name`, not `certname` — unlike +// internal/puppetdb's query-API responses. This package's +// wireCatalog type reflects that; RequestCandidate maps // wireCatalog.Name into the returned puppetdb.Catalog's Certname // field so the rest of the codebase (which already keys everything // on Certname) does not need a second identity field name. Version // is accepted as either a JSON string or number (the OpenVox example // response shows a bare integer; PuppetDB's own query-API catalog // responses show a string) via wireCatalog's custom decoding. -// - v3/v4 `resources`/`edges` are plain JSON arrays in the compiler's -// response, not the `{href, data}` expansion internal/puppetdb's +// - v3 response envelope: none. `POST /puppet/v3/catalog/` +// returns the catalog document as the entire response body. Source: +// the example response in OpenVox's api/docs/http_catalog.md (and +// puppetlabs/puppet's identical copy of that file). +// - v4 response envelope: `{"catalog": }` — the v4 endpoint +// wraps it, v3 does not. Source: puppetserver's own implementation, +// src/ruby/puppetserver-lib/puppet/server/compiler.rb, whose +// `compile` returns `{ catalog: catalog }` (or `{ catalog:, logs: }` +// when options.capture_logs is set, which PIACE never sets), and +// src/clj/puppetlabs/services/master/master_core.clj, whose +// v4-catalog-fn JSON-encodes that hash verbatim as the 200 response +// body. catalogDocument (response.go) unwraps it, keyed on the API +// version of the request that produced the response — never sniffed +// from the body. This difference is silent if unhandled: a v4 body +// decodes cleanly into wireCatalog with every field absent, so an +// unwrapped read reports "malformed response" for a compilation the +// compiler's own log records as successful. +// - `resources`/`edges` are plain JSON arrays in the compiler's +// catalog document, not the `{href, data}` expansion internal/puppetdb's // doc.go documents for a PuppetDB *query-API* catalog response. This // package passes them through as-is (puppetdb.Catalog.Resources/ // Edges are already typed json.RawMessage precisely so a later stage @@ -88,8 +150,8 @@ // PuppetDB-sourced baseline and a compiler-sourced candidate. // - v4 request body: `{"certname", "persistence": {"facts": false, // "catalog": false}, "environment", "facts": {"values": {...}}, -// "trusted_facts": {"values": {...}}}`, per Puppet Server's -// documented v4 catalog API. `persistence` is always `{false, +// "trusted_facts": {"values": {...}}}`, matching Puppet Server's +// CatalogRequestV4 schema in master_core.clj. `persistence` is always `{false, // false}` in every request this package builds — requirements.md // 1.6 ("SHALL not persist candidate facts or candidate catalogs to // PuppetDB") makes this non-negotiable, not a configurable option. @@ -103,6 +165,34 @@ // two fields that distinguish Puppet's documented trusted-fact shape // from an unrelated fact that happens to be named "trusted". // +// # Persistence +// +// A v4 request suppresses persistence and a v3 request cannot, and the +// difference is contractual rather than cosmetic. Verified against a +// deployed OpenVox compiler on 2026-08-25 with a certname PuppetDB had +// never seen: +// +// - `POST /puppet/v4/catalog` with `persistence: {facts: false, +// catalog: false}` returned the catalog and left PuppetDB with no +// factset, no catalog, and no node for that certname; +// - `POST /puppet/v3/catalog/` returned the catalog and left +// PuppetDB holding a factset and a catalog for it under the requested +// environment, the stored catalog carrying the request's own +// transaction_uuid, and a node whose facts_environment and +// catalog_environment were both the candidate environment. +// +// The v3 endpoint has no persistence parameter to set: the compiler saves +// the facts submitted with the request, and stores the compiled catalog +// through its PuppetDB catalog cache terminus. For a real target this +// overwrites the target's stored factset and catalog — which is exactly +// the PuppetDB baseline a comparison reads, so a v3 candidate compilation +// destroys its own run's baseline for every subsequent target. That is +// why requirements.md 1.8 constrains a v3 target to baseline.source: +// file, and why the v3 warning covers persistence as well as $trusted. +// This package cannot prevent either effect; it sends the v4 persistence +// fields where they exist and reports the v3 consequences where they do +// not. +// // # Verified-unsupported-v4 detection // // design.md section 3.1 permits a v4-to-v3 fallback "only for a diff --git a/internal/compiler/property_test.go b/internal/compiler/property_test.go index 561dae0..310429a 100644 --- a/internal/compiler/property_test.go +++ b/internal/compiler/property_test.go @@ -66,15 +66,27 @@ func TestProperty_CandidateIdentityIntegrity(t *testing.T) { wantAccept := returnedCertname == requestedCertname && returnedEnv == requestedEnv + // Alternate the candidate API across iterations: identity + // integrity is a property of both endpoints, and the two do not + // share a response envelope (v4 wraps the catalog document, v3 + // does not — see doc.go), so exercising only one would leave the + // other's identity check unproven. + useV4 := i%2 == 0 + body := wireCatalogBody(returnedCertname, returnedEnv) + target := v3Target(requestedCertname, requestedEnv) + if useV4 { + body = v4CatalogBody(returnedCertname, returnedEnv) + target = v4Target(requestedCertname, requestedEnv, false, false) + } + fixture := newTLSFixture(t, "127.0.0.1") srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) - w.Write(wireCatalogBody(returnedCertname, returnedEnv)) + w.Write(body) }) adapter := newAdapter(t, fixture, srv) - fs := factsetWithTrusted(requestedCertname, requestedEnv, false) - target := v3Target(requestedCertname, requestedEnv) + fs := factsetWithTrusted(requestedCertname, requestedEnv, useV4) cat, _, _, diag := adapter.RequestCandidate(context.Background(), target, fs) diff --git a/internal/compiler/request.go b/internal/compiler/request.go index 42824aa..d0ac913 100644 --- a/internal/compiler/request.go +++ b/internal/compiler/request.go @@ -59,6 +59,16 @@ func decideTrustedFacts(flatFacts map[string]json.RawMessage, compilerLookupConf return trustedFactsDecision{available: false} } +// v3CatalogAcceptHeader is the Accept header sent with every v3 catalog +// request. `application/json` is one of the catalog indirection's +// supported formats and is the only response encoding this package's +// response decoding handles; text/pson is deliberately not offered. The +// agent's richer `application/vnd.puppet.rich+json` list is deliberately +// not requested either: measured against a deployed compiler, it selects +// only the response Content-Type, not the catalog's rich-data encoding. +// See doc.go's v3 rich-data bullet. +const v3CatalogAcceptHeader = "application/json" + // buildV4Request constructs the POST /puppet/v4/catalog request. See // doc.go for the documented v4 request-shape assumption. func buildV4Request(ctx context.Context, client *transport.Client, baseURL *url.URL, certname, environment string, flatFacts map[string]json.RawMessage, decision trustedFactsDecision) (*http.Request, error) { @@ -98,7 +108,9 @@ func buildV4Request(ctx context.Context, client *transport.Client, baseURL *url. // buildV3Request constructs the POST /puppet/v3/catalog/:certname // request. See doc.go for the documented v3 request-shape assumption: // form-encoded body with environment, facts_format, facts (a JSON string -// of {"name", "values"}), and transaction_uuid. +// of {"name", "values"}), and transaction_uuid, plus an explicit Accept +// header, which the v3 endpoint requires (see doc.go's v3 request +// bullet: the request is rejected outright without one). func buildV3Request(ctx context.Context, client *transport.Client, baseURL *url.URL, certname, environment string, flatFacts map[string]json.RawMessage) (*http.Request, error) { factsJSON, err := json.Marshal(v3Facts{Name: certname, Values: flatFacts}) if err != nil { @@ -125,5 +137,11 @@ func buildV3Request(ctx context.Context, client *transport.Client, baseURL *url. return nil, err } req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + // Non-optional for v3: the endpoint is served by the compiler's + // embedded Ruby Puppet request handler, which rejects a catalog + // request carrying no Accept header before it ever compiles + // ("Missing required Accept header"). The v4 endpoint has no such + // requirement, which is why only this builder sets it. + req.Header.Set("Accept", v3CatalogAcceptHeader) return req, nil } diff --git a/internal/compiler/response.go b/internal/compiler/response.go index a790ca7..b025ab8 100644 --- a/internal/compiler/response.go +++ b/internal/compiler/response.go @@ -1,10 +1,12 @@ package compiler import ( + "bytes" "encoding/json" "errors" "net/http" + "github.com/example42/piace/internal/config" "github.com/example42/piace/internal/model" "github.com/example42/piace/internal/puppetdb" "github.com/example42/piace/internal/transport" @@ -21,10 +23,11 @@ import ( // // - 404 is the literal, unambiguous signal that `POST // /puppet/v4/catalog` is not a registered route at all — the case a -// Puppet Server build older than 6.3.0 (which introduced the v4 -// endpoint; see doc.go) or an OpenVox compiler (which documents only -// v3; see doc.go) would produce, since neither server has any route -// bound to that path. This is exactly "unsupported-endpoint." +// Puppet Server or OpenVox build predating the v4 catalog endpoint +// would produce, since such a server has no route bound to that +// path. This is exactly "unsupported-endpoint." Current builds of +// both serve v4, so this signal means "too old", not "wrong +// product". // - 501 is the standard HTTP status a server uses to say "the server // does not support the functionality required to fulfill the // request" — the natural status for a server that recognizes the @@ -73,7 +76,7 @@ func isVerifiedUnsupportedV4(statusCode int) bool { // (model.OperationLoadFacts/OperationLoadBaseline, both operational), // rather than forcing every response-shape problem into the same // compilation-failure bucket as a verified rejection. -func processResponse(resp *transport.Response, host, certname, environment string) (puppetdb.Catalog, *model.Diagnostic) { +func processResponse(resp *transport.Response, host, certname, environment string, effectiveAPI config.CatalogAPI) (puppetdb.Catalog, *model.Diagnostic) { if resp.StatusCode < 200 || resp.StatusCode >= 300 { diag := compilationFailureDiagnostic(certname, host, resp.StatusCode, "compiler returned a non-2xx status for the candidate catalog request") @@ -96,8 +99,13 @@ func processResponse(resp *transport.Response, host, certname, environment strin return puppetdb.Catalog{}, &diag } + document, diag := catalogDocument(resp, host, certname, effectiveAPI) + if diag != nil { + return puppetdb.Catalog{}, diag + } + var wc wireCatalog - if err := json.Unmarshal(resp.Body, &wc); err != nil || wc.Name == "" { + if err := json.Unmarshal(document, &wc); err != nil || wc.Name == "" { diag := operationalResponseDiagnostic(certname, host, resp.StatusCode, "malformed or unparseable compiler catalog response") return puppetdb.Catalog{}, &diag @@ -126,6 +134,48 @@ func processResponse(resp *transport.Response, host, certname, environment strin }, nil } +// catalogDocument extracts the catalog document from a 2xx compiler +// response according to the API version that actually served it. +// +// The two endpoints do not share a response envelope, and the difference +// is silent rather than loud: a v4 body decodes cleanly into wireCatalog +// with every field absent, which is why an unhandled v4 envelope +// surfaces as "malformed or unparseable response" against a compiler +// whose own log records a successful compilation. See doc.go's wire-shape +// section for the primary sources. +// +// - v3 returns the catalog document as the whole response body. +// - v4 returns `{"catalog": }`. +// +// The version is taken from the caller rather than sniffed from the +// body. A "top-level `name`, else look under `catalog`" heuristic would +// accept either shape from either endpoint, which is exactly the +// speculative-probing behavior design.md section 5 rules out — and it +// would also mask a compiler that started returning the wrong envelope. +// +// effectiveAPI is the API of the request that produced this very +// response, never target.Candidate.CatalogAPI: on the permitted +// v4-to-v3 fallback path (design.md section 3.1) the target is +// configured for v4 while the response in hand came from v3. +func catalogDocument(resp *transport.Response, host, certname string, effectiveAPI config.CatalogAPI) (json.RawMessage, *model.Diagnostic) { + if effectiveAPI != config.CatalogAPIv4 { + return resp.Body, nil + } + // A present-but-null "catalog" member is treated the same as a + // missing one: json.RawMessage("null") is four non-empty bytes, so + // without this it would slip through to wireCatalog decoding and + // report the generic malformed-response message instead of the + // specific one naming the envelope. + var envelope v4CatalogEnvelope + if err := json.Unmarshal(resp.Body, &envelope); err != nil || + len(envelope.Catalog) == 0 || bytes.Equal(envelope.Catalog, []byte("null")) { + diag := operationalResponseDiagnostic(certname, host, resp.StatusCode, + `malformed or unparseable compiler catalog response: v4 response has no "catalog" member`) + return nil, &diag + } + return envelope.Catalog, nil +} + // compilationFailureDiagnostic builds a model.Diagnostic classified as // design.md section 10's "compilation failure" (model.OperationRequestCandidate). func compilationFailureDiagnostic(certname, host string, statusCode int, message string) model.Diagnostic { diff --git a/internal/compiler/wire.go b/internal/compiler/wire.go index 7692d86..97e90f6 100644 --- a/internal/compiler/wire.go +++ b/internal/compiler/wire.go @@ -34,11 +34,28 @@ func (v *wireVersion) UnmarshalJSON(data []byte) error { return nil } -// wireCatalog is the compiler's direct (unwrapped) v3/v4 catalog response -// shape, per doc.go's documented assumption: `{"name", "environment", -// "code_id", "catalog_uuid", "transaction_uuid", "resources", "edges", -// ...}`. Unlike internal/puppetdb's query-API Catalog carrier, the -// identity field here is `name`, not `certname`, and resources/edges are +// v4CatalogEnvelope is the POST /puppet/v4/catalog response wrapper. +// Unlike v3, which returns the catalog document directly, the v4 +// endpoint returns it under a "catalog" member (alongside an optional +// "logs" member when the request asked for captured logs, which PIACE +// never does). See doc.go's wire-shape section for the primary sources: +// puppetserver's compiler.rb returns `{ catalog: ... }` and its +// master_core.clj v4 handler JSON-encodes that hash verbatim as the +// response body. +// +// Logs is intentionally not declared: PIACE never sets +// options.capture_logs, and a compiler that returned logs anyway would +// have them dropped rather than mistaken for catalog content. +type v4CatalogEnvelope struct { + Catalog json.RawMessage `json:"catalog"` +} + +// wireCatalog is the compiler's catalog document shape: `{"name", +// "environment", "code_id", "catalog_uuid", "transaction_uuid", +// "resources", "edges", ...}`. It is the whole v3 response body, and the +// value of a v4 response's "catalog" member (see v4CatalogEnvelope). +// Unlike internal/puppetdb's query-API Catalog carrier, the identity +// field here is `name`, not `certname`, and resources/edges are // plain JSON arrays, not a `{href, data}` expansion. Fields this package // does not consume (tags, classes, catalog_format, metadata, // recursive_metadata) are intentionally not declared and are dropped by @@ -174,7 +191,7 @@ type v4Request struct { // v3Facts is the JSON value the v3 catalog request's form-encoded `facts` // parameter carries: `{"name": , "values": {...}}`, per doc.go's -// documented assumption from OpenVox's v3 catalog API. +// v3 request bullet. type v3Facts struct { Name string `json:"name"` Values map[string]json.RawMessage `json:"values"` diff --git a/internal/filecontent/doc.go b/internal/filecontent/doc.go index b810c1e..bdb032a 100644 --- a/internal/filecontent/doc.go +++ b/internal/filecontent/doc.go @@ -174,10 +174,15 @@ // Per tasks.md's Notes section ("Protocol adapters remain the // compatibility boundary. Their exact requests and responses must be // demonstrated with fixtures from the deployed service versions before -// declaring a compiler/PuppetDB combination supported"), the exact -// endpoint shape below is a documented assumption, not yet verified -// against a live Puppet Server/OpenVox instance — the same caveat tasks -// 4 and 6 already carry for their own documented wire-shape assumptions: +// declaring a compiler/PuppetDB combination supported"), what follows +// separates the two. Verified against a deployed OpenVox compiler on +// 2026-08-25: the request path and query shape, the 200 response with +// Content-Type application/octet-stream and raw bytes, and the whole +// Accept contract (400 without the header, 200 with +// application/octet-stream, 406 with application/json), exercised over +// one `puppet:///modules//` reference. Still documented- +// only, and marked as such below: the 404 response body for a missing +// file, and the treatment of non-`puppet:` source schemes. // // - GET /puppet/v3/file_content//?environment= // returns the raw bytes of the referenced file with Content-Type @@ -185,10 +190,29 @@ // file_content endpoint (puppetlabs/puppet, api/docs/http_file_content.md): // "The file_content endpoint returns the contents of the specified // file." A 404 response ("Not Found: Could not find file_content -// ") is documented for a missing file; this package treats any -// non-2xx response as a retrieval failure (step 4b), never +// ") is documented for a missing file but was not exercised +// against a live compiler; nothing depends on the body text, because +// this package treats any non-2xx response as a retrieval failure +// (step 4b), never // inspecting the response body for meaning, matching this package's // "never render bytes, never trust echoed content" posture. +// - The request carries `Accept: application/octet-stream`, and the +// header is mandatory. Every /puppet/v3/ route is served by the +// compiler's embedded Ruby Puppet request handler, whose +// Puppet::Network::HTTP::Request#response_formatters_for raises +// "Missing required Accept header" when no Accept header is present +// — the request is rejected before any file is served. Verified +// against a deployed OpenVox server (2026-08-25) on this exact +// endpoint: no Accept header returns HTTP 400 +// "Bad Request: Missing required Accept header", and +// `Accept: application/octet-stream` returns HTTP 200 with the +// file's raw bytes. The value is endpoint-specific and cannot be +// shared with the catalog endpoint's: the file-content indirection +// serves only the binary format, and the same verified request with +// `Accept: application/json` returns HTTP 406 +// "Not Acceptable: No supported formats are acceptable". Reusing the +// catalog's Accept value here would trade one rejected request for +// another. // - A Puppet File resource's `source` value in the form // `puppet:////` (the documented form for the // `modules/` and other file-serving mount points; see @@ -197,9 +221,10 @@ // with its leading slash trimmed, is exactly the endpoint's // `/` path segment — see parsePuppetSourceURI in // resolver.go. -// - A `source` value using any other URI scheme (a bare local -// filesystem path, a `file:` URI, or an `http(s):` URI) is not -// retrievable through this endpoint at all — Puppet's own File type +// - Documented-only, not exercised: a `source` value using any other +// URI scheme (a bare local filesystem path, a `file:` URI, or an +// `http(s):` URI) is not retrievable through this endpoint at all — +// Puppet's own File type // documentation describes those as resolved directly by the agent, // not proxied through the compiler's file-serving API. This package // reports that case as a retrieval failure (step 4b: diff --git a/internal/filecontent/resolver.go b/internal/filecontent/resolver.go index 97525ba..9661042 100644 --- a/internal/filecontent/resolver.go +++ b/internal/filecontent/resolver.go @@ -33,6 +33,13 @@ func NewCompilerContentResolver(client *transport.Client, endpoint *url.URL) *Co return &CompilerContentResolver{client: client, baseURL: endpoint} } +// fileContentAcceptHeader is the Accept header sent with every +// file_content request. The file-content indirection serves only the +// binary format, so application/octet-stream is the single acceptable +// value -- offering application/json here would trade a rejected +// missing-Accept request for a rejected unacceptable-format one. +const fileContentAcceptHeader = "application/octet-stream" + // Digest implements ContentRetriever. It resolves reference (a Puppet // File `source` value) into a DigestEvidence by retrieving the // referenced bytes through the compiler's documented v3 file_content @@ -61,6 +68,12 @@ func (r *CompilerContentResolver) Digest(ctx context.Context, reference string, if err != nil { return DigestEvidence{}, fmt.Errorf("filecontent: building content retrieval request: %w", err) } + // Non-optional: the v3 file_content endpoint is served by the + // compiler's embedded Ruby Puppet request handler, which rejects a + // request carrying no Accept header ("Missing required Accept + // header") before serving anything. application/octet-stream is the + // only content type this endpoint serves (see doc.go). + req.Header.Set("Accept", fileContentAcceptHeader) resp, err := r.client.Do(req, 0) if err != nil { diff --git a/internal/filecontent/resolver_test.go b/internal/filecontent/resolver_test.go index 3b67f19..153642f 100644 --- a/internal/filecontent/resolver_test.go +++ b/internal/filecontent/resolver_test.go @@ -37,10 +37,19 @@ func TestParsePuppetSourceURI(t *testing.T) { func TestCompilerContentResolver_Digest_Success(t *testing.T) { const raw = "the quick brown fox" fixture := newTLSFixture(t, "127.0.0.1") - var gotPath, gotQuery string + var gotPath, gotQuery, gotAccept string srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { gotPath = r.URL.Path gotQuery = r.URL.RawQuery + gotAccept = r.Header.Get("Accept") + // Mirrors the compiler's embedded Ruby Puppet request handler, + // which rejects any /puppet/v3/ request with no Accept header + // before serving anything (see doc.go). + if gotAccept == "" { + w.WriteHeader(http.StatusBadRequest) + w.Write([]byte("Bad Request: Missing required Accept header")) + return + } w.WriteHeader(http.StatusOK) w.Write([]byte(raw)) }) @@ -64,6 +73,9 @@ func TestCompilerContentResolver_Digest_Success(t *testing.T) { if !strings.Contains(gotQuery, "environment=production") { t.Errorf("request query = %q, want environment=production", gotQuery) } + if gotAccept != "application/octet-stream" { + t.Errorf("Accept = %q, want application/octet-stream", gotAccept) + } if strings.Contains(digest.Digest, raw) { t.Errorf("digest leaks raw content: %q", digest.Digest) } diff --git a/internal/normalize/catalog_test.go b/internal/normalize/catalog_test.go index 5d037dc..1121d95 100644 --- a/internal/normalize/catalog_test.go +++ b/internal/normalize/catalog_test.go @@ -360,3 +360,105 @@ func TestCatalog_LargeIntegerPreservesAllDigits(t *testing.T) { t.Errorf("Parameters[serial] = %#v, want model.Number(%q)", got.Resources[0].Parameters["serial"], bigDigits) } } + +// TestCatalog_CompilerShape_StringResourceReferenceEdges covers the edge +// vertex form a real compiler actually returns: a `Type[title]` reference +// string, not a `{type, title}` object. Puppet::Relationship#to_data_hash +// serializes each vertex as `source.to_s`/`target.to_s`, so this is what +// every v3/v4 catalog response and every `capture catalog` snapshot +// carries — the object form only appears in a terminus-submitted wire +// format v8 catalog. +func TestCatalog_CompilerShape_StringResourceReferenceEdges(t *testing.T) { + raw := compilerShapedCatalog("web-01.example.test", "production", + `[ + {"type":"File","title":"/etc/motd","parameters":{"ensure":"file"}}, + {"type":"Notify","title":"hello","parameters":{"message":"hi"}} + ]`, + `[ + {"source":"Notify[hello]","target":"File[/etc/motd]"}, + {"source":"Class[Main]","target":"Notify[hello]"} + ]`, + ) + + got, diag := Catalog(raw) + if diag != nil { + t.Fatalf("unexpected diagnostic: %+v", diag) + } + want := []model.Edge{ + {Source: "Class[Main]", Target: "Notify[hello]"}, + {Source: "Notify[hello]", Target: "File[/etc/motd]"}, + } + if len(got.Edges) != len(want) { + t.Fatalf("Edges = %+v, want %+v", got.Edges, want) + } + for i := range want { + if got.Edges[i] != want[i] { + t.Errorf("Edges[%d] = %+v, want %+v", i, got.Edges[i], want[i]) + } + } +} + +// TestCatalog_ResourceReferenceCompositeTitle pins the split semantics +// ported from the PuppetDB terminus's resource_ref_to_hash regex: the +// type stops at the first bracket and the title runs to the last one, so +// a title that itself contains brackets survives intact. A naive split on +// the first "]" would truncate it, and the resulting identity would not +// match the same resource's identity on the PuppetDB side of the +// comparison. +func TestCatalog_ResourceReferenceCompositeTitle(t *testing.T) { + raw := compilerShapedCatalog("web-01.example.test", "production", + `[]`, + `[{"source":"Class[Main]","target":"File[/etc/foo[bar]]"}]`, + ) + + got, diag := Catalog(raw) + if diag != nil { + t.Fatalf("unexpected diagnostic: %+v", diag) + } + want := model.Edge{Source: "Class[Main]", Target: "File[/etc/foo[bar]]"} + if len(got.Edges) != 1 || got.Edges[0] != want { + t.Fatalf("Edges = %+v, want [%+v]", got.Edges, want) + } +} + +// TestCatalog_MixedEdgeVertexForms covers one edge carrying one vertex of +// each form. The terminus converts per vertex (`%w[source target].each`), +// so a half-converted edge is representable and must not be rejected. +func TestCatalog_MixedEdgeVertexForms(t *testing.T) { + raw := compilerShapedCatalog("web-01.example.test", "production", + `[]`, + `[{"source":"Class[Main]","target":{"type":"Notify","title":"hello"}}]`, + ) + + got, diag := Catalog(raw) + if diag != nil { + t.Fatalf("unexpected diagnostic: %+v", diag) + } + want := model.Edge{Source: "Class[Main]", Target: "Notify[hello]"} + if len(got.Edges) != 1 || got.Edges[0] != want { + t.Fatalf("Edges = %+v, want [%+v]", got.Edges, want) + } +} + +// TestCatalog_RejectsUnparseableResourceReference asserts a reference +// string that does not match Type[title] is a reported normalization +// failure. The Ruby original silently yields {nil, nil} there; this +// package's contract forbids a silently empty result. +func TestCatalog_RejectsUnparseableResourceReference(t *testing.T) { + for _, ref := range []string{"Notify hello", "Notify[]", "[hello]", ""} { + body, err := json.Marshal(map[string]any{"source": ref, "target": "Notify[hello]"}) + if err != nil { + t.Fatalf("Marshal: %v", err) + } + raw := compilerShapedCatalog("web-01.example.test", "production", `[]`, "["+string(body)+"]") + + _, diag := Catalog(raw) + if diag == nil { + t.Errorf("ref %q: expected a diagnostic, got none", ref) + continue + } + if diag.Operation != model.OperationNormalize { + t.Errorf("ref %q: Operation = %q, want %q", ref, diag.Operation, model.OperationNormalize) + } + } +} diff --git a/internal/normalize/doc.go b/internal/normalize/doc.go index 6eddeb1..dabf7bc 100644 --- a/internal/normalize/doc.go +++ b/internal/normalize/doc.go @@ -38,15 +38,31 @@ // [{"relationship", "source_title", "source_type", "target_title", // "target_type"}, ...]}` (PuppetDB catalogs endpoint documentation, // https://puppet.com/docs/puppetdb/8/catalogs.html). +// // - The compiler's v3/v4 catalog response (candidate catalogs, and any // snapshot captured via `capture catalog`) uses the plain-array // catalog interchange format: `resources: [{"type", "title", // "aliases", "exported", "file", "line", "tags", "parameters"}, ...]` -// and `edges: [{"source": {"type", "title"}, "target": {"type", -// "title"}, "relationship"}, ...]` (PuppetDB's documented catalog -// wire format v8, which internal/compiler/doc.go's own documented -// assumption states the compiler's direct response matches: -// https://puppet.com/docs/puppetdb/8/catalog_format_v8.html). +// and `edges: [{"source", "target", "relationship"}, ...]`. +// +// An edge vertex takes either of two forms there, and both are +// accepted (see resourceSpecWire in wire.go for the full rationale +// and the primary sources): +// +// A compiler's own response carries each vertex as a `Type[title]` +// *reference string* — Puppet::Relationship#to_data_hash serializes +// `source.to_s`/`target.to_s`, and Puppet::Resource#to_s is its ref. +// PIACE splits it with a Go port of the PuppetDB terminus's own +// resource_ref_to_hash regex, which is the same function that +// produced the source_type/source_title of the PuppetDB baseline +// being compared against — so the two sides line up by construction. +// +// PuppetDB's documented catalog wire format v8 defines the vertex as +// a `` *object*, `{"type", "title"}` +// (https://puppet.com/docs/puppetdb/8/catalog_format_v8.html); that +// is what the terminus submits, and the terminus itself converts +// reference strings into it (munge_edges). A plain-array catalog can +// therefore legitimately carry either form. // // Catalog auto-detects which shape it was given (an object vs. an array // at the top level of each field) rather than requiring the caller to say diff --git a/internal/normalize/wire.go b/internal/normalize/wire.go index 503aaab..ec827aa 100644 --- a/internal/normalize/wire.go +++ b/internal/normalize/wire.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "regexp" ) // resourceWire is the subset of a catalog resource entry this package @@ -47,19 +48,99 @@ type pdbEdgeEntry struct { TargetTitle string `json:"target_title"` } -// resourceSpecWire is a `` per PuppetDB's documented -// catalog wire format v8: `{"type": , "title": }`. +// resourceReferencePattern is a Go port of the PuppetDB terminus's own +// resource_ref_to_hash regex, `/^([^\[\]]+)\[(.+)\]$/m` (puppetlabs/ +// puppetdb, puppet/lib/puppet/indirector/catalog/puppetdb.rb). The +// semantics are load-bearing and are preserved exactly: +// +// - `[^\[\]]+` — the type stops at the FIRST bracket of either kind; +// - `(.+)` is greedy against a `$`-anchored `\]`, so the title runs to +// the LAST `]`, which is what makes a composite title like +// `File[/etc/foo[bar]]` split into `File` / `/etc/foo[bar]`; +// - Ruby's `/m` (dot matches newline) is Go's `(?s)`, not `(?m)`. +// +// Porting it rather than approximating it with a Split is what makes the +// two sides of a comparison line up by construction: a PuppetDB baseline +// edge's source_type/source_title were produced by this exact function, +// applied to this exact reference string, when the terminus submitted +// the catalog. +var resourceReferencePattern = regexp.MustCompile(`(?s)^([^\[\]]+)\[(.+)\]$`) + +// resourceSpecWire is one vertex of a plain-array catalog edge. It +// accepts both forms that legitimately occur there: +// +// - a JSON object, `{"type": , "title": }` — PuppetDB's +// documented catalog wire format v8 `` +// (https://puppet.com/docs/puppetdb/8/catalog_format_v8.html), which +// is what the terminus submits; +// - a JSON string in `Type[title]` reference form — what a compiler's +// own v3/v4 catalog response carries, because +// Puppet::Relationship#to_data_hash serializes each vertex as +// `source.to_s` / `target.to_s`, and Puppet::Resource#to_s is its +// `Type[title]` ref (openvoxproject/openvox, lib/puppet/relationship.rb). +// +// Accepting both is not the "sniff the shape" behavior +// internal/compiler/doc.go rules out for the v4 response envelope. There, +// one endpoint has exactly one envelope and the version is known at the +// call site. Here, a single documented container — the plain array — +// genuinely carries either vertex form, and the PuppetDB terminus itself +// branches on precisely this (`edge[vertex] = resource_ref_to_hash(...) +// if edge[vertex].is_a?(String)` in munge_edges). This type mirrors the +// terminus rather than inventing a rule. type resourceSpecWire struct { Type string `json:"type"` Title string `json:"title"` } -// compilerEdgeEntry is one element of a compiler plain-array edges list, -// per PuppetDB's documented catalog wire format v8's `` shape: -// `{"source": , "target": , "relationship": -// }` (https://puppet.com/docs/puppetdb/8/catalog_format_v8.html). -// Relationship is intentionally undeclared for the same reason noted on -// pdbEdgeEntry. +// resourceSpecObject is resourceSpecWire's object form, declared +// separately so UnmarshalJSON can decode into it without recursing into +// itself. +type resourceSpecObject struct { + Type string `json:"type"` + Title string `json:"title"` +} + +// UnmarshalJSON decodes either vertex form. A reference string that does +// not parse, or a form missing its type or title, is an error rather +// than a silently zero-valued vertex: the Ruby original yields +// `{nil, nil}` on a non-matching ref, which this package's contract +// forbids ("Unknown or malformed catalog/fact data is an operational +// normalization failure, never an empty catalog"). The offending +// reference is named in the error because a resource identity is not +// secret in this model — every report prints identities like +// `Service[nginx]` — and it is the one detail that makes the failure +// actionable. +func (s *resourceSpecWire) UnmarshalJSON(data []byte) error { + trimmed := bytes.TrimSpace(data) + if len(trimmed) > 0 && trimmed[0] == '"' { + var ref string + if err := json.Unmarshal(trimmed, &ref); err != nil { + return fmt.Errorf("decoding resource reference: %w", err) + } + match := resourceReferencePattern.FindStringSubmatch(ref) + if match == nil { + return fmt.Errorf("resource reference %q is not in Type[title] form", ref) + } + s.Type, s.Title = match[1], match[2] + return nil + } + + var obj resourceSpecObject + if err := json.Unmarshal(trimmed, &obj); err != nil { + return fmt.Errorf("decoding resource spec: %w", err) + } + if obj.Type == "" || obj.Title == "" { + return fmt.Errorf(`resource spec is missing its "type" or "title"`) + } + s.Type, s.Title = obj.Type, obj.Title + return nil +} + +// compilerEdgeEntry is one element of a plain-array edges list: +// `{"source": , "target": , "relationship": +// }`, where each vertex is either form resourceSpecWire +// accepts. Relationship is intentionally undeclared for the same reason +// noted on pdbEdgeEntry. type compilerEdgeEntry struct { Source resourceSpecWire `json:"source"` Target resourceSpecWire `json:"target"` diff --git a/internal/transport/client.go b/internal/transport/client.go index fda3041..0ea4323 100644 --- a/internal/transport/client.go +++ b/internal/transport/client.go @@ -38,6 +38,12 @@ type Client struct { host string // authority (host:port) this client is dedicated to timeout time.Duration maxBodyBytes int64 + // observer, when non-nil, receives one Event per executed request. + // See debug.go for the seam and its redaction boundary. + observer Observer + // captureBodies makes each emitted Event carry raw request/response + // bodies. Off unless a caller opted in with WithBodyCapture. + captureBodies bool } // Option customizes a Client at construction time. Callers building the @@ -217,25 +223,93 @@ func (c *Client) Do(req *http.Request, timeout time.Duration) (*Response, error) defer cancel() req = req.WithContext(ctx) + // The request body is snapshotted before the request is sent, while + // req.GetBody still can replay it; net/http consumes the original + // reader. Only done when a caller opted into body capture. + var requestBody []byte + if c.observer != nil && c.captureBodies { + requestBody = snapshotRequestBody(req) + } + started := time.Now() + resp, err := c.httpClient.Do(req) if err != nil { - return nil, classifyDoErr(req.URL.Host, err) + classified := classifyDoErr(req.URL.Host, err) + c.observe(req, requestBody, nil, 0, "", time.Since(started), classified) + return nil, classified } defer resp.Body.Close() limited := &io.LimitedReader{R: resp.Body, N: c.maxBodyBytes + 1} body, readErr := io.ReadAll(limited) if readErr != nil { - return nil, newError(KindUnknown, req.URL.Host, "reading response body", readErr) + wrapped := newError(KindUnknown, req.URL.Host, "reading response body", readErr) + c.observe(req, requestBody, nil, resp.StatusCode, resp.Header.Get("Content-Type"), time.Since(started), wrapped) + return nil, wrapped } if int64(len(body)) > c.maxBodyBytes { - return nil, newError(KindResponseTooLarge, req.URL.Host, + tooLarge := newError(KindResponseTooLarge, req.URL.Host, fmt.Sprintf("response body exceeded the %d byte limit", c.maxBodyBytes), nil) + c.observe(req, requestBody, nil, resp.StatusCode, resp.Header.Get("Content-Type"), time.Since(started), tooLarge) + return nil, tooLarge } + c.observe(req, requestBody, body, resp.StatusCode, resp.Header.Get("Content-Type"), time.Since(started), nil) return &Response{StatusCode: resp.StatusCode, Header: resp.Header, Body: body}, nil } +// snapshotRequestBody replays req's body via GetBody, which +// http.NewRequestWithContext populates for the in-memory readers this +// package's callers use. It returns nil for a request with no body, or +// one whose body cannot be replayed — debug observation must never +// change what is sent or fail a request. +func snapshotRequestBody(req *http.Request) []byte { + if req.GetBody == nil { + return nil + } + rc, err := req.GetBody() + if err != nil { + return nil + } + defer rc.Close() + data, err := io.ReadAll(rc) + if err != nil { + return nil + } + return data +} + +// observe emits one Event to the configured Observer, if any. It is the +// only place this package derives anything from a response body, and it +// derives only the body's outermost shape and top-level member names +// (see debug.go). Raw bodies are attached solely when the Client was +// built WithBodyCapture. +func (c *Client) observe(req *http.Request, requestBody, responseBody []byte, statusCode int, contentType string, elapsed time.Duration, err error) { + if c.observer == nil { + return + } + shape, keys, truncated := describeBody(responseBody) + ev := Event{ + Method: req.Method, + URL: req.URL.String(), + Host: req.URL.Host, + StatusCode: statusCode, + Duration: elapsed, + RequestBodyBytes: req.ContentLength, + ResponseBodyBytes: len(responseBody), + ContentType: contentType, + Shape: shape, + TopLevelKeys: keys, + KeysTruncated: truncated, + Err: err, + } + if c.captureBodies { + ev.RequestBody = requestBody + ev.ResponseBody = responseBody + } + c.observer(ev) +} + // Host returns the authority (host:port) this Client is dedicated to, safe // to include in logs/diagnostics. func (c *Client) Host() string { return c.host } diff --git a/internal/transport/debug.go b/internal/transport/debug.go new file mode 100644 index 0000000..eef35bd --- /dev/null +++ b/internal/transport/debug.go @@ -0,0 +1,147 @@ +// This file implements the operator-facing `--debug` observation seam. +// +// Client.Do is the single chokepoint every compiler and PuppetDB request +// in PIACE passes through, so one Observer wired at construction covers +// both services and all three subcommands without any adapter needing a +// debug field of its own. +// +// Redaction boundary (requirements.md 3.5, and this package's Sanitize +// contract in redact.go): an Event carries only safe metadata by +// default — method, URL, host, status, duration, body sizes, content +// type, and the response body's *top-level JSON member names*. Member +// names, not member values: `{"catalog": {...}}` yields ["catalog"], +// which is enough to diagnose a wire-shape mismatch without putting one +// byte of catalog content into a CI log. +// +// Raw bodies are carried only when a caller explicitly opts in with +// WithBodyCapture. That is a deliberate, operator-requested bypass of +// the redaction boundary: a captured body can contain Puppet Sensitive +// values and unredacted catalog parameters. cmd/piace only enables it +// for --debug-dump-dir, which writes to 0600 files in an operator-named +// directory and never to stdout/stderr. +package transport + +import ( + "bytes" + "encoding/json" + "time" +) + +// maxTopLevelKeys bounds how many top-level member names one Event +// reports, so a pathological response cannot turn one debug line into +// thousands of columns. A catalog document has fewer than ten. +const maxTopLevelKeys = 64 + +// BodyShape classifies a response body's outermost JSON structure. +type BodyShape string + +const ( + // ShapeEmpty is a zero-length body. + ShapeEmpty BodyShape = "empty" + // ShapeObject is a JSON object; Event.TopLevelKeys names its members. + ShapeObject BodyShape = "object" + // ShapeArray is a JSON array. + ShapeArray BodyShape = "array" + // ShapeScalar is a bare JSON string/number/bool/null. + ShapeScalar BodyShape = "scalar" + // ShapeNonJSON is a body that does not parse as JSON. That is an + // ordinary, expected outcome for an endpoint that does not serve + // JSON — the compiler's file_content endpoint returns + // application/octet-stream — so the name states the fact rather than + // implying a fault. + ShapeNonJSON BodyShape = "non-json" +) + +// Event is one observed request/response. Every field except +// RequestBody/ResponseBody is safe to print to a CI log. +type Event struct { + Method string + // URL is the full request URL, including any query string. PIACE only + // ever puts certnames, environments, and endpoint paths in a URL, all + // of which already appear in ordinary diagnostics. + URL string + Host string + // StatusCode is zero when no response was received (Err is set). + StatusCode int + Duration time.Duration + // RequestBodyBytes is the request body length, taken from + // http.Request.ContentLength; -1 when unknown. + RequestBodyBytes int64 + ResponseBodyBytes int + ContentType string + Shape BodyShape + // TopLevelKeys holds the response body's top-level JSON member names + // in wire order (member *names* only — never values), truncated at + // maxTopLevelKeys. Empty unless Shape is ShapeObject. + TopLevelKeys []string + // KeysTruncated reports that TopLevelKeys was cut at maxTopLevelKeys. + KeysTruncated bool + // Err is the transport failure, when the request produced no response. + Err error + + // RequestBody and ResponseBody are populated only when the Client was + // built WithBodyCapture(true). They are raw and unredacted: see this + // file's package comment. + RequestBody []byte + ResponseBody []byte +} + +// Observer receives one Event per request executed by a Client. It is +// called synchronously from Do, after the response body has been read. +type Observer func(Event) + +// WithObserver installs obs on the Client. A nil obs disables +// observation (the zero value), so callers can pass one through +// unconditionally. +func WithObserver(obs Observer) Option { + return func(c *Client) { c.observer = obs } +} + +// WithBodyCapture makes the Client include raw request and response +// bodies in every Event it emits. Off by default. See this file's +// package comment for why enabling it is a deliberate redaction bypass. +func WithBodyCapture(enabled bool) Option { + return func(c *Client) { c.captureBodies = enabled } +} + +// describeBody classifies body's outermost JSON structure and, for an +// object, collects its top-level member names. It decodes member values +// as json.RawMessage rather than into any typed structure, so no member +// value is ever interpreted, retained, or returned. +func describeBody(body []byte) (BodyShape, []string, bool) { + if len(body) == 0 { + return ShapeEmpty, nil, false + } + dec := json.NewDecoder(bytes.NewReader(body)) + tok, err := dec.Token() + if err != nil { + return ShapeNonJSON, nil, false + } + delim, ok := tok.(json.Delim) + if !ok { + return ShapeScalar, nil, false + } + if delim != '{' { + return ShapeArray, nil, false + } + + var keys []string + truncated := false + for dec.More() { + nameTok, err := dec.Token() + if err != nil { + return ShapeNonJSON, keys, truncated + } + var value json.RawMessage + if err := dec.Decode(&value); err != nil { + return ShapeNonJSON, keys, truncated + } + name, _ := nameTok.(string) + if len(keys) >= maxTopLevelKeys { + truncated = true + continue + } + keys = append(keys, name) + } + return ShapeObject, keys, truncated +} diff --git a/internal/transport/debug_test.go b/internal/transport/debug_test.go new file mode 100644 index 0000000..bfde870 --- /dev/null +++ b/internal/transport/debug_test.go @@ -0,0 +1,193 @@ +package transport + +import ( + "context" + "net/http" + "strings" + "testing" +) + +func TestDescribeBody(t *testing.T) { + cases := []struct { + name string + body string + wantShape BodyShape + wantKeys []string + }{ + {"empty", "", ShapeEmpty, nil}, + {"v4 envelope", `{"catalog": {"name": "web-01", "resources": []}}`, ShapeObject, []string{"catalog"}}, + {"v3 document", `{"name": "web-01", "version": 1, "resources": [], "edges": []}`, ShapeObject, + []string{"name", "version", "resources", "edges"}}, + {"array", `[1, 2, 3]`, ShapeArray, nil}, + {"scalar", `"hello"`, ShapeScalar, nil}, + {"non-json", `{not json`, ShapeNonJSON, nil}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + shape, keys, truncated := describeBody([]byte(tc.body)) + if shape != tc.wantShape { + t.Errorf("shape = %q, want %q", shape, tc.wantShape) + } + if strings.Join(keys, ",") != strings.Join(tc.wantKeys, ",") { + t.Errorf("keys = %v, want %v", keys, tc.wantKeys) + } + if truncated { + t.Error("truncated = true, want false") + } + }) + } +} + +// TestDescribeBody_NeverReturnsValues asserts the one property that makes +// an Event safe to print to a CI log under requirements.md 3.5: only +// top-level member *names* are collected, never member values, however +// deeply the value nests. +func TestDescribeBody_NeverReturnsValues(t *testing.T) { + body := `{"catalog": {"resources": [{"parameters": {"password": "s3cret"}}]}}` + shape, keys, _ := describeBody([]byte(body)) + if shape != ShapeObject { + t.Fatalf("shape = %q, want %q", shape, ShapeObject) + } + if len(keys) != 1 || keys[0] != "catalog" { + t.Fatalf("keys = %v, want [catalog]", keys) + } + for _, k := range keys { + if strings.Contains(k, "s3cret") { + t.Errorf("member value leaked into keys: %q", k) + } + } +} + +func TestDescribeBody_TruncatesManyKeys(t *testing.T) { + var b strings.Builder + b.WriteString("{") + for i := 0; i < maxTopLevelKeys+10; i++ { + if i > 0 { + b.WriteString(",") + } + b.WriteString(`"k`) + b.WriteString(string(rune('a' + i%26))) + b.WriteString(string(rune('a' + i/26))) + b.WriteString(`": 1`) + } + b.WriteString("}") + + shape, keys, truncated := describeBody([]byte(b.String())) + if shape != ShapeObject { + t.Fatalf("shape = %q, want %q", shape, ShapeObject) + } + if len(keys) != maxTopLevelKeys { + t.Errorf("len(keys) = %d, want %d", len(keys), maxTopLevelKeys) + } + if !truncated { + t.Error("truncated = false, want true") + } +} + +func TestClient_Do_EmitsObserverEvent(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"catalog": {"name": "web-01"}}`)) + }) + defer srv.Close() + + var events []Event + client, err := NewClient(fixture.endpointFor(t, srv.URL), WithObserver(func(ev Event) { + events = append(events, ev) + })) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + + req, err := client.NewRequest(context.Background(), http.MethodPost, srv.URL+"/puppet/v4/catalog", strings.NewReader(`{"certname":"web-01"}`)) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + if _, err := client.Do(req, 0); err != nil { + t.Fatalf("Do: %v", err) + } + + if len(events) != 1 { + t.Fatalf("len(events) = %d, want 1", len(events)) + } + ev := events[0] + if ev.Method != http.MethodPost || !strings.HasSuffix(ev.URL, "/puppet/v4/catalog") { + t.Errorf("Method/URL = %s %s", ev.Method, ev.URL) + } + if ev.StatusCode != http.StatusOK { + t.Errorf("StatusCode = %d, want 200", ev.StatusCode) + } + if ev.Shape != ShapeObject || len(ev.TopLevelKeys) != 1 || ev.TopLevelKeys[0] != "catalog" { + t.Errorf("Shape/TopLevelKeys = %q/%v, want object/[catalog]", ev.Shape, ev.TopLevelKeys) + } + if ev.RequestBodyBytes != int64(len(`{"certname":"web-01"}`)) { + t.Errorf("RequestBodyBytes = %d", ev.RequestBodyBytes) + } + // Without WithBodyCapture, no raw body reaches the observer. + if ev.RequestBody != nil || ev.ResponseBody != nil { + t.Error("Event carries raw bodies without WithBodyCapture") + } +} + +func TestClient_Do_BodyCaptureCarriesRawBodies(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`{"catalog": {"name": "web-01"}}`)) + }) + defer srv.Close() + + var ev Event + client, err := NewClient(fixture.endpointFor(t, srv.URL), + WithObserver(func(e Event) { ev = e }), WithBodyCapture(true)) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + + req, err := client.NewRequest(context.Background(), http.MethodPost, srv.URL+"/puppet/v4/catalog", strings.NewReader(`{"certname":"web-01"}`)) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + if _, err := client.Do(req, 0); err != nil { + t.Fatalf("Do: %v", err) + } + + if string(ev.RequestBody) != `{"certname":"web-01"}` { + t.Errorf("RequestBody = %q", ev.RequestBody) + } + if string(ev.ResponseBody) != `{"catalog": {"name": "web-01"}}` { + t.Errorf("ResponseBody = %q", ev.ResponseBody) + } +} + +// TestClient_Do_EmitsObserverEventOnTransportFailure asserts a request +// that never produced a response is still observed — the case an +// operator running --debug most needs to see. +func TestClient_Do_EmitsObserverEventOnTransportFailure(t *testing.T) { + fixture := newTLSFixture(t, "127.0.0.1") + srv := newMTLSTestServer(t, fixture, func(w http.ResponseWriter, r *http.Request) {}) + ep := fixture.endpointFor(t, srv.URL) + srv.Close() // nothing is listening now + + var ev Event + seen := false + client, err := NewClient(ep, WithObserver(func(e Event) { ev, seen = e, true })) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + req, err := client.NewRequest(context.Background(), http.MethodGet, srv.URL+"/pdb/query/v4", nil) + if err != nil { + t.Fatalf("NewRequest: %v", err) + } + if _, err := client.Do(req, 0); err == nil { + t.Fatal("Do succeeded against a closed server") + } + if !seen { + t.Fatal("no Event emitted for a transport failure") + } + if ev.StatusCode != 0 || ev.Err == nil { + t.Errorf("Event = %+v, want StatusCode 0 and a non-nil Err", ev) + } +}