fix(enclaves): encode dynamic enclave max_identity_ttl in nanoseconds - #59290
Merged
Conversation
buildMCPGatewayDelegationEnvelope emitted "max_identity_ttl": enclave.Timeout, where enclave.Timeout is an integer number of seconds. mcpg v0.4.17 decodes max_identity_ttl into a Go time.Duration, whose JSON representation is an integer number of nanoseconds. A configured 120-second enclave therefore installed a 120-nanosecond identity ceiling, so mcpg's Store.validateAgainstEnvelope rejected every realistic AWF create-or-confirm request with 403 delegation_request_denied. AWF already sends nanoseconds (secondsToGoDurationNanos in src/enclave/delegation-control-client.ts), so gh-aw was the mismatched side. Encode the value as time.Duration(enclave.Timeout) * time.Second so encoding/json emits the exact nanosecond integer mcpg expects. Before: "max_identity_ttl":120. After: "max_identity_ttl":120000000000. The runtime envelope expiry clamp (expires_at, MCP_GATEWAY_DELEGATION_EXPIRES_AT, buildDynamicEnclaveExpiryScript) is a separate contract and remains in seconds / RFC3339, unchanged. Also bound enclaves[].timeout for dynamic enclaves at maxDynamicEnclaveTimeoutSeconds = 4740, matching gh-aw-firewall's MAX_ENCLAVE_TIMEOUT_SECONDS preflight and the awf-config schema. Previously gh-aw only enforced Timeout > 0, so gh-aw and AWF could disagree about what compiled, and time.Duration(enclave.Timeout) * time.Second could theoretically overflow int64 for pathological values. The new bound also keeps the multiplication trivially inside int64. Tests added: - TestBuildMCPGatewayDelegationEnvelopeMaxIdentityTTLNanoseconds pins the units contract by round-tripping the emitted JSON through a struct mirroring mcpg's Envelope.MaxIdentityTTL time.Duration, and by asserting that a 120-second AWF-requested TTL is accepted while a 121-second one is not - exactly the comparison mcpg's Store.validateAgainstEnvelope performs. - TestValidateDynamicEnclaveBoundsRejectsOversizedTimeout locks the new upper bound. - TestDynamicEnclaveGatewayContract now asserts the exact "max_identity_ttl":120000000000 wire value and explicitly rejects the pre-fix 120 to prevent a units regression. No .lock.yml files reference max_identity_ttl, so no workflows need recompilation. Fixes #59258 Unblocks github/gh-aw-firewall#8195 and the merged github/gh-aw-firewall#8276. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The new maxDynamicEnclaveTimeoutSeconds comment split the const block's alignment group, so gofmt rewrapped the two following assignments. CI's lint-go-format caught it. Whitespace only; no behaviour change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation matches the external contracts and includes focused regression coverage.
Review tier: Balanced
Findings: None
What changed in this PR
Fixes dynamic enclave delegation TTL serialization to match mcpg’s nanosecond-based time.Duration contract.
Changes:
- Serializes enclave timeouts as nanoseconds.
- Enforces the 4,740-second dynamic enclave limit.
- Adds regression and boundary tests.
| File | Description |
|---|---|
pkg/workflow/enclaves.go |
Corrects TTL encoding and validates timeout bounds. |
pkg/workflow/enclaves_test.go |
Verifies wire encoding, TTL comparisons, and limits. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Collaborator
|
@lpcox can we use seconds in user configurations and perform the conversion during parsing |
Contributor
|
🎉 This pull request is included in a new release. Release: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #59258.
The bug
buildMCPGatewayDelegationEnvelopeinpkg/workflow/enclaves.goemitted:enclave.Timeoutis an integer number of seconds (seeEnclaveConfig.Timeout, andbuildDynamicEnclaveExpiryScriptwhich computes$(date -u +%s) + <Timeout>). But mcpg v0.4.17 decodesmax_identity_ttlinto a Gotime.Duration, whose JSON representation is an integer number of nanoseconds (gh-aw-mcpginternal/delegation/envelope.go:MaxIdentityTTL time.Duration \json:"max_identity_ttl"``).A configured 120-second enclave therefore installed a 120-nanosecond identity ceiling. mcpg's
Store.validateAgainstEnvelopethen rejected every realistic AWFcreate-or-confirmrequest with403 delegation_request_denied, making the merged dynamic enclave runtime ingithub/gh-aw-firewall#8276non-functional in production. AWF already sends the contract-correct nanosecond value (secondsToGoDurationNanosinsrc/enclave/delegation-control-client.ts), so gh-aw was the mismatched side.Before / after wire value
For a workflow with
enclaves[].timeout: 120, the envelope embedded inMCP_GATEWAY_DELEGATION_ENVELOPE:max_identity_ttl120120000000000What changed
buildMCPGatewayDelegationEnvelopenow storestime.Duration(enclave.Timeout) * time.Secondinmax_identity_ttl, soencoding/jsonserializes the exact nanosecond integer mcpg expects.validateDynamicEnclaveBoundsnow rejectsenclaves[].timeout > 4740(newmaxDynamicEnclaveTimeoutSeconds). This matchesgh-aw-firewall'sMAX_ENCLAVE_TIMEOUT_SECONDSpreflight limit and theawf-configschema, so gh-aw and AWF cannot disagree about what compiles. It also keeps thetime.Duration(int) * time.Secondmultiplication comfortably insideint64.expires_at,MCP_GATEWAY_DELEGATION_EXPIRES_AT,buildDynamicEnclaveExpiryScript) is a separate contract and is deliberately unchanged; it remains in seconds / RFC3339.Tests
TestBuildMCPGatewayDelegationEnvelopeMaxIdentityTTLNanoseconds(new) pins the cross-component contract: it round-trips the emitted JSON through a struct mirroring mcpg v0.4.17'sEnvelope.MaxIdentityTTL time.Duration, asserts the exact120000000000wire value, and confirms an AWF-requested 120 s TTL is accepted while a 121 s one exceeds the ceiling — exactly the comparisonStore.validateAgainstEnvelopeperforms.TestValidateDynamicEnclaveBoundsRejectsOversizedTimeout(new) locks the new upper bound.TestDynamicEnclaveGatewayContractnow asserts the exact\"max_identity_ttl\":120000000000substring in the generated shell and explicitly rejects the pre-fix\"max_identity_ttl\":120,to prevent a units regression.Recompile
grep -rl max_identity_ttl .github/workflows/returns nothing — no workflow currently compiles a dynamic enclave envelope — so no.lock.ymlregeneration is required.Validation
go build ./...cleango vet ./pkg/workflow/...cleango test ./pkg/workflow/...— all enclave / delegation tests pass. One unrelated pre-existing failure remains (TestGeneratePackageLock_UsesNormalizedWorkflowDir, a macOS/private/varvs/varsymlink mismatch that reproduces onmain).Unblocks
github/gh-aw-firewall#8195and the mergedgithub/gh-aw-firewall#8276.