Unify HTTP servers #32409

alex-hunt-materialize · 2025-05-05T11:50:13Z

Unify HTTP servers, and mostly unify their authentication.

We still retain the x-materialize-user header for now, but there is a change in behavior that we no longer specify a default value for it if it is not passed.

Motivation

This PR refactors existing code.
This is an attempt to split up Mz system password support #32245, as that PR is too big and complicated for review, and it conflated a few things which made isolating test failures difficult.

The x-materialize-user header is only used by Teleport, and it always uses the mz_support user, not the mz_system user we default to. This header doesn't even need to exist, as we could just have Teleport use basic auth without a password. Removing the default value for this header should allow us to migrate Teleport to using a basic auth header instead.

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.

def-

I have pushed a commit that fixes the remaining test failures: d9a4779
Also triggered a nightly run: https://buildkite.com/materialize/nightly/builds/11971

alex-hunt-materialize · 2025-05-07T09:07:40Z

src/environmentd/src/lib.rs

+        let (authenticator_frontegg_tx, authenticator_frontegg_rx) = oneshot::channel();
+        let authenticator_frontegg_rx = authenticator_frontegg_rx.shared();
+        let (authenticator_none_tx, authenticator_none_rx) = oneshot::channel();
+        let authenticator_none_rx = authenticator_none_rx.shared();


This is partially in preparation for Authenticator::Password(AdapterClient) support coming in the next PR. While we can immediately create the Authenticator::Frontegg and Authenticator::None now, we won't be able to immediately create Authenticator::Password, since we want to launch this HTTP server before the AdapterClient is instantiated.

Interesting, why won't we be able to construct a Password Authenticator here?

We can't construct an Authenticator::Password because it contains an AdapterClient, which isn't ready yet. We want the listener to be available for metrics endpoints during the startup, so we can't wait to start the HTTP server until after the AdapterClient is ready.

Gotcha. I remember having to do something similar with pgwire. Perhaps in the future we should split out the metrics server if it's lifecycle is separate.

Seen in https://buildkite.com/materialize/test/builds/103189

DAlperin

Looking good! Left a few notes

DAlperin · 2025-05-07T19:03:08Z

src/authenticator/src/lib.rs

