Tighten SCRAM-SHA-256 SASL check #6023
Conversation
| @@ -95,7 +95,7 @@ async Task AuthenticateSASL(List<string> mechanisms, string username, bool async | |||
| if (clientSupportsSha256Plus) | |||
| DataSource.TransportSecurityHandler.AuthenticateSASLSha256Plus(this, ref mechanism, ref cbindFlag, ref cbind, ref successfulBind); | |||
|
|
|||
| if (!successfulBind && serverSupportsSha256) | |||
| if (!successfulBind && clientSupportsSha256) | |||
There was a problem hiding this comment.
Any reason to not just check both, in order to avoid attempting SCRAM-SHA-256 if the server doesn't support it? At the very least, it would probably help generate a more meaningful exception message?
BTW we can in general try to raise more informative/specific exception messages here for the precise condition. For example, if I read the code write, we always throw "unable to bind to SCRAM-SHA-256-PLUS" below, even if e.g. the client allows SCRAM-SHA-256 but the server doesn't etc.
There was a problem hiding this comment.
Any reason to not just check both, in order to avoid attempting SCRAM-SHA-256 if the server doesn't support it? At the very least, it would probably help generate a more meaningful exception message?
clientSupportsSha256 has a bit misleading name, as it accounts for whether both the client (us) and the server support SCRAM-SHA-256. I just can't really think of a better name other than clientAndServerSupportSha256 or connectionSupportsSha256, and both of them are quite mouthful. Maybe allowSha256?
npgsql/src/Npgsql/Internal/NpgsqlConnector.Auth.cs
Lines 73 to 74 in 44a7ab1
BTW we can in general try to raise more informative/specific exception messages here for the precise condition. For example, if I read the code write, we always throw "unable to bind to SCRAM-SHA-256-PLUS" below, even if e.g. the client allows SCRAM-SHA-256 but the server doesn't etc.
You mean, the client wants only SCRAM-SHA-256 but the server wants SCRAM-SHA-256-PLUS? Because we already handle that above, this check in particular is if and only if the server supports only SCRAM-SHA-256-PLUS but there was some issue with it (e.g. unsupported cert algorithm).
npgsql/src/Npgsql/Internal/NpgsqlConnector.Auth.cs
Lines 73 to 88 in 44a7ab1
There was a problem hiding this comment.
clientSupportsSha256 has a bit misleading name, as it accounts for whether both the client (us) and the server support SCRAM-SHA-256. I just can't really think of a better name other than clientAndServerSupportSha256 or connectionSupportsSha256, and both of them are quite mouthful. Maybe allowSha256?
Ah I see, I was indeed misled... allowSha256 sounds pretty good...
Otherwise I didn't look too closely at the exception, if you think we're specific enough that's more than fine for me.
(cherry picked from commit 01155b6)
(cherry picked from commit 01155b6)
Right now whenever we authenticate via SASL, we might either connect with channel binding (SCRAM-SHA-256-PLUS) or without (SCRAM-SHA-256), depending on whether the server supports it and
ChannelBindingproperty in connection string. Now, in some cases (like the cert passed from the server has an unsupported hash algorithm) we might allow to downgrade fromSCRAM-SHA-256-PLUStoSCRAM-SHA-256, as long as the server supports auth without channel binding. The problem is that it seems like we ignoreChannelBinding, so even if a user passesChannelBinding.Requirewe can still potentially downgrade. This change makes sure that in that case we'll throw an exception instead of going through.Shouldn't be much of a security issue as I fully expect that in case of a problematic cert we'll fail somewhere along TLS handshake.