Skip to content

Commit e337a6f

Browse files
SriganeshNkclaude
authored andcommitted
Add OAuth 2.1 resource-server package (#70)
* Add OAuth 2.1 resource-server package pkg/oauth implements the resource-server side of OAuth for the MCP server, validated against the authorization server by RFC 7662 token introspection (the MCP server has no direct access to token storage): - Config loaded from OAUTH_* env vars, validated at startup, with the resource URI held in RFC 3986 canonical form so audience comparison is immune to equivalent spellings. - RFC 9728 protected-resource metadata document, served at both the path-insertion well-known URI (RFC 9728 section 3.1) and the root form. - RFC 6750 bearer middleware: introspected OAuth tokens are checked against this server's audience; non-OAuth bearers pass through to the Render API unchanged so existing API-key users are unaffected. Introspection outages fail closed as 503, not invalid_token, so clients don't discard valid tokens during a blip. - Introspection client with a bounded in-process cache (sha256 keys, TTL capped by token expiry, negative caching) and collapsing of concurrent lookups for the same token. Nothing is wired into the server yet; that lands separately behind OAUTH_ENABLED. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Reject query and fragment components in OAuth config URLs A query on either URL would corrupt derived URLs built by concatenation (the introspection endpoint, the RFC 9728 path-insertion metadata URL — which previously dropped the query while audience matching preserved it), and RFC 8707 forbids fragments in resource indicators. Neither component identifies anything for these values, so fail at startup instead of carrying them inconsistently. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Accept bare API keys through the OAuth middleware and harden config edges Round-2 self-review fixes: - Clients may send API keys without the Bearer scheme — the existing header parsing accepts that, so enabling OAuth must not 401 them. Credential extraction moves to a shared authn.BearerToken helper (strip the scheme case-insensitively when present, else use the raw value) so the middleware and the server's context func can never disagree about what the credential is. Bare credentials flow through the same introspection path: bare API keys pass through exactly as before, and a bare OAuth token still gets the full audience check. - Boolean env vars now reject unrecognized values instead of silently treating them as false: OAUTH_ENABLED=on previously disabled OAuth and skipped all startup validation; a typo'd OAUTH_API_KEY_PASSTHROUGH silently enabled strict mode. - The query/fragment guard checks the raw string, catching a bare trailing "?" or "#" (url.Parse reports those as no query/fragment) that would corrupt the concatenated introspection URL. - Audience values are canonicalized once at decode, so the per-request audience check is plain string equality; the middleware canonicalizes its configured resource at construction so hand-built Configs can't silently break matching, and precomputes the challenge string. - Client disconnects while waiting on introspection are no longer logged as introspection failures (they'd pollute the outage signal). - The introspection 401 error distinguishes "no service token configured" from "service token rejected". - Drop the unused IssuedAt field; consolidate duplicate test fixtures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Use BearerToken in ContextWithAPITokenFromHeader The helper's contract is that every Authorization parser goes through it; adopt it at the one existing parse site in the same PR that introduces it. Behavior change is limited to case-variant schemes ("bearer x"), which RFC 7235 says must be accepted and which previously failed downstream with the prefix left in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address PR review: split cache file, nearest-expiry eviction, table tests - Move tokenCache into its own file (tokencache.go) so introspect.go is just the introspection client; tests move alongside it. - Eviction now drops the entries closest to expiry (not arbitrary map-iteration order) down to three-quarters capacity, so it sheds the entries with the least useful life left and the policy is described accurately. - Collapse the formulaic FromEnv validation-error subtests into a table (TestFromEnv_Errors); behavior coverage is unchanged. - Note in wellknown.go that the metadata body is marshaled once at startup, not per request. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address review: table-driven middleware/metadata tests, trim comments Per review feedback (wendorf): - Collapse the per-case TestMiddleware_* functions into one input→output table (the structurally different disabled-identity case stays separate); same for the TestHandleProtectedResourceMetadata status matrix, with the JSON document-shape assertions kept as a focused test. - Tighten the wordier doc comments (Introspector, Introspect, canonicalResourceURI, evictLocked, APIKeyPassthrough) without dropping the rationale. - Drop the self-evident fakeAS doc comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Use Lock/defer Unlock consistently in test helpers Adopt the Lock() / defer Unlock() idiom in the two test lock sites that released manually before a trailing non-critical call. The early unlock saved nothing here, and the deferred form is harder to get wrong and consistent with the rest of the package. The singleflight in Introspect keeps its manual unlock on purpose: it releases the lock before the introspection network call, so concurrent lookups don't serialize behind one request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> GitOrigin-RevId: e1e811c99d6e311cbdefa824267beceb7fc6b253
1 parent 36f10f9 commit e337a6f

