[rust-server] Restrict from_headers matches to the intended auth scheme - #24607
Conversation
|
This fixes the issue here, but the untyped from_headers still seems like a footgun for anyone hand-rolling swagger-rs 7.x. Worth an issue against Metaswitch/swagger-rs too? |
|
@wing328 circle CI looks unrelated. Are there issues occurring on that CI system? |
|
Two potential testing gaps to consider. There's no fixture proving Basic+Bearer coexist without swallowing each other (only OAuth+Basic is tested), and the assertions are string-exact rather than runtime tested. A request-level test through AddContext::call would prove the actual fallthrough behavior as |
|
for circleci failures, please ignore those for the time being |
Add a fixture pairing HTTP Basic with Bearer (the untested isBasicBearer section) ahead of an apiKey scheme, plus a runtime test through AddContext::call in the petstore sample.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
5 issues found across 31 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/server/petstore/rust-server/output/overlapping-auth-schemes/docs/default_api.md">
<violation number="1" location="samples/server/petstore/rust-server/output/overlapping-auth-schemes/docs/default_api.md:7">
P3: The generated documentation shows the signature as `pingGet(ctx, ctx, ctx, )` (one ctx per auth scheme), while the actual generated method `ping_get(&self, context: &C)` takes a single context argument. The `ctx` repetition comes from `api_doc.mustache` iterating `{{#authMethods}}`; with 3 auth schemes the doc misleads readers about the callable signature. Consider rendering a single context parameter in the doc.</violation>
</file>
<file name="samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/client/main.rs">
<violation number="1" location="samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/client/main.rs:42">
P2: The documented `--https` option is not a boolean flag: without `ArgAction::SetTrue`, Clap treats it as a value-taking argument and rejects a bare `--https`; declaring it as a boolean flag would make the HTTPS example usable.</violation>
<violation number="2" location="samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/client/main.rs:83">
P2: The generated client panics on every normal invocation because `port` is parsed as a `String` but retrieved as `u16`; retrieving it as `String` (or adding a `u16` value parser to the argument) would let the documented client command run.</violation>
</file>
<file name="samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/server/server_auth.rs">
<violation number="1" location="samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/server/server_auth.rs:40">
P2: Bearer validation accepts the algorithm declared by the untrusted JWT header instead of enforcing the HS512 algorithm used by this example. A token signed with another supported HMAC algorithm using the shared key will therefore be accepted; using a fixed algorithm or configured allowlist would preserve the intended validation policy.</violation>
<violation number="2" location="samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/server/server_auth.rs:88">
P2: With the documented debug logging enabled, the example writes complete authentication credentials to logs. Logging only scheme metadata or a redacted identifier would avoid turning normal request tracing into a credential-leak path.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| .required(true) | ||
| .index(1)) | ||
| .arg(Arg::new("https") | ||
| .long("https") |
There was a problem hiding this comment.
P2: The documented --https option is not a boolean flag: without ArgAction::SetTrue, Clap treats it as a value-taking argument and rejects a bare --https; declaring it as a boolean flag would make the HTTPS example usable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/client/main.rs, line 42:
<comment>The documented `--https` option is not a boolean flag: without `ArgAction::SetTrue`, Clap treats it as a value-taking argument and rejects a bare `--https`; declaring it as a boolean flag would make the HTTPS example usable.</comment>
<file context>
@@ -0,0 +1,129 @@
+ .required(true)
+ .index(1))
+ .arg(Arg::new("https")
+ .long("https")
+ .help("Whether to use HTTPS or not"))
+ .arg(Arg::new("host")
</file context>
There was a problem hiding this comment.
This is a cli.mustache bug in every sample - I think a fix should be done in a separate PR
| let base_url = format!("{}://{}:{}", | ||
| if is_https { "https" } else { "http" }, | ||
| matches.get_one::<String>("host").unwrap(), | ||
| matches.get_one::<u16>("port").unwrap()); |
There was a problem hiding this comment.
P2: The generated client panics on every normal invocation because port is parsed as a String but retrieved as u16; retrieving it as String (or adding a u16 value parser to the argument) would let the documented client command run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/client/main.rs, line 83:
<comment>The generated client panics on every normal invocation because `port` is parsed as a `String` but retrieved as `u16`; retrieving it as `String` (or adding a `u16` value parser to the argument) would let the documented client command run.</comment>
<file context>
@@ -0,0 +1,129 @@
+ let base_url = format!("{}://{}:{}",
+ if is_https { "https" } else { "http" },
+ matches.get_one::<String>("host").unwrap(),
+ matches.get_one::<u16>("port").unwrap());
+
+ let context: ClientContext =
</file context>
| matches.get_one::<u16>("port").unwrap()); | |
| matches.get_one::<String>("port").unwrap()); |
There was a problem hiding this comment.
This is a cli.mustache bug in every sample - I think a fix should be done in a separate PR
| // See https://github.com/Keats/jsonwebtoken for more information. | ||
| let header = decode_header(token)?; | ||
| let validation = { | ||
| let mut validation = Validation::new(header.alg); |
There was a problem hiding this comment.
P2: Bearer validation accepts the algorithm declared by the untrusted JWT header instead of enforcing the HS512 algorithm used by this example. A token signed with another supported HMAC algorithm using the shared key will therefore be accepted; using a fixed algorithm or configured allowlist would preserve the intended validation policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/server/server_auth.rs, line 40:
<comment>Bearer validation accepts the algorithm declared by the untrusted JWT header instead of enforcing the HS512 algorithm used by this example. A token signed with another supported HMAC algorithm using the shared key will therefore be accepted; using a fixed algorithm or configured allowlist would preserve the intended validation policy.</comment>
<file context>
@@ -0,0 +1,126 @@
+ // See https://github.com/Keats/jsonwebtoken for more information.
+ let header = decode_header(token)?;
+ let validation = {
+ let mut validation = Validation::new(header.alg);
+ validation.set_audience(&["org.acme.Resource_Server"]);
+ validation.validate_exp = true;
</file context>
There was a problem hiding this comment.
This is an example_server_auth.mustache bug that appears in every sample - I think a fix should be done in a separate PR
|
|
||
| /// Implementation of the method to map a Bearer-token to an Authorization | ||
| fn bearer_authorization(&self, bearer: &Bearer) -> Result<Authorization, ApiError> { | ||
| debug!("\tAuthorizationApi: Received Bearer-token, {bearer:#?}"); |
There was a problem hiding this comment.
P2: With the documented debug logging enabled, the example writes complete authentication credentials to logs. Logging only scheme metadata or a redacted identifier would avoid turning normal request tracing into a credential-leak path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/rust-server/output/overlapping-auth-schemes/examples/server/server_auth.rs, line 88:
<comment>With the documented debug logging enabled, the example writes complete authentication credentials to logs. Logging only scheme metadata or a redacted identifier would avoid turning normal request tracing into a credential-leak path.</comment>
<file context>
@@ -0,0 +1,126 @@
+
+ /// Implementation of the method to map a Bearer-token to an Authorization
+ fn bearer_authorization(&self, bearer: &Bearer) -> Result<Authorization, ApiError> {
+ debug!("\tAuthorizationApi: Received Bearer-token, {bearer:#?}");
+
+ match extract_token_data(&bearer.token(), b"secret") {
</file context>
There was a problem hiding this comment.
This is an example_server_auth.mustache bug that appears in every sample - I think a fix should be done in a separate PR
There was a problem hiding this comment.
Sounds good. I think we do want this addressing pretty rapidly given it results in logging credentials.
|
|
||
| Method | HTTP request | Description | ||
| ------------- | ------------- | ------------- | ||
| **pingGet**](default_api.md#pingGet) | **GET** /ping | |
There was a problem hiding this comment.
P3: The generated documentation shows the signature as pingGet(ctx, ctx, ctx, ) (one ctx per auth scheme), while the actual generated method ping_get(&self, context: &C) takes a single context argument. The ctx repetition comes from api_doc.mustache iterating {{#authMethods}}; with 3 auth schemes the doc misleads readers about the callable signature. Consider rendering a single context parameter in the doc.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/server/petstore/rust-server/output/overlapping-auth-schemes/docs/default_api.md, line 7:
<comment>The generated documentation shows the signature as `pingGet(ctx, ctx, ctx, )` (one ctx per auth scheme), while the actual generated method `ping_get(&self, context: &C)` takes a single context argument. The `ctx` repetition comes from `api_doc.mustache` iterating `{{#authMethods}}`; with 3 auth schemes the doc misleads readers about the callable signature. Consider rendering a single context parameter in the doc.</comment>
<file context>
@@ -0,0 +1,31 @@
+
+Method | HTTP request | Description
+------------- | ------------- | -------------
+**pingGet**](default_api.md#pingGet) | **GET** /ping |
+
+
</file context>
There was a problem hiding this comment.
I think this is an api_doc.mustache bug that this 3 scheme sample is the first to expose, would again suggest a separate PR to fix
dsteeley
left a comment
There was a problem hiding this comment.
LGTM, thanks for changes.
Fixes #24095
Problem
The
rust-servercontext.mustachetemplate emits one block per security scheme, each doing an earlyreturnon match. TheAuthorization-header blocks callswagger::auth::from_headers(headers)and bind the result unconditionally:Since swagger-rs 7,
from_headersis no longer scheme-typed. It returnsOption<AuthData>and matches either scheme (swagger-7.0.1src/auth.rs:216):So a
Basic-only block swallowsBearerrequests (and vice versa) and returns immediately, making every later security scheme block unreachable — including in-headerapiKeyblocks.Impact: for any spec where an
Authorization-header scheme precedes an in-headerapiKeyscheme, a request carryingAuthorization: Bearer …is authorized via the wrong scheme and the API key / client certificate is never evaluated. Where the mismatched path has a permissive fallback (e.g.AllowAllAuthenticatorwhen OAuth is unconfigured) this is a silent authorization bypass. This is a regression from the swagger 5/6 typedfrom_headers::<Basic>(headers)form.Fix
Option B from the bug ticket — restrict each block's pattern to the variant it was generated for, so a non-matching header falls through to the next block instead of being consumed:
AuthDatais already imported by the template, so no new imports are needed, and no change to swagger-rs is required — this fixes every 7.x consumer immediately.Note this fixes
isOAuthandisBasicBeareras well as the reportedisBasicBasic: aBasicheader could equally be swallowed by a Bearer/OAuth block.Behaviour change
Worth calling out explicitly: on an API declaring only Basic auth, a request with
Authorization: Bearer …previously reached the authenticator asAuthData::Bearer; it now falls through tocontext.push(None::<AuthData>)and is treated as unauthenticated (and symmetrically for aBasicheader on a Bearer/OAuth-only API). That is the intended security fix, but it is a semantic change for anyone relying on the permissive behaviour. Happy to retarget if maintainers consider this breaking.Not addressed (pre-existing, out of scope): an in-header
apiKeyscheme whose header name is literallyAuthorizationwould still collide.Changes
modules/openapi-generator/src/main/resources/rust-server/context.mustache— scheme-restricted patterns for theisBasicBasic,isBasicBearerandisOAuthblocks.RustServerCodegenTest.testAuthSchemeBlocksOnlyMatchTheirOwnScheme— new regression test asserting both the correct forms and, viaassertFileNotContains, the absence of the unrestricted forms; also asserts the in-headerapiKeyblock is still generated.openapi-v3,petstore-with-fake-endpoints-models-for-testing,ping-bearer-auth.Testing
./mvnw clean package— BUILD SUCCESS, full test suite green../bin/generate-samples.sh bin/configs/rust-server*.yaml— 7/7 generators succeeded, no sample drift.mvn -pl modules/openapi-generator -am test -Dtest=RustServerCodegenTest— 7/7 pass.context.mustachealone makes it fail withdoes not contain line [if let Some(bearer @ AuthData::Bearer(..)) = …].cargo check --all-featureson the regeneratedpetstore-with-fake-endpoints-models-for-testingsample compiles clean.rust-serversample retains an unrestricted= swagger::auth::from_headers(headers), and thatcontext.mustacheis the only template referencingfrom_headers.CC @frol @farcaller @richardwhiuk @paladinzh @jacob-pro @dsteeley
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
Summary by cubic
Restricts
rust-serverauth handling to match only the intended scheme (Basic vs Bearer) so cross-scheme matches no longer short-circuit later auth blocks like headerapiKey(fixes #24095). Also updates the client auth match to allow an unreachable catch-all arm so builds with-D warningsremain clean.Bug Fixes
context.mustache:AuthData::Basic(..)for Basic andAuthData::Bearer(..)for Bearer/OAuth.apiKeyreachable.#[allow(unreachable_patterns)]inclient-operation.mustache.apiKeybetween them; runtimeAddContexttests validate scheme precedence andapiKeyreachability.overlapping-auth-schemes; drop imports left unused by the scheme restriction.Migration
Authorizationheaders on single-scheme APIs now fall through as unauthenticated.swagger-rs7.x generated servers.Written for commit e4625a7. Summary will update on new commits.