Conversation
| 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)
Updated [Npgsql](https://github.com/npgsql/npgsql) from 9.0.3 to 10.0.2. <details> <summary>Release notes</summary> _Sourced from [Npgsql's releases](https://github.com/npgsql/npgsql/releases)._ ## 10.0.2 v10.0.2 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/135?closed=1) **Full Changelog**: npgsql/npgsql@v10.0.1...v10.0.2 ## 10.0.1 v10.0.1 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/134?closed=1) **Full Changelog**: npgsql/npgsql@v10.0.0...v10.0.1 ## 10.0.0 See the [release notes](https://www.npgsql.org/doc/release-notes/10.0.html). The full list of changes is available [here](https://github.com/npgsql/npgsql/milestone/122?closed=1). ## What's Changed * STJ 9.0 alternative approach by @NinoFloris in npgsql/npgsql#5941 * Remove support for net6.0 by @roji in npgsql/npgsql#5947 * Some leftover cleanup for removing net6.0 by @roji in npgsql/npgsql#5949 * Map date/time to DateOnly/TimeOnly by default by @roji in npgsql/npgsql#5948 * Make the cidr<->IPNetwork mapping the default by @roji in npgsql/npgsql#5950 * Fix connecting with VerifyCA and VerifyFull by @vonzshik in npgsql/npgsql#5944 * Remove stopwatch allocations by @vonzshik in npgsql/npgsql#5977 * Bump actions/setup-dotnet from 4.1.0 to 4.2.0 by @dependabot[bot] in npgsql/npgsql#5983 * Use exception convenience methods by @bbowyersmyth in npgsql/npgsql#5982 * Bump actions/setup-dotnet from 4.2.0 to 4.3.0 by @dependabot[bot] in npgsql/npgsql#6007 * Add support for postgresql type names with dots by @dvas-hash in npgsql/npgsql#5971 * Send close_notify TLS alert on connection shutdown by @vonzshik in npgsql/npgsql#5995 * Remove DisplayClass struct creation in PgReader by @bbowyersmyth in npgsql/npgsql#6014 * Always dispose RemoteCertificate on SslStream by @vonzshik in npgsql/npgsql#6022 * Remove LongRunningConnection field from NpgsqlConnector by @vonzshik in npgsql/npgsql#6024 * Tighten SCRAM-SHA-256 SASL check by @vonzshik in npgsql/npgsql#6023 * Add SHA3 hash algorithms for SASL authentication by @vonzshik in npgsql/npgsql#6028 * Remove dotnet SDK version from CI (use global.json) by @roji in npgsql/npgsql#6037 * Add support for specifying allowed auth methods by @vonzshik in npgsql/npgsql#6036 * Migrate to SLNX by @roji in npgsql/npgsql#6053 * Switch to Ubuntu 24.04 in CI by @roji in npgsql/npgsql#6054 * Bump actions/setup-dotnet from 4.3.0 to 4.3.1 by @dependabot[bot] in npgsql/npgsql#6059 * Add basic testing for tracing by @vonzshik in npgsql/npgsql#6051 * parameter-collection Clone() should set correct collection instance by @mgravell in npgsql/npgsql#6066 * Fix brew on mac CI by @NinoFloris in npgsql/npgsql#6071 * Fix adding to hash lookup while renaming an unnamed parameter by @vonzshik in npgsql/npgsql#6073 * Update LICENSE date (2024 -> 2025) by @kurnakovv in npgsql/npgsql#6082 * Add tracing for physical connection open by @vonzshik in npgsql/npgsql#6091 * Start testing on .NET 9 by @vonzshik in npgsql/npgsql#5945 * Turn on <IsAotCompatible> by @roji in npgsql/npgsql#6097 * Reenable public API analyzer by @roji in npgsql/npgsql#6101 * Update Npgsql to .NET 9 by @vonzshik in npgsql/npgsql#6099 * Ignore system CA store if root certificate is provided by @vonzshik in npgsql/npgsql#6102 * Fix reading columns asynchronously via JsonNet plugin by @vonzshik in npgsql/npgsql#6109 * Fixes #6107 missed should buffer in biginteger numeric converter by @NinoFloris in npgsql/npgsql#6117 * Fix logging parameters with batches by @vonzshik in npgsql/npgsql#6079 * Implement GSSAPI session encryption by @vonzshik in npgsql/npgsql#6131 * feat: add support for PGAPPNAME to set application name by @michael-todorovic in npgsql/npgsql#6139 * Fix returning null from KerberosUsernameProvider.GetUsername with concurrent calls by @vonzshik in npgsql/npgsql#6137 * Add NpgsqlTsVector.Empty by @roji in npgsql/npgsql#6145 * Add assert to NpgsqlCommand.Transaction if it's completed by @vonzshik in npgsql/npgsql#6151 * Compare normalized type names by @0MG-DEN in npgsql/npgsql#6011 * Do CI testing for PG18 (beta) by @roji in npgsql/npgsql#6155 * Fix infinite consume on error with connection break by @vonzshik in npgsql/npgsql#6161 * Bump actions/checkout from 4 to 5 by @dependabot[bot] in npgsql/npgsql#6174 ... (truncated) ## 10.0.0-rc.1 ## 9.0.5 v9.0.5 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/131?closed=1) **Full Changelog**: npgsql/npgsql@v9.0.4...v9.0.5 ## 9.0.4 v9.0.4 contains several minor bug fixes. [Milestone issues](https://github.com/npgsql/npgsql/milestone/127?closed=1) **Full Changelog**: npgsql/npgsql@v9.0.3...v9.0.4 Commits viewable in [compare view](npgsql/npgsql@v9.0.3...v10.0.2). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.