12 files changed

Lines changed: 1633 additions & 7 deletions

pkg/authn/authn.go

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"errors"
66
"log"
77
"net/http"
8+
"strings"
89

910
"github.com/render-oss/render-mcp-server/pkg/cfg"
1011
"github.com/render-oss/render-mcp-server/pkg/logging"
@@ -25,6 +26,18 @@ func ContextWithAPIToken(ctx context.Context, token string) context.Context {
2526
return context.WithValue(ctx, apiTokenKey, token)
2627
}
2728

29+
// BearerToken extracts the credential from an Authorization header value: the
30+
// value with its "Bearer " prefix removed (case-insensitively, per RFC 7235)
31+
// when present, otherwise the value unchanged — some MCP clients send bare
32+
// tokens without the scheme. Every parser of the Authorization header must go
33+
// through this so they can't disagree about what the credential is.
34+
func BearerToken(headerValue string) string {
35+
if len(headerValue) > 7 && strings.EqualFold(headerValue[:7], "Bearer ") {
36+
return headerValue[7:]
37+
}
38+
return headerValue
39+
}
40+
2841
func ContextWithAPITokenFromHeader(ctx context.Context, req *http.Request) context.Context {
2942
token := req.Header.Get("Authorization")
3043

@@ -33,13 +46,7 @@ func ContextWithAPITokenFromHeader(ctx context.Context, req *http.Request) conte
3346
return ctx
3447
}
3548

36-
// Note: we strip the "Bearer " prefix if it exists
37-
// MCP Inspector attaches this prefix automatically, but it's unclear how standard this is
38-
if len(token) > 7 && token[:7] == "Bearer " {
39-
token = token[7:]
40-
}
41-
42-
return ContextWithAPIToken(ctx, token)
49+
return ContextWithAPIToken(ctx, BearerToken(token))
4350
}
4451

