orchestratord: Add SASL/SCRAM-SHA-256 authentication #34045
Conversation
doy-materialize
left a comment
doy-materialize
left a comment
There was a problem hiding this comment.
overall looks reasonable, just a couple questions about the tests
test/orchestratord/mzcompose.py
Outdated
| if self.value == "Password" and version <= MzVersion.parse_mz("v0.147.6"): | ||
| if ( | ||
| self.value == "Password" or self.value == "Sasl" | ||
| ) and version <= MzVersion.parse_mz("v0.147.6"): |
There was a problem hiding this comment.
presumably we'll need a different version check for sasl vs password, since sasl was only supported later?
There was a problem hiding this comment.
hm yes, good point, let me try to track down when this was introduced
There was a problem hiding this comment.
as far as I can tell this was introduced in v0.147.16: https://github.com/MaterializeInc/release/issues/302
test/orchestratord/mzcompose.py
Outdated
| 6875 | ||
| if version >= MzVersion.parse_mz("v0.147.0") and self.value == "Password" | ||
| if version >= MzVersion.parse_mz("v0.147.0") | ||
| and (self.value == "Password" or self.value == "Sasl") |
bobbyiliev
force-pushed
the
orchestratord-scram-auth
branch
4 times, most recently
from
5531f5f to
96856ee
Compare
| let authenticator_kind = mz.spec.authenticator_kind; | ||
| let auth_kind = mz.spec.authenticator_kind; |
| base: BaseListenerConfig { | ||
| addr: SocketAddr::new(IpAddr::V4(Ipv4Addr::new(0,0,0,0)), config.environmentd_http_port), | ||
| authenticator_kind, | ||
| authenticator_kind: http_auth, |
There was a problem hiding this comment.
NIT (style) - I think this could be fewer changes and just as clean.
// SASL authentication is only supported for SQL (PostgreSQL wire protocol).
// HTTP listeners must use Password authentication when SASL is enabled.
// This is validated at environmentd startup via ListenerConfig::validate().
authenticator_kind: if authenticator_kind == AuthenticatorKind::Sasl {
AuthenticatorKind::Password
} else {
authenticator_kind
}This is fine for now, but I think it would be rather confusing if we introduce SSO/JWT auth.... we may want to allow users to set http and pgwire authenticator kinds separately, but that would require us to more or less completely change how this logic works.
There was a problem hiding this comment.
Good suggestion! Thanks Justin!
| if matches!( | ||
| auth_kind, | ||
| AuthenticatorKind::Password | AuthenticatorKind::Sasl | ||
| ) { |
There was a problem hiding this comment.
If we are checking this a bunch perhaps at some point we could add a fn authenticated(&self) -> bool to AuthenticatorKind, but fine for now
There was a problem hiding this comment.
Yeah good call! Especially once we start adding SSO/JWT auth, having something like authenticated() will probably make things cleaner. Worth keeping in mind.
bobbyiliev
force-pushed
the
orchestratord-scram-auth
branch
from
96856ee to
b15bf4f
Compare
bobbyiliev
force-pushed
the
orchestratord-scram-auth
branch
from
b15bf4f to
ba6e97a
Compare
4 participants
Motivation
Enables SCRAM authentication for SQL connections in self-managed deployments. HTTP listeners automatically fall back to Password authentication when SASL is configured, as SASL is only supported for the PostgreSQL wire
protocol.
As suggested by Justin, we might want to refactor this as we introduce additional authentication modes like SSO and JWT etc.
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.