Auth: migrate hash string to be directly compatible with pg/pgbouncer #33714
Conversation
There was a problem hiding this comment.
Thanks for adding tests!
Nightly triggered: https://buildkite.com/materialize/nightly/builds/13582
DAlperin
force-pushed
the
dov/migrate-auth-string-compatability
branch
from
6c8ceef to
3baac42
Compare
|
oops had to rebase https://buildkite.com/materialize/nightly/builds/13593 |
DAlperin
force-pushed
the
dov/migrate-auth-string-compatability
branch
5 times, most recently
from
e82e4e9 to
6286755
Compare
This adds auth testdrive tests (and the ability for testdrive to test auth).
DAlperin
force-pushed
the
dov/migrate-auth-string-compatability
branch
3 times, most recently
from
f1f3b1c to
e25a29c
Compare
DAlperin
force-pushed
the
dov/migrate-auth-string-compatability
branch
from
e25a29c to
643670c
Compare
When I originally wrote auth I used a postgres inspired string format to store the hashes, but didn't index on making it 1:1 compatible since we had originally decided not to surface the hash to the user... That didn't last. Notion (and perhaps others) want to be able to use pgbouncer. It would be easy enough to give instructions/a script to make the existing hash format into the one pgbouncer wants in its config file but... we could just not do that.
DAlperin
force-pushed
the
dov/migrate-auth-string-compatability
branch
from
643670c to
a119277
Compare
|
@def- I've had to do some unfortunate hacks to get around the lack of auth in older versions. Can you take a look when you get a chance and see if there is a better way to do what I've done? |
| auth_user3 | ||
| auth_user_nopass | ||
| # Test connection with user2 |
There was a problem hiding this comment.
We should test with all users during the validate phase.
| listeners_config_path = ( | ||
| f"{MZ_ROOT}/src/materialized/ci/listener_configs/no_auth.json" | ||
| ) | ||
| if (self.tag and self.tag >= MzVersion.parse("v0.159.0-dev")) or not self.tag: | ||
| listeners_config_path = ( | ||
| f"{MZ_ROOT}/src/materialized/ci/listener_configs/mixed_auth.json" | ||
| ) |
There was a problem hiding this comment.
This might conflict with some changes @SangJunBak is working on, but I guess we can merge it.
| config: ServiceConfig = { | ||
| "mzbuild": name, | ||
| "ports": [6875, 6876, 6877, 6878, 26257], | ||
| "ports": [6875, 6876, 6877, 6878, 6885, 6895, 26257], |
There was a problem hiding this comment.
Comment would be nice about what these ports do. Do they always exist? If not, have a ports parameter and pass it in for platform-checks and other tests that require these extra ports.
| .Raw != "postgresql://materialize:[email protected]:5432" and | ||
| .Raw != "d3aa325086974cdfb3912f28e5a8c168" and | ||
| .Raw != "jdbc:postgresql://postgres:5432/postgres" and | ||
| (.Raw | test("^postgres://auth_user[0-9]:password[0-9]@[^:]+:68$") | not) and |
There was a problem hiding this comment.
Ending on 68 is weird, but I guess trufflehog was complaining like that?
2 participants
When I originally wrote auth I used a postgres inspired string format to
store the hashes, but didn't index on making it 1:1 compatible since we
had originally decided not to surface the hash to the user... That
didn't last.
Notion (and perhaps others) want to be able to use pgbouncer. It would
be easy enough to give instructions/a script to make the existing hash
format into the one pgbouncer wants in its config file but... we could
just not do that.
(This PR also adds platform-checks for passwords to ensure functionality
across versions)
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.