adapter: Prevent login to mz_system externally #14093

jkosh44 · 2022-08-04T23:05:17Z

mz_system is the system user that is responsible for executing commands
that only an administrator should be doing. This commit prevents
logging into the mz_system user from an external port. You can only
login to the mz_system user via internal ports.

Works towards resolving: https://github.com/MaterializeInc/cloud/issues/3292

Motivation

This PR adds a known-desirable feature.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way) and therefore is tagged with a T-protobuf label.
This PR includes the following user-facing behavior changes:
- You can no longer use the mz_system role with an external port.

that only an administrator should be doing. This commit prevents logging into the mz_system user from an external port. You can only login to the mz_system user via internal ports. Works towards resolving: MaterializeInc/cloud#3292

src/adapter/src/client.rs

maddyblue · 2022-08-11T19:34:00Z

src/adapter/src/client.rs

@@ -152,6 +164,8 @@ impl ConnClient {
        session: Session,
        create_user_if_not_exists: bool,
    ) -> Result<(SessionClient, StartupResponse), AdapterError> {
+        self.validate_user(session.user())?;


Should this go wherever the frontegg login stuff also happens? I guess not because it needs the client type. But I'm not thrilled that "can i login?" is now in multiple places.

I'm not very familiar with the Frontegg code, but I agree with your point. I'll do some digging and see if I can integrate it with the Frontegg stuff.

maddyblue · 2022-08-11T19:41:53Z

src/adapter/src/catalog.rs

@@ -95,7 +95,8 @@ pub mod builtin;
 pub mod storage;

 pub const SYSTEM_CONN_ID: ConnectionId = 0;
-const SYSTEM_USER: &str = "mz_system";
+pub const SYSTEM_USER: &str = "mz_system";
+pub const HTTP_DEFAULT_USER: &str = "mz_http_default_user";


Very slight preference for this to be mz_http_internal_user because default suggests that it's a fallback and is regularly used on the external http connection.

This is actually only used on the external http connection as a fallback if there's no user authenticated. So maybe it should be mz_http_default_external_user?

src/adapter/src/client.rs

src/environmentd/src/lib.rs

maddyblue · 2022-08-11T20:03:56Z

src/environmentd/src/lib.rs

@@ -330,7 +331,7 @@ pub async fn serve(config: Config) -> Result<Server, anyhow::Error> {
            let http_server = http::Server::new(http::Config {
                tls: http_tls,
                frontegg: config.frontegg,
-                adapter_client,
+                adapter_client: external_adapter_client,


Where does the internal http server get its internal adapter client?

This is a bit perplexing to me. The internal http port uses the http::InternalServer struct (initialized above all of this on line 204) which doesn't use an adapter client and has the following endpoints:

/metrics

/api/livez

/api/opentelemetry/config

The external http port uses the http::Server struct which uses an adapter client and has the following endpoints:

/

/api/internal/catalog (This one is really weird since it contains "internal" in it's path but is only accessible from the external port).

/api/sql

/memory

/hierarchical-memory

/prof/

/static/*path

Oh, good point! We should move that internal catalog API to the internal HTTP server.

benesch

Thanks very much for whipping this one up, @jkosh44! A few quick thoughts now having looked at your PR:

Can we make mz_http_default_user not a built-in role? It's something that only is relevant when you're running locally without authentication, so it'd be nice to hide it. What if we called the user anonymous, and created it automatically iff frontegg_auth.is_none() && tls_mode.is_none()?
I'd love to leave authentication the sole responsibility of the environmentd crate, rather than putting some of the logic into the adapter crate! At present it's kinda nice that the adapter layer just blindly trusts the user that you tell it. Can we instead add a check to the external HTTP and SQL servers that rejects any user name that begins with mz_? That'll give us the flexibility to add future restricted-access system users, should they become necessary.

jkosh44 · 2022-08-12T15:57:09Z

src/environmentd/src/http.rs

+    // Create http default user for local usage.
+    let create_default_user = frontegg.is_none() && tls_mode.is_none() && user == HTTP_DEFAULT_USER;
    // Add the authenticated user as an extension so downstream handlers can
    // inspect it if necessary.
    req.extensions_mut().insert(AuthedUser {
        user,
-        create_if_not_exists: frontegg.is_some(),
+        create_if_not_exists: frontegg.is_some() || create_default_user,


@benesch Is this what you had in mind for creating the http user on demand?

Is it expected ever that frontegg.is_none() and the tls mode is TlsMode::Require? If so this won't work because TlsMode::Require and frontegg.is_none() defaults the user to HTTP_DEFAULT_USER, but then we won't create the default user because tls_mode.is_some().

I'm asking because there are tests in auth.rs that explicitly test the HTTP API with TlsMode::Require and no frontegg and expect it to work.

After discussing with @benesch in person, this condition should be something like frontegg.is_some() || tls_mode != TlsMode::AssumeUser.

jkosh44 · 2022-08-12T18:16:48Z

@mjibson @benesch I moved the authentication logic out of the adapter crate and into the pgwire and environmentd crates, into the function that already handles authentication. I couldn't find a common place to include the logic that both pgwire and environmentd would have access to that wasn't the adapter crate. If you have any ideas then let me know.

48848ec

…system-login � Conflicts: � src/environmentd/src/lib.rs

maddyblue · 2022-08-15T19:32:44Z

src/adapter/src/catalog.rs

@@ -93,7 +93,8 @@ pub mod builtin;
 pub mod storage;

 pub const SYSTEM_CONN_ID: ConnectionId = 0;
-const SYSTEM_USER: &str = "mz_system";
+pub const SYSTEM_USER: &str = "mz_system";
+pub const HTTP_DEFAULT_USER: &str = "anonymous_http_user";


I think this should start with mz_ because that's guaranteed to be a protected username suffix.

I don't think this is possible because the way this works is that anonymous_http_user doesn't exist unless someone tries to use the HTTP API with a username. Then we will create anonymous_http_user on the spot as a user Role. However we don't allow any user roles to be created with the prefix mz_ because it's a reserved prefix.

maddyblue · 2022-08-15T19:34:31Z

src/environmentd/tests/auth.rs

@@ -988,7 +989,7 @@ fn test_auth() -> Result<(), Box<dyn Error>> {
                assert: Assert::Success,
            },
            TestCase::Http {
-                user: "mz_system",


Needs a test for logging in as mz_system but failing.

maddyblue · 2022-08-15T19:55:02Z

doesn't exist unless someone tries to use the HTTP API with a username

Oh, I missed this and the discussion about it during my review. I didn't realize it's local only. Ok, LGTM then.

jkosh44 force-pushed the mz-system-login branch 11 times, most recently from 654ecc2 to 3dfd866 Compare August 10, 2022 23:21

jkosh44 marked this pull request as ready for review August 10, 2022 23:45

jkosh44 force-pushed the mz-system-login branch from 3dfd866 to 1c8a30c Compare August 10, 2022 23:57

jkosh44 requested review from maddyblue and benesch August 10, 2022 23:57

maddyblue reviewed Aug 11, 2022

View reviewed changes

jkosh44 added 2 commits August 11, 2022 16:28

Clean up code

330430e

Change method name

8c30970

benesch reviewed Aug 12, 2022

View reviewed changes

jkosh44 added 2 commits August 12, 2022 11:38

Fix auth logic

80826de

Remove http builtin role

4230a90

jkosh44 commented Aug 12, 2022

View reviewed changes

Move auth out of the adapter crate

48848ec

jkosh44 added 5 commits August 12, 2022 15:42

Fix tests

53935c6

Fix tests

d05f40f

Add error for missing username

9eff207

Merge branch 'main' of github.com:MaterializeInc/materialize into mz-…

799d2c0

…system-login � Conflicts: � src/environmentd/src/lib.rs

Fix merge conflict

e087c15

jkosh44 added 2 commits August 15, 2022 14:46

Fix create user logic

eb23b31

Clean up imports and fix test

dbf4f15

jkosh44 requested review from maddyblue and benesch August 15, 2022 19:24

maddyblue approved these changes Aug 15, 2022

View reviewed changes

maddyblue reviewed Aug 15, 2022

View reviewed changes

Add tests for system user

a1e5ace

jkosh44 enabled auto-merge (squash) August 15, 2022 20:12

jkosh44 merged commit 041f326 into MaterializeInc:main Aug 15, 2022

jkosh44 deleted the mz-system-login branch August 15, 2022 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapter: Prevent login to mz_system externally #14093

adapter: Prevent login to mz_system externally #14093

jkosh44 commented Aug 4, 2022 •

edited

Loading

maddyblue Aug 11, 2022

jkosh44 Aug 11, 2022

maddyblue Aug 11, 2022

jkosh44 Aug 11, 2022

maddyblue Aug 11, 2022

jkosh44 Aug 11, 2022

benesch Aug 12, 2022

benesch left a comment

jkosh44 Aug 12, 2022

jkosh44 Aug 12, 2022 •

edited

Loading

jkosh44 Aug 12, 2022

jkosh44 Aug 15, 2022

jkosh44 commented Aug 12, 2022

maddyblue Aug 15, 2022

jkosh44 Aug 15, 2022

maddyblue Aug 15, 2022

jkosh44 Aug 15, 2022

maddyblue commented Aug 15, 2022

adapter: Prevent login to mz_system externally #14093

adapter: Prevent login to mz_system externally #14093

Conversation

jkosh44 commented Aug 4, 2022 • edited Loading

Motivation

Checklist

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

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkosh44 Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkosh44 commented Aug 12, 2022

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

maddyblue commented Aug 15, 2022

jkosh44 commented Aug 4, 2022 •

edited

Loading

jkosh44 Aug 12, 2022 •

edited

Loading