docs(osep): propose exact request-header credential selectors - #1451
docs(osep): propose exact request-header credential selectors#1451andreweacott wants to merge 12 commits into
Conversation
github-actions
Bot
added
documentation
There was a problem hiding this comment.
Pull request overview
This PR adds a new OSEP proposal (OSEP-0018) describing an additive extension to Credential Vault bindings that enables disambiguation via exact request-header selectors while preserving OSEP-0012’s fail-closed behavior for ambiguous matches.
Changes:
- Introduces the
requestHeadersselector concept forCredentialMatch, including matching semantics and privacy boundaries. - Defines a conservative candidate-validation rule to allow overlapping destination scopes only when selectors prove bindings are mutually exclusive.
- Outlines expected SDK/CLI behavior and a cross-SDK conformance-oriented test plan.
Suppressed comments (2)
oseps/0018-exact-request-header-selectors-for-credential-binding-disambiguation.md:159
- This privacy rule bans selector values in “documentation examples”, but the proposal necessarily needs illustrative examples and already includes a
valuefield. Consider tightening the rule to “never echo configured selector values from real bindings” while allowing clearly non-production placeholder strings in docs/examples.
Do not include selector values in serialized metadata, API errors, structured logs, metrics labels, tracing attributes, diagnostics, test failure output, or documentation examples. Secret-free audit output may include binding name, decision (`matched`, `no_match`, or `ambiguous`), and selected header names.
oseps/0018-exact-request-header-selectors-for-credential-binding-disambiguation.md:128
- “After trimming outer HTTP optional whitespace, names and values must be non-empty” is ambiguous for header names (HTTP field-names are tokens and should not contain/allow whitespace). It would be clearer to specify strict field-name validation (RFC 9110 token) and apply OWS trimming only to values.
After trimming outer HTTP optional whitespace, names and values must be non-empty. A binding must not repeat a header name after case-insensitive normalization. This prevents unsatisfiable predicates.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
oseps/0018-exact-request-header-selectors-for-credential-binding-disambiguation.md:10
- This PR adds a new OSEP, but
oseps/README.md(the index that claims to be the “complete list”) is not updated to include OSEP-0018. Please add an entry so the proposal is discoverable alongside the others.
# OSEP-0018: Exact Request-Header Selectors for Credential Binding Disambiguation
oseps/0018-exact-request-header-selectors-for-credential-binding-disambiguation.md:118
maxItemsis set to 8 here, but requirement R1 and the Test Plan both define a limit of four request-header predicates. Please align the schema snippet with the stated limit (or update R1/Test Plan if 8 is intended).
maxItems: 8
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (1)
oseps/0018-exact-request-header-selectors-for-credential-binding-disambiguation.md:118
- The proposed request schema sets
requestHeaders.maxItems: 8, but both the safety boundary and requirement R1 state a binding may have at most four AND-combined request-header predicates. The schema should match the stated limit to avoid an inconsistent contract.
maxItems: 8
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efd3a5459f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@andreweacott Thanks for drafting this. The overall direction looks sound and Before implementation, I think we should resolve the existing OSEP number Once the proposal reaches implementable status, I’d be happy to own the egress |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91336ac36f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Thanks @luppyw I've applied review fixes:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1481f4f5c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@andreweacott The egress core implementation and regression-test scope works for me. I also noticed the latest P1 about SDK compatibility: changing CredentialBindingMetadata.match from the existing CredentialMatch type may break selector-free consumers across the supported SDKs. Before implementation, I suggest we agree on an additive compatibility model or explicitly document this as a breaking migration. I can help prepare the TypeScript SDK/schema tests first, then add matching and validation regression coverage for OWS normalization and Cookie semantics. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bed8871d9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
That implementation split works for me. Please take the TypeScript/schema tests, egress matching and validation, and the associated regression coverage. I’ll handle the remaining SDK/CLI integration, documentation, and cross-surface compatibility (see #1455) The latest revision:
|
|
@andreweacott Thanks, this implementation split works for me. I’ll wait for OSEP-0020 approval before opening the implementation PR, then take the OpenAPI/TypeScript schema, Credential Proxy matching and validation, and the associated regression tests. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 727425bf09
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| Existing scheme, host, method, and path checks determine the highest matching host precedence before selectors are evaluated. A binding at that precedence is eligible only when every `requestHeaders` predicate matches the original intercepted request. Credential Proxy injects a credential only when exactly one binding is eligible. | ||
|
|
||
| When no binding is eligible at the highest base host precedence, current behavior is preserved: inject no credential and let ordinary egress policy decide whether traffic may continue. Credential Proxy does not fall back to a lower-precedence binding. Multiple eligible bindings retain the current ambiguous-binding denial. |
There was a problem hiding this comment.
When an active vault request base-matches a selector-bound binding but omits or mismatches the selected header, this rule lets the request proceed whenever ordinary egress policy allows the host. OSEP-0012 requires Credential Proxy to deny non-matching bindings (oseps/0012-credential-vault.md:98,1218), so this would turn a wrong-placeholder request from a denial into unauthenticated upstream traffic while describing the change as additive and current behavior; either preserve the denial or explicitly revise the parent protocol and migration story.
AGENTS.md reference: AGENTS.md:L55-L58
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Changed to preserve OSEP-0012’s default-deny behavior -if no binding is selector-eligible at the highest precedence, Credential Proxy returns a 403 selector-mismatch response
| 3. Evaluate every normalized `requestHeaders` predicate only for bindings at that precedence: | ||
| - Header names compare case-insensitively. | ||
| - A selected header must have exactly one received field occurrence before library coalescing. Two field lines do not match; one field line containing a comma remains one occurrence whose entire value is compared. | ||
| - After trimming only outer SP and HTAB, the request value must exactly equal the canonical configured value. | ||
| - Values otherwise receive no case folding, decoding, internal-whitespace normalization, prefix matching, or expression evaluation. | ||
| 4. Zero eligible bindings yields no injection. Exactly one is selected. Two or more retain the current ambiguous-binding denial. |
There was a problem hiding this comment.
With an allowed pair consisting of a broad /* binding selected by X: A and a narrower /foo/bar* binding selected by X: B, /foo%2fbar with X: A is uniquely eligible under this algorithm. The existing preselection check _path_encoded_slash_changes_binding in components/egress/mitmscripts/system.py:298-308 compares base-binding index sets before selectors, so decoding adds the narrower binding and the request receives a 403 before selection; require this safety comparison to use selector eligibility and add coverage for this interaction.
AGENTS.md reference: AGENTS.md:L119-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Changed to require encoded-slash safety check to compare selector-eligible outcomes for raw and decoded paths, rather than base matches alone
| | R1 | `CredentialMatch` may contain one to four AND-combined exact request-header predicates. | Must Have | | ||
| | R2 | Header names match case-insensitively. Configured values and request values match case-sensitively after trimming only outer HTTP optional whitespace (SP and HTAB). | Must Have | | ||
| | R3 | Each selected request header must occur exactly once; missing or repeated header fields do not satisfy a predicate. | Must Have | | ||
| | R4 | Selectors permit ordinary end-to-end headers, including `Authorization`, but reject HTTP/2 pseudo-headers and these case-insensitive names: `Host`, `Content-Length`, `Content-Type`, `Transfer-Encoding`, `Connection`, `Upgrade`, `TE`, `Trailer`, `Cookie`, `Proxy-Authorization`, `Proxy-Authenticate`, `Forwarded`, `X-Forwarded-For`, `X-Forwarded-Host`, and `X-Forwarded-Proto`. The egress API contract is the maintained source for this fixed denylist; changes are reviewed public-contract changes. | Must Have | |
There was a problem hiding this comment.
Content-Type doesn't fit the stated rationale for this denylist, and the list appears to be lifted verbatim from OSEP-0012's injection header denylist, whose risk model differs from matching.
The requirement characterizes the list as routing/framing/hop-by-hop/proxy-control/forwarding headers, but Content-Type is an end-to-end entity header — it is none of those. Rejecting it as a selector name may be a defensible conservative choice, but it needs an explicit rationale (e.g., proxies may rewrite or normalize Content-Type, making exact matching deployment-fragile). The asymmetry matters too: for injection you must never touch Content-Type for content-semantics reasons, whereas matching on a client-sent Content-Type could legitimately disambiguate API variants behind the same host/path.
Suggest either removing Content-Type from the list or stating why it is excluded. A one-liner for Cookie (clients reorder/merge cookies, so exact matching is fragile) would also help readers understand the boundary.
There was a problem hiding this comment.
Agreed. I've removed Content-Type from the deny list because it's an end-to-end entity header and matching doesn't alter its semantics. Cookie remains excluded, and I've added a rationale for that to the row.
| additionalProperties: false | ||
| ``` | ||
|
|
||
| The `oneOf` rejects representations that contain neither directional field or both directional fields. It does not choose the correct operation direction. Create and patch handlers require `name` and `value` and reject `valueConfigured`; get and list serializers require `name` and `valueConfigured` and must never emit `value`. Operation-level schema and conformance tests enforce those directional rules. |
There was a problem hiding this comment.
valueConfigured looks like a tautology that exists only to make the oneOf discriminable, and the resulting oneOf + writeOnly/readOnly combination is a known codegen risk.
Since a selector always carries a value at write time, the presence of a requestHeaders entry in a read response already means the selector is configured — valueConfigured: true carries zero information (there is no "declared but unconfigured selector" state). Its only purpose is to satisfy oneOf: [required: [value], required: [valueConfigured]].
The concern: many OpenAPI generators strip writeOnly properties from response schemas, which leaves the required: [value] branch unsatisfiable on reads and tends to produce never/degenerate generated types; combining that with readOnly + enum: [true] is likewise fragile. The proposal already has to fall back to "operation-level schema and conformance tests enforce those directional rules", which suggests the shared schema level cannot express the intent cleanly.
Suggested simplification: drop valueConfigured (and the oneOf), return [{ name }] in reads, and enforce direction via operation-level schemas (write requires value; reads never emit it). This removes the tautological field and the codegen hazard without forking CredentialMatch — the fork concern in Alternatives applies to CredentialMatch, not to the new selector shape.
There was a problem hiding this comment.
I've removed valueConfigured and the shared oneOf. Instead, I've proposed using CredentialMatchInput / RequestHeaderSelectorInput where selectors require {name, value} and get/list responses retain the existing CredentialMatch type.
727425b to
6018b29
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d058e90f6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| additionalProperties: false | ||
| ``` | ||
|
|
||
| `CredentialBindingInput.match` uses `CredentialMatchInput`; get and list serializers use `CredentialMatch` and emit only `name` for each selector. They must never emit `value`. Operation-level schema and conformance tests enforce those directional rules. |
There was a problem hiding this comment.
The new CredentialBindingInput.match split moves the compatibility break to the write side: the current Go, C#, Kotlin, Python, and JavaScript SDKs expose CredentialBinding.match as CredentialMatch, so repointing create and mutation bindings to CredentialMatchInput makes existing typed code that passes a CredentialMatch incompatible even when it uses no selectors. Keep CredentialMatchInput behind wire adapters that preserve the public CredentialBinding.match type, or document this as a versioned breaking change; the proposed compatibility test only protects CredentialBindingMetadata.match.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This seems to be in disagreement with the comment from @Pangjiping.
I'll defer to you which way you think makes most sense.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6b6a544a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| CredentialMatchInput: | ||
| type: object | ||
| # Same schemes, hosts, methods, and paths as CredentialMatch. |
There was a problem hiding this comment.
When implementing this schema split, existing create or replace payloads that include the still-accepted match.ports property from specs/egress-api.yaml:559-564 will be rejected: the proposed CredentialMatchInput copies only schemes, hosts, methods, and paths, while retaining additionalProperties: false. Even though ports is deprecated and standard values are ignored, it remains part of the public input contract, so preserve it in the split schema to keep this change additive.
AGENTS.md reference: AGENTS.md:L47-L47
Useful? React with 👍 / 👎.
Summary
Address Issue #1373
This draft adds OSEP-0020, a proposal for choosing between Credential Vault bindings that otherwise have the same scheme, host, method, and path.
Today, Credential Proxy fails closed when more than one binding matches. This makes two legitimate credentials for the same destination shape impossible, even when the client already sends a distinct non-secret placeholder or authentication form that could distinguish the request.
The OSEP proposes optional exact request-header selectors on a binding. A selector refines an existing destination match; it does not replace destination matching or become a general request-policy language.
Proposed safety boundary
What this does not propose
Review focus
Feedback is particularly useful on the exact-match contract, duplicate-header handling, the conservative static overlap rule, sanitized read shape, and the cross-SDK compatibility rollout.
Testing
Breaking Changes
Checklist