4552
func ContextWithAPITokenFromConfig(ctx context.Context) context.Context {

pkg/authn/authn_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
package authn_test
2+
3+
import (
4+
"testing"
5+
6+
"github.com/render-oss/render-mcp-server/pkg/authn"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
func TestBearerToken(t *testing.T) {
11+
cases := map[string]struct {
12+
in string
13+
want string
14+
}{
15+
"strips the Bearer prefix": {"Bearer rnd_abc", "rnd_abc"},
16+
"strips case-insensitively": {"bearer rnd_abc", "rnd_abc"},
17+
"bare token passes through": {"rnd_abc", "rnd_abc"},
18+
"other schemes pass through": {"Basic dXNlcjpwYXNz", "Basic dXNlcjpwYXNz"},
19+
"empty stays empty": {"", ""},
20+
"prefix without a token is bare": {"Bearer ", "Bearer "},
21+
"prefix must end with the space": {"Bearerrnd_abc", "Bearerrnd_abc"},
22+
}
23+
for name, tc := range cases {
24+
t.Run(name, func(t *testing.T) {
25+
require.Equal(t, tc.want, authn.BearerToken(tc.in))
26+
})
27+
}
28+
}

pkg/oauth/config.go

Lines changed: 188 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,188 @@
1+
// Package oauth implements the OAuth 2.1 resource-server pieces of the Render
2+
// MCP server: RFC 9728 protected-resource metadata, RFC 6750 bearer-token
3+
// middleware, and a cached RFC 7662 token-introspection client.
4+
//
5+
// The MCP server has no direct access to token storage — it validates bearer
6+
// tokens by calling the authorization server's introspection endpoint.
7+
package oauth
8+
9+
import (
10+
"fmt"
11+
"net/url"
12+
"os"
13+
"strings"
14+
)
15+
16+
const wellKnownPath = "/.well-known/oauth-protected-resource"
17+
18+
// Config is the OAuth resource-server configuration, loaded from environment
19+
// variables by FromEnv.
20+
type Config struct {
21+
// Enabled gates the OAuth middleware. When false the server keeps its
22+
// pre-OAuth behavior — bearer tokens pass through to the Render API
23+
// unchanged — so enabling OAuth is opt-in per deployment.
24+
Enabled bool
25+
26+
// AuthorizationServerURL is the base URL of the authorization server. It
27+
// is advertised in the protected-resource metadata and used to build the
28+
// introspection endpoint URL.
29+
AuthorizationServerURL string
30+
31+
// CanonicalResourceURI identifies this resource server. Tokens are
32+
// accepted only when their introspected audience matches it. Held in
33+
// canonical form (see canonicalResourceURI).
34+
CanonicalResourceURI string
35+
36+
// IntrospectionServiceToken optionally authenticates this server to the
37+
// introspection endpoint.
38+
IntrospectionServiceToken string
39+
40+
// APIKeyPassthrough decides what happens to tokens the authorization
41+
// server reports inactive (e.g. Render API keys, which aren't OAuth
42+
// tokens). True (default) forwards them to the Render API so API-key users
43+
// keep working; false rejects them (strict OAuth-only mode).
44+
APIKeyPassthrough bool
45+
}
46+
47+
// FromEnv builds a Config from OAUTH_* environment variables. When
48+
// OAUTH_ENABLED is unset (or not "true"/"1"/"yes") it returns a disabled
49+
// Config and ignores every other variable; when set, the URL variables are
50+
// required and validated so a misconfigured deployment fails at startup
51+
// instead of rejecting every request.
52+
func FromEnv() (Config, error) {
53+
enabled, err := boolEnv("OAUTH_ENABLED", false)
54+
if err != nil {
55+
return Config{}, err
56+
}
57+
if !enabled {
58+
return Config{}, nil
59+
}
60+
authServer, err := requiredURLEnv("OAUTH_AUTHORIZATION_SERVER_URL")
61+
if err != nil {
62+
return Config{}, err
63+
}
64+
resource, err := requiredURLEnv("OAUTH_CANONICAL_RESOURCE_URI")
65+
if err != nil {
66+
return Config{}, err
67+
}
68+
passthrough, err := boolEnv("OAUTH_API_KEY_PASSTHROUGH", true)
69+
if err != nil {
70+
return Config{}, err
71+
}
72+
return Config{
73+
Enabled: true,
74+
AuthorizationServerURL: strings.TrimRight(authServer, "/"),
75+
CanonicalResourceURI: canonicalResourceURI(resource),
76+
IntrospectionServiceToken: os.Getenv("OAUTH_INTROSPECTION_SERVICE_TOKEN"),
77+
APIKeyPassthrough: passthrough,
78+
}, nil
79+
}
80+
81+
// MetadataPaths returns the request paths that serve the protected-resource
82+
// metadata document: the RFC 9728 §3.1 path-insertion form derived from the
83+
// resource URI (resource https://host/mcp → /.well-known/oauth-protected-resource/mcp)
84+
// plus the root form for clients that probe it directly.
85+
func (c Config) MetadataPaths() []string {
86+
if p := metadataPath(c.CanonicalResourceURI); p != wellKnownPath {
87+
return []string{p, wellKnownPath}
88+
}
89+
return []string{wellKnownPath}
90+
}
91+
92+
// MetadataURL returns the absolute URL of the protected-resource metadata
93+
// document, sent in WWW-Authenticate challenges so clients can discover the
94+
// authorization server.
95+
func (c Config) MetadataURL() string {
96+
u, err := url.Parse(c.CanonicalResourceURI)
97+
if err != nil || u.Scheme == "" || u.Host == "" {
98+
return strings.TrimRight(c.CanonicalResourceURI, "/") + wellKnownPath
99+
}
100+
return u.Scheme + "://" + u.Host + metadataPath(c.CanonicalResourceURI)
101+
}
102+
103+
// metadataPath is the RFC 9728 §3.1 well-known path for a resource URI: the
104+
// well-known segment inserted between the host and the resource's path.
105+
func metadataPath(canonicalResource string) string {
106+
u, err := url.Parse(canonicalResource)
107+
if err != nil || u.EscapedPath() == "" || u.EscapedPath() == "/" {
108+
return wellKnownPath
109+
}
110+
return wellKnownPath + u.EscapedPath()
111+
}
112+
113+
// canonicalResourceURI normalizes an RFC 8707 resource indicator (RFC 3986 §6)
114+
// so equivalent spellings compare equal: scheme/host lowercased, default ports
115+
// dropped, bare "/" path treated as none; path, query, and fragment are
116+
// otherwise preserved. It mirrors the authorization server's audience
117+
// normalization so matching can't be defeated by spelling on either side.
118+
// Unparseable values are returned unchanged (they fail matching anyway).
119+
func canonicalResourceURI(raw string) string {
120+
u, err := url.Parse(raw)
121+
if err != nil || u.Scheme == "" || u.Host == "" {
122+
return raw
123+
}
124+
u.Scheme = strings.ToLower(u.Scheme)
125+
u.Host = strings.ToLower(u.Host)
126+
if (u.Scheme == "https" && u.Port() == "443") || (u.Scheme == "http" && u.Port() == "80") {
127+
// Hostname() strips the brackets from IPv6 literals ("[::1]" → "::1"),
128+
// so re-bracket before assigning back to Host.
129+
host := u.Hostname()
130+
if strings.Contains(host, ":") {
131+
host = "[" + host + "]"
132+
}
133+
u.Host = host
134+
}
135+
if u.Path == "/" {
136+
u.Path = ""
137+
}
138+
return u.String()
139+
}
140+
141+
func requiredURLEnv(name string) (string, error) {
142+
v := strings.TrimSpace(os.Getenv(name))
143+
if v == "" {
144+
return "", fmt.Errorf("%s is required when OAUTH_ENABLED is set", name)
145+
}
146+
u, err := url.Parse(v)
147+
if err != nil {
148+
return "", fmt.Errorf("%s: %w", name, err)
149+
}
150+
if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
151+
return "", fmt.Errorf("%s must be an absolute http(s) URL, got %q", name, v)
152+
}
153+
// A query would corrupt URLs derived by concatenation (the introspection
154+
// endpoint, the RFC 9728 path-insertion metadata URL), and RFC 8707
155+
// forbids fragments in resource indicators. Neither identifies anything
156+
// here; reject rather than carry them inconsistently. Checked on the raw
157+
// string so a bare trailing "?" or "#" (empty query/fragment, which
158+
// url.Parse reports as no query/fragment) can't slip through.
159+
if strings.ContainsAny(v, "?#") {
160+
return "", fmt.Errorf("%s must not contain a query or fragment, got %q", name, v)
161+
}
162+
// These values are embedded in the WWW-Authenticate header, whose
163+
// quoted-string grammar can't carry non-ASCII; require the encoded form
164+
// (punycode hosts, percent-encoded paths) up front.
165+
for j := 0; j < len(v); j++ {
166+
if v[j] <= 0x20 || v[j] > 0x7e {
167+
return "", fmt.Errorf("%s must be printable ASCII (punycode / percent-encoded), got %q", name, v)
168+
}
169+
}
170+
return v, nil
171+
}
172+
173+
// boolEnv parses a boolean environment variable, returning def when unset.
174+
// Unrecognized values are an error rather than silently false — for these
175+
// flags a typo must not flip a security posture unnoticed.
176+
func boolEnv(name string, def bool) (bool, error) {
177+
v := strings.ToLower(strings.TrimSpace(os.Getenv(name)))
178+
switch v {
179+
case "":
180+
return def, nil
181+
case "true", "1", "yes":
182+
return true, nil
183+
case "false", "0", "no":
184+
return false, nil
185+
default:
186+
return false, fmt.Errorf("%s must be true or false (also accepted: 1/0, yes/no), got %q", name, os.Getenv(name))
187+
}
188+
}

0 commit comments

Comments
 (0)