Skip to content

fix(resource,metadata,verifier,http): preserve issuer identity, derive-only slash handling, escaped-path PRM - #26

Open
RobertoIskandarani wants to merge 1 commit into
mainfrom
port/issuer-identity-prm
Open

fix(resource,metadata,verifier,http): preserve issuer identity, derive-only slash handling, escaped-path PRM#26
RobertoIskandarani wants to merge 1 commit into
mainfrom
port/issuer-identity-prm

Conversation

@RobertoIskandarani

Copy link
Copy Markdown

Replaces #23, split so the mechanical CI change and the semantic one are reviewed
separately. The catalog pin is #25; this is the identifier half.

What

Identifiers are identity, not something to normalise. The SDK stripped and
reconciled them in several places, and each was a silent rewrite of a value the
operator configured.

Identity is preserved verbatim (RFC 8414/9728 §3.3); slash removal happens only
at derivation (§3.1).
Those are the two concerns the old code conflated.

  • core/resource/verifier — the verifier stores the configured issuer
    byte-for-byte and compares a token's iss exactly. RFC 8414 §4 spells the
    comparison out as code-point-for-code-point with no normalisation. An
    authorization server whose issuer legitimately ends in / mints tokens whose
    iss keeps the slash; comparing against the stripped form rejected every
    otherwise-valid token, with no workaround short of misconfiguring the issuer.
  • core/internal/metadata — the §3.3 check compares both sides raw.
    Derivation is many-to-one: …/tenant and …/tenant/ share one well-known URL
    and the RFC provides no way to host both. The strict comparison is what turns
    that unavoidable collision into a clean discovery failure rather than a silent
    bind to a different issuer's metadata — the impersonation §3.3 and RFC 9728 §7.3
    exist to defeat.
  • core/resource — PRM derivation reads the escaped path, so a
    percent-encoded octet (RFC 3986 §3.3 path data) survives instead of decoding to
    a delimiter and changing which resource is named.

One rule, one place

verifier.ValidateIssuer is exported and every construction boundary calls it:
NewTokenVerifier, resource.New, and authplane.NewClient.

NewClient needs its own call rather than inheriting the verifier's — a *Client
used only for token, introspection and revocation never constructs a
TokenVerifier, so without it a relative or query-bearing issuer would reach
eager discovery ungated.

The rejected identifier is redacted to scheme and host. The query/fragment
branch fires precisely for the shape that carries a credential
(https://as.example.com?access_token=…), and a construction error lands in
whatever startup log is configured. Parse failures are still wrapped with %w so
errors.As(err, new(*url.Error)) keeps working; only the URL the error prints is
substituted, since url.Error does not redact.

http adapter

The RFC 9728 PRM discovery bypass compares r.URL.EscapedPath() against the
escaped well-known path. Comparing the decoded path let %2F collapse to /, the
two sides disagreed, and the discovery endpoint returned 401 — which §3.2 requires
to be publicly reachable.

Breaking changes

All enumerated in CHANGELOG.md with migration notes. They fall into two shapes:
construction-time rejections of configurations that used to start, and output
changes on WellKnownPRMPath() / PRMURL().

Worth singling out one that is easy to miss: authplane.ErrInvalidIssuer is now
an alias of verifier.ErrInvalidIssuer, so its message changes and
errors.Is(verifierErr, authplane.ErrInvalidIssuer) returns true where it
returned false. Code that only matched a NewClient error is unaffected.

Known gap, tracked

RFC 9728 §3.1 derives over the identifier's path and/or query; only the path
half is handled. Two identifiers differing solely by query still collapse onto one
document. Tracked at #24, which the TODO in core/resource/resource.go names —
deciding it means picking a behaviour every implementation has to share, so it is
deliberately not settled here.

Verification

go build, vet, test and gofmt green across core, http, mcp and
mark3labs. The conformance suite passes against the pinned catalog revision,
with rows added for the trailing-slash and escaped-path cases the catalog already
carries.

…e-only slash handling, escaped-path PRM

Identifiers are identity, not something to normalise. The SDK stripped and
reconciled them in several places, and every one of those was a silent rewrite of
a value the operator configured.

Identity is now preserved verbatim (RFC 8414/9728 §3.3) and slash removal happens
only at derivation (§3.1) — the two concerns the old code conflated:

- The token verifier stores the configured issuer byte-for-byte and compares a
  token's `iss` exactly (§4 spells the comparison out as code-point-for-code-point
  with no normalisation), so an AS whose issuer legitimately ends in `/` stops
  having every token rejected.
- The RFC 8414 §3.3 metadata check compares both sides raw. Derivation is
  many-to-one, and the strict comparison is what turns an unavoidable collision
  into a clean discovery failure instead of a silent bind to another issuer's
  metadata.
- PRM derivation reads the escaped path, so a percent-encoded octet survives
  instead of decoding to a delimiter and changing which resource is named.

One rule, one place. `verifier.ValidateIssuer` is exported and every construction
boundary calls it — `NewTokenVerifier`, `resource.New`, and `authplane.NewClient`,
which needs its own call because a client used only for token, introspection and
revocation never builds a verifier and would otherwise have no gate at all. The
rejected identifier is redacted to scheme and host: that branch fires precisely
for a credential-shaped query, and a construction error lands in a startup log.

The `net/http` adapter's PRM discovery bypass compares escaped paths on both
sides. Comparing the decoded path let `%2F` collapse to `/`, the two sides
disagreed, and the discovery endpoint returned 401 — which RFC 9728 §3.2 requires
to be publicly reachable.

Breaking changes are enumerated in the changelog with migration notes. All are
construction-time rejections of configurations that used to start, or output
changes on two exported methods.

Conformance rows added for the trailing-slash and escaped-path cases the catalog
already carries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant