Repository navigation
Conversation
Signed-off-by: Francisco Delgado Lopez <fdelgadolope@nvidia.com>
johnnygreco
left a comment
johnnygreco
left a comment
There was a problem hiding this comment.
@delgadof — I recommend addressing two P2 defects before merging. Both were reproduced against fb1eb17651d741131d00022ea3c372ddfc63a19b; the inline comments include the triggers and focused remedies.
- Configured key redaction can leak through newly derived identifiers. Native OCSF UIDs, relay session aliases, and container-derived CloudEvent fields are copied from unredacted input under different names. Focused serialized-output assertions fail on this head and pass when the two relevant implementation files are restored from the merge-base.
- A name-based watch can misattribute a replacement sandbox’s events. After delete/recreate with the same name, the gateway returns the new sandbox while the watch retains the old discovery ID. A deterministic test through the real watch loop and normalizer produced a replacement-B lifecycle/payload but a deleted-A envelope and CloudEvent source.
The retry-copy approach and context projections from redacted input are sound within the inspected paths. I rejected the proposed Prometheus metric-suffix issue after checking the pinned Collector defaults and a passing test using its actual exporter.
Panel: GPT-6.1 Sol/high reviewed receiver lifecycle, retries and delivery correctness; GPT-6 Astra/high reviewed privacy, identity and normalization; GPT-6.1 Sol/medium reviewed configuration, packaging, dependencies and diagnostics. I assessed the findings and independently reran both reproductions and the redaction baseline comparison.
Validation independently run on Linux AMD64 with Go 1.27.2:
- Passed:
go mod verify,go vet ./...,go test -race -count=1 ./..., andgo build ./cmd/.... - Passed:
go run ./cmd/openshell-event-exporter validate --config config.yaml; all 83 source-file hashes; the focused test using the pinned Prometheus exporter and status renderer. - The added review-only regression assertions intentionally fail on the PR head for the defects above. All four redaction cases pass with the two relevant files restored from the merge-base under the PR dependency graph. Reproduction files remained in separate temporary copies.
Review record: findings. Base and merge-base 6ea69fa660dc4376ec1a74f7692b34a4ba6e8ea4; head fb1eb17651d741131d00022ea3c372ddfc63a19b; scope git diff 6ea69fa660dc4376ec1a74f7692b34a4ba6e8ea4 fb1eb17651d741131d00022ea3c372ddfc63a19b -- projects/tools/openshell-exporter. No uncommitted work included or repository files changed. Docker runtime, live gateway/external destinations, ARM64/macOS runtime, sustained capacity, and the separate integration harness were not rerun; synthetic reproductions do not qualify those environments.
| if isOCSFEvidence(kind) { | ||
| if value, ok := lookupPath(original, "metadata.uid"); ok { | ||
| correlation["ocsf_event_uid"] = value | ||
| } |
There was a problem hiding this comment.
[P2] Derive exported identifiers from the redacted source
This new copy reads metadata.uid from original after redactedOriginal has been computed. With uid (or its parent metadata) in redaction.keys, the source is redacted but correlation.ocsf_event_uid still contains the raw value: the final key-based pass sees a different key. I reproduced this in the serialized output. The new nemo_relay.session_id aliases and container.uid → CloudEvent source/subject path have the same problem. All four regression assertions fail here and pass with processor.go/envelope.go restored to the merge-base under the same dependencies.
Keep stable-ID hashing on pristine input, but derive exported correlations and sandbox source/subject values from redacted input/attributes. Add assertions over the complete serialized record for both sensitive leaf keys and redacted parent objects; checking only original misses the leak.
| return | ||
| } | ||
| stream, err := r.client.Watch(ctx, sandbox.ID) | ||
| stream, err := r.client.Watch(ctx, sandbox.Name) |
There was a problem hiding this comment.
[P2] Verify the immutable sandbox ID after opening a watch by name
If discovery saw {ID: A, Name: demo} and A is deleted/recreated as B before this call or a reconnect, the name-based RPC opens B while the loop continues passing A's captured sandboxRef to conversion. The initial lifecycle event reports B, but subsequent logs/platform events/draft notifications retain A's source attributes. A focused test through the real watch loop and normalizer produced lifecycle ID=replacement-B, log payload ID=replacement-B, normalized log ID=deleted-A, and a CloudEvent source containing /sandboxes/deleted-A/.
This lasts until successful rediscovery cancels the stale watch (30 seconds by default, longer during discovery failures), corrupting attribution and potentially changing replay identities. Validate the first lifecycle snapshot against the discovered immutable ID before accepting records or scheduling stream-connected reconciliation; cancel and rediscover on mismatch. Add a delete/recreate-between-discovery-and-watch regression test.
This branch has not been deployed
2 participants
Refresh the self-contained exporter in
projects/tools/openshell-exporterwith the current implementation and local/Docker instructions. Gateway collection now requires OpenShell >=0.1.0 and targets v0.1.3, including gateway JSONL and structured supervisor logs.The update preserves source records and stable IDs across downstream retries, bounds reconnect/shutdown behavior, adds delivery diagnostics, and exposes searchable actor/action/target/outcome/policy context. It updates Go and Collector dependencies while retaining the security fixes already on Research main.
source.jsonrecords the exact source snapshot and per-file hashes for repeatable updates. The project keeps its existing module path, license and notices. This change is limited to the exporter project and adds no deployment scripts or shared CI changes.Validation from the Research project:
go mod verify, unchangedgo mod tidy,go vet ./...,go test -race -count=1 ./..., and both command builds passed.Validation used Linux AMD64 and isolated synthetic sources/receivers. ARM64 runtime, live gateway/external application qualification, and sustained capacity testing were not rerun. The separate OTLP proxy is outside this public distribution and its restart test was skipped. No release tag or image is published by this PR.