SQL-81: Improve OIDC error messages by SangJunBak · Pull Request #34891 · MaterializeInc/materialize

SangJunBak · 2026-02-03T00:09:31Z

see commit messages for details.

Motivation

Fixes https://linear.app/materializeinc/issue/SQL-81/create-better-error-messages-on-authentication-error

Tips for reviewer

Checklist

This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

Before we'd assume `issuer` ends with '/', but this is actually incorrect given JWTs return an audience with no trailing '/' and validate_aud would fail later.

The idea is propagate these errors back up to the user such that they can debug more easily. Currently we only surface the "Invalid password" error for pgwire clients.

teskje · 2026-02-24T11:56:05Z

src/authenticator/src/oidc.rs

-    JwksFetchFailed(String),
+    InvalidIssuerUrl(
+        /// Issuer URL.
+        String,


Once you start documenting the fields, this seems like a sign that their meaning isn't obvious and you should perhaps use a struct variant rather than a tuple variant, i.e.:

InvalidIssuerUrl { url: String, }

Although... in this case I'd actually say it is pretty obvious. The string is just the invalid issuer URL, which is exactly the variant name. So maybe just remove the comment here. But some of the below variants should be struct variants I think.

src/authenticator/src/oidc.rs

teskje · 2026-02-24T12:17:12Z

src/authenticator/src/oidc.rs

+    pub fn detail(&self) -> Option<String> {
+        match self {
+            OidcError::InvalidIssuerUrl(issuer) => {
+                Some(format!("Could not parse \"{}\" as a URL.", issuer))


Nit: Consider inlining for better readability

Suggested change

Some(format!("Could not parse \"{}\" as a URL.", issuer))

Some(format!("Could not parse \"{issuer}\" as a URL."))

teskje · 2026-02-24T12:21:57Z

src/authenticator/src/oidc.rs

 impl GenericOidcAuthenticatorInner {
    async fn fetch_jwks_uri(&self, issuer: &str) -> Result<String, OidcError> {
-        let openid_config_url = Url::parse(issuer)
+        let openid_config_url = Url::parse(&format!("{}/", issuer))


Why is this done? It seems like appending a / either doesn't change the semantics of a URL (e.g. https://foo.com -> https://foo.com/) or changes the semantics in unexpected ways (e.g. https://foo.com/hello.html -> https://foo.com/hello.html/).

It changes the semantics of the URL! We need to append an / to issuer such that url.join below appends .well-known/openid-configuration. If we don't have the trailing slash, we replace the last path segment instead of append onto it.

For example, the URL we want to create is https://accounts.google.com/.well-known/openid-configuration where the issuer URL is https://accounts.google.com.

We also want to make sure the issuer URL is https://accounts.google.com and not https://accounts.google.com/ otherwise the issuer validation later on fails

I was thinking we could all change it to this:

let mut openid_config_url = Url::parse(issuer).map_err(|_| OidcError::InvalidIssuerUrl(issuer.to_string()))?; { let mut segments = openid_config_url .path_segments_mut() .map_err(|_| OidcError::InvalidIssuerUrl(issuer.to_string()))?; segments.push(".well-known"); segments.push("openid-configuration"); }

Hm, at least for you example, this works fine without appending a /:

println!("{}", url::Url::parse("https://accounts.google.com") .unwrap() .join(".well-known/openid-configuration") .unwrap() );

... prints https://accounts.google.com/.well-known/openid-configuration.

Appending the / makes a difference when the issuer URL has a path. However, it's not clear to me what the correct behavior is if somebody supplies an issuer URL like https://accounts.google.com/index.html. Just converting that to https://accounts.google.com/index.html/.well-known/openid-configuration seems guaranteed to be wrong at least?

Do you know if path's are valid in issuer URLs? If not, we could just return an error if one was supplied.

Appending the / makes a difference when the issuer URL has a path.

Ah I think the issuer URL can have a path. The motivation for this fix is when testing via Okta, https://dev-123456.okta.com/oauth2/default was turning into https://dev-123456.okta.com/oauth2/.well-known/openid-configuration.

Do you know if path's are valid in issuer URLs?

Paths are valid! Through discussion, I'm thinking we should handle the case where we get https://accounts.google.com or https://accounts.google.com/ as the issuer. #34891 (comment) should achieve this

Through discussion, I'm thinking we should handle the case where we get https://accounts.google.com or https://accounts.google.com/ as the issuer.

Those seem like easy cases though. Just calling Url::parse on the given string directly works for both.

The hard case is https://dev-123456.okta.com/oauth2/default because you can't know whether default is a file or a directory. The solution the url crate takes is to require a trailing / to denote a directory, but seems like auth providers don't follow that convention?

Looking at the OpenID spec, it has this:

OpenID Providers supporting Discovery MUST make a JSON document available at the path formed by concatenating the string /.well-known/openid-configuration to the Issuer.

Which I think means your current implementation is correct! If you put in https://accounts.google.com/index.html, that's an invalid issuer URL and we will give you an error about not being able to check the discovery file, which seems fine.

One suggestion I have would be to simplify by just following the above spec to a tee:

Url::parse(&format!("{issuer}/.well-known/openid-configuration"))

teskje · 2026-02-24T12:33:20Z

src/authenticator/src/oidc.rs

+    FetchFromProviderFailed(
+        /// Fetch URL.
+        String,
+        // HTTP status code.
+        Option<StatusCode>,
+    ),


Istm like this error is not really useful if we don't have a status code. It will just say "failed to fetch from {url}", but I think we should say at least something like "timed out" or "failed to decode". So maybe make the second field an error string instead?

teskje · 2026-02-24T12:44:15Z

src/environmentd/tests/auth.rs

Seems like here we should also test the DETAIL, given that this contains most of the interesting information about the error?

teskje · 2026-02-24T12:46:09Z

src/authenticator/src/oidc.rs

+                    if let Some(audience) = &audience {
+                        OidcError::InvalidAudience(audience.clone())
+                    } else {
+                        debug_assert!(


Use soft_panic_or_log instead!

No need to put in an extra false to make it panic

Will make CI fail, if hit

Will log an error in production

Ah good call!

SangJunBak · 2026-02-24T21:16:55Z

@teskje Addressed all your feedback. TFTR!

teskje

LGTM!

teskje · 2026-02-25T14:48:20Z

src/authenticator/src/oidc.rs

                warn!(
-                    "Audience validation skipped. It is discouraged
-                    to skip audience validation since it allows
-                    anyone with a JWT issued by the same issuer
-                    to authenticate."
+                    "{}",
+                    concat!(
+                        "Audience validation skipped. It is discouraged ",
+                        "to skip audience validation since it allows anyone ",
+                        "with a JWT issued by the same issuer to authenticate."
+                    )
                );


This should work:

warn!( "Audience validation skipped. It is discouraged \ to skip audience validation since it allows \ anyone with a JWT issued by the same issuer \ to authenticate." );

- inline errors - Add test for fetch error - simplify url parsing - Replace assert with soft panic - Structify error enum bodies - Test on error detail

SangJunBak force-pushed the jun/improve-error-messages branch 2 times, most recently from 6ccc11c to 0cda15c Compare February 9, 2026 20:16

SangJunBak force-pushed the jun/improve-error-messages branch 3 times, most recently from ad906bf to a681dd4 Compare February 20, 2026 21:15

adapter: Remove trailing '/' expectation of issuer

df5b46f

Before we'd assume `issuer` ends with '/', but this is actually incorrect given JWTs return an audience with no trailing '/' and validate_aud would fail later.

SangJunBak force-pushed the jun/improve-error-messages branch 2 times, most recently from 4a44537 to 3e4247e Compare February 23, 2026 16:05

SangJunBak changed the title ~~Oidc auth: Improve error messages~~ SQL-81: Improve OIDC error messages Feb 23, 2026

SangJunBak requested review from teskje February 23, 2026 16:09

SangJunBak marked this pull request as ready for review February 23, 2026 16:09

SangJunBak requested a review from a team as a code owner February 23, 2026 16:09

SangJunBak requested a review from mtabebe February 23, 2026 16:09

SangJunBak added 2 commits February 23, 2026 12:20

adapter: Improve error messages in OIDC authenticator

b7ae8ba

The idea is propagate these errors back up to the user such that they can debug more easily. Currently we only surface the "Invalid password" error for pgwire clients.

Fix tests

32edf9e

SangJunBak force-pushed the jun/improve-error-messages branch from 3e4247e to 32edf9e Compare February 23, 2026 17:20

SangJunBak mentioned this pull request Feb 24, 2026

SQL-65: Add password authentication to HTTP/WS server for OIDC #35185

Open

SangJunBak removed the request for review from mtabebe February 24, 2026 09:34

teskje reviewed Feb 24, 2026

View reviewed changes

teskje approved these changes Feb 25, 2026

View reviewed changes

adapter: Address Jan's feedback

98de882

- inline errors - Add test for fetch error - simplify url parsing - Replace assert with soft panic - Structify error enum bodies - Test on error detail

SangJunBak force-pushed the jun/improve-error-messages branch from 58262cf to 98de882 Compare February 25, 2026 15:07

SangJunBak enabled auto-merge (squash) February 25, 2026 15:07

SangJunBak merged commit 15c4357 into MaterializeInc:main Feb 25, 2026
135 checks passed

	Some(format!("Could not parse \"{}\" as a URL.", issuer))
	Some(format!("Could not parse \"{issuer}\" as a URL."))

Conversation

SangJunBak commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Tips for reviewer

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SangJunBak Feb 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SangJunBak commented Feb 24, 2026

Uh oh!

teskje left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SangJunBak commented Feb 3, 2026 •

edited

Loading

SangJunBak Feb 24, 2026 •

edited

Loading