+#[derive(Debug, Clone)]
+pub enum Authenticator {
+    Frontegg(FronteggAuthenticator),
+    None,


What is the meaning of None here?

It means no authentication, like our internal port currently behaves (with the exception of removal of the default value for x-materialize-user header).

DAlperin · 2025-05-07T19:04:28Z

src/environmentd/src/http.rs

+            if let AuthenticatorKind::None = authenticator_kind {
+                ws_router = ws_router.layer(middleware::from_fn(x_materialize_user_header_auth));
+            }


If AuthenticatorKind::None we want to let anyone auth as mz_system? (We might very well, I just want to be careful here)

Yes, this is maintaining the current behavior.

Even without this, if they set the basic auth user to mz_system that would also allow them in with AuthenticatorKind::None.

DAlperin · 2025-05-07T19:05:44Z

src/environmentd/src/http.rs

+        match authenticator_kind {
+            AuthenticatorKind::None => {
+                base_router = base_router.layer(middleware::from_fn(x_materialize_user_header_auth))
+            }
+            _ => {}
+        }


We manually place this above on specific routepath. If we generally apply it here, do we need the specific cases?

We want it on these two sets of routes. We don't want it on all routes, though. For example, the webhook router does not have it.

src/environmentd/src/http.rs

DAlperin · 2025-05-07T19:22:47Z

test/legacy-upgrade/check-from-v0.83.0-role.td

@@ -11,6 +11,7 @@ $ skip-if
 SELECT mz_version_num() < 8300;

 > SELECT name FROM mz_roles;
+anonymous_http_user


where does this user come from?

This is fixing an inconsistency in our prior behavior. The anonymous_http_user comes from the DEFAULT_HTTP_USER constant, which is used with Authenticator::None when no x-materialize-user or basic auth header is provided. Previously, the x-materialize-user handling was applied first and defaulted to mz_system user. I have removed this x-materialize-user default, so it instead gets the basic auth default of DEFAULT_HTTP_USER.

I hope to follow up after this PR is deployed with changing teleport to use basic auth instead of the x-materialize-user header, so that we can later entirely remove the x-materialize-user code path.

src/server-core/src/listeners.rs

DAlperin · 2025-05-07T19:24:31Z

src/environmentd/src/lib.rs

+        let (authenticator_frontegg_tx, authenticator_frontegg_rx) = oneshot::channel();
+        let authenticator_frontegg_rx = authenticator_frontegg_rx.shared();
+        let (authenticator_none_tx, authenticator_none_rx) = oneshot::channel();
+        let authenticator_none_rx = authenticator_none_rx.shared();


Interesting, why won't we be able to construct a Password Authenticator here?

DAlperin

Looks good! Thanks for the changes

DAlperin · 2025-05-08T18:01:29Z

src/environmentd/src/http.rs

+async fn x_materialize_user_header_auth(mut req: Request, next: Next) -> impl IntoResponse {
+    // TODO integrate this into the single auth flow
+    // or migrate teleport to basic auth.
+    if let Some(username) = req.headers().get("x-materialize-user").map(|h| h.to_str()) {


Not needed in this PR but in the follow up if we are hanging onto x-materialize-user lets throw it in a constant

DAlperin · 2025-05-08T18:02:57Z

src/environmentd/src/lib.rs

+        let (authenticator_frontegg_tx, authenticator_frontegg_rx) = oneshot::channel();
+        let authenticator_frontegg_rx = authenticator_frontegg_rx.shared();
+        let (authenticator_none_tx, authenticator_none_rx) = oneshot::channel();
+        let authenticator_none_rx = authenticator_none_rx.shared();


Gotcha. I remember having to do something similar with pgwire. Perhaps in the future we should split out the metrics server if it's lifecycle is separate.

alex-hunt-materialize force-pushed the unify_http_server branch 4 times, most recently from a3dd5eb to 2b01e80 Compare May 6, 2025 13:44

def- reviewed May 7, 2025

View reviewed changes

def- force-pushed the unify_http_server branch 2 times, most recently from 7d5dd69 to bd359e7 Compare May 7, 2025 05:55

alex-hunt-materialize commented May 7, 2025

View reviewed changes

alex-hunt-materialize marked this pull request as ready for review May 7, 2025 09:09

alex-hunt-materialize requested review from ParkMyCar and a team as code owners May 7, 2025 09:09

alex-hunt-materialize force-pushed the unify_http_server branch 3 times, most recently from 2712413 to 86384cb Compare May 7, 2025 14:03

alex-hunt-materialize and others added 3 commits May 7, 2025 15:14

Unify HTTP servers

a1bf91c

Remove default user for x-materialize-user header.

43c49b5

Fix remaining test failures

267b2af

Seen in https://buildkite.com/materialize/test/builds/103189

alex-hunt-materialize force-pushed the unify_http_server branch from 86384cb to 267b2af Compare May 7, 2025 14:14

DAlperin reviewed May 7, 2025

View reviewed changes

Address review comments.

e222f6f

alex-hunt-materialize force-pushed the unify_http_server branch from f2834d9 to e222f6f Compare May 8, 2025 14:44

alex-hunt-materialize requested a review from DAlperin May 8, 2025 15:46

DAlperin approved these changes May 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify HTTP servers #32409

Unify HTTP servers #32409

alex-hunt-materialize commented May 5, 2025 •

edited

Loading

def- left a comment •

edited

Loading

alex-hunt-materialize May 7, 2025

DAlperin May 7, 2025

alex-hunt-materialize May 8, 2025

DAlperin May 8, 2025

DAlperin left a comment

DAlperin May 7, 2025

alex-hunt-materialize May 8, 2025 •

edited

Loading

DAlperin May 7, 2025

alex-hunt-materialize May 8, 2025 •

edited

Loading

DAlperin May 7, 2025

alex-hunt-materialize May 8, 2025

DAlperin May 7, 2025

alex-hunt-materialize May 8, 2025

DAlperin May 7, 2025

DAlperin left a comment

DAlperin May 8, 2025

DAlperin May 8, 2025

Unify HTTP servers #32409

Are you sure you want to change the base?

Unify HTTP servers #32409

Conversation

alex-hunt-materialize commented May 5, 2025 • edited Loading

Motivation

Tips for reviewer

Checklist

def- left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DAlperin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-hunt-materialize May 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-hunt-materialize May 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DAlperin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-hunt-materialize commented May 5, 2025 •

edited

Loading

def- left a comment •

edited

Loading

alex-hunt-materialize May 8, 2025 •

edited

Loading

alex-hunt-materialize May 8, 2025 •

edited

Loading