sasl/scram #33468

DAlperin · 2025-08-30T04:42:45Z

This implements the SASL/SCRAM protocol as implemented by postgres. The goal is to be compatible with postgres clients that support/expect SASL/SCRAM.

Motivation

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.

jasonhernandez

I did a preliminary review and wanted to give some guidance on timing attacks and input validation. I assume some of these are already on your mental to-do list even if not called out.

src/auth/src/hash.rs

src/adapter/src/coord/command_handler.rs

SangJunBak · 2025-09-16T15:03:39Z

Sorry I just saw this! Looking over it now

src/environmentd/src/http.rs

src/server-core/src/listeners.rs

src/pgwire/src/protocol.rs

src/auth/src/hash.rs

SangJunBak

Overall the code looks great, just recommendations on documentation and testing. Nits are optional but I think we should do the comments related to testing. Lemme know if you want me to clarify anything in this review

SangJunBak · 2025-09-19T13:06:26Z

src/adapter/src/coord/command_handler.rs

+    ) {
+        let role = self.catalog().try_get_role_by_name(role_name.as_str());
+        let role_auth = role
+            .as_ref()


nit: I think the as_refs here, line 292, and line 297 are redundant

SangJunBak · 2025-09-19T13:09:02Z

src/adapter/src/coord/command_handler.rs

+        let real_hash = role_auth
+            .as_ref()
+            .and_then(|auth| auth.password_hash.as_ref());
+        let hash_ref: &str = real_hash.map(|s| s.as_str()).unwrap_or(&mock_hash);


nit: I think the explicit type annotation is redundant

SangJunBak · 2025-09-22T13:07:52Z

src/auth/src/hash.rs

+        let signing_key = openssl::pkey::PKey::hmac(&server_key)
+            .map_err(|e| VerifyError::Hash(HashError::Openssl(e)))?;
+        let mut signer =
+            openssl::sign::Signer::new(openssl::hash::MessageDigest::sha256(), &signing_key)
+                .map_err(|e| VerifyError::Hash(HashError::Openssl(e)))?;
+        signer
+            .update(auth_message.as_bytes())
+            .map_err(|e| VerifyError::Hash(HashError::Openssl(e)))?;
+        let server_signature = signer
+            .sign_to_vec()
+            .map_err(|e| VerifyError::Hash(HashError::Openssl(e)))?;


nit: Can probably de-duplicate this code w/ the client signature code by making it a helper function or anonymous function

SangJunBak · 2025-09-22T13:12:05Z

src/pgwire/src/message.rs

+pub enum SASLServerFinalMessageKinds {
+    Verifier(String),
+    // The spec specifies an Error kind here but PG just uses
+    // it's own error handling


SangJunBak · 2025-09-22T13:32:24Z

src/adapter/src/coord/command_handler.rs

+                    Err(_) => {
+                        send_mock_challenge(
+                            role_name,
+                            self.catalog().state().mock_authentication_nonce(),


It's not obvious the purpose of mock_authentication_nonce. We should probably document this above the definition of send_mock_challenge and here as well

SangJunBak · 2025-09-22T13:35:34Z

src/auth/src/hash.rs

+        let opts1 = mock_sasl_challenge(username, mock);
+        let opts2 = mock_sasl_challenge(username, mock);
+        assert_eq!(opts1, opts2);
+    }


I'd actually like to see more unit tests for sasl_verify! Especially since it's a pure function. Maybe something like:

Positive case

Client proof doesn't match

SangJunBak · 2025-09-22T13:46:40Z

src/pgwire/src/protocol.rs

+                                    &user,
+                                    &response.proof,
+                                    &auth_message,
+                                    &mock_hash,


I'm guessing mock_hash is the hash generated from the mock_nonce and mock_sasl_challenge? Not immediately obvious the purpose of it and where it comes from, especially since the control flow goes jumps back and forth between coord messages. Should probably document where it comes from and the purpose it serves!

src/pgwire/src/codec.rs

SangJunBak · 2025-09-22T14:30:15Z

I think the next step is rigging up the orchestratord changes. Not sure if you'd like to do that or delegate that to Alex. Regardless should prolly keep the self managed peeps updated!

jasonhernandez

Thanks for addressing my comments / review. I think this looks ok from a security standpoint. More comments / test cases in line with @SangJunBak's recommendations are nice to add but not blocking re: security.

The (SASL/SCRAM)[MaterializeInc#33468] PR introduces the need for some stable, cluster wide, cryptographically random key material. We use this material to be able to present deterministic challenges for even users that don't exist to guard against enumeration attacks. Old versions of the catalog won't have this so we have to add it.

The (SASL/SCRAM)[MaterializeInc#33468] PR introduces the need for some stable, cluster wide, cryptographically random key material. We use this material to be able to present deterministic challenges for even users that don't exist to guard against enumeration attacks. However that PR made a bad assumption that the initialize step of catalog opening would always add this new key. But old versions that have already been initialized wouldn't have it! This PR add code to generate it for old versions

The [SASL/SCRAM](MaterializeInc#33468) PR introduces the need for some stable, cluster wide, cryptographically random key material. We use this material to be able to present deterministic challenges for even users that don't exist to guard against enumeration attacks. However that PR made a bad assumption that the initialize step of catalog opening would always add this new key. But old versions that have already been initialized wouldn't have it! This PR add code to generate it for old versions

This reverts commit 2c17d81.

The [SASL/SCRAM](MaterializeInc#33468) PR introduces the need for some stable, cluster wide, cryptographically random key material. We use this material to be able to present deterministic challenges for even users that don't exist to guard against enumeration attacks. However that PR made a bad assumption that the initialize step of catalog opening would always add this new key. But old versions that have already been initialized wouldn't have it! This PR add code to generate it for old versions

The [SASL/SCRAM](#33468) PR introduces the need for some stable, cluster wide, cryptographically random key material. We use this material to be able to present deterministic challenges for even users that don't exist to guard against enumeration attacks. However that PR made a bad assumption that the initialize step of catalog opening would always add this new key. But old versions that have already been initialized wouldn't have it! This PR add code to generate it for old versions  ### Motivation Fixes MaterializeInc/database-issues#9724  ### Tips for reviewer  ### Checklist - [x] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design.  - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (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](MaterializeInc/cloud#5021)).  - [ ] If this PR includes major [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note), I have pinged the relevant PM to schedule a changelog post.

jasonhernandez requested changes Aug 30, 2025

View reviewed changes

DAlperin force-pushed the dov/sasl-scram branch 4 times, most recently from a5bf100 to fde3947 Compare September 8, 2025 21:00

DAlperin marked this pull request as ready for review September 8, 2025 21:00

DAlperin requested a review from a team as a code owner September 8, 2025 21:00

DAlperin requested a review from SangJunBak September 8, 2025 21:00