-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release-24.1: ccl, cli,server, sql, testutils: fix IdMap setting + client cert auth #122738
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This update fixes the logic for matching certificate names in the identity map. In version 22.2, client certificate authentication combined with auth-username-maps diverged from PostgreSQL's implementation. For instance, PostgreSQL required the connection string `postgresql://db_user@localhost:26257?sslcert=certs/client.cert_user.crt&sslkey=certs/client.cert_user.key&sslmode=verify-full&sslrootcert=certs/ca.crt` for authentication with a certificate issued to CN=cert_user, whereas CockroachDB 22.2 accepted `postgresql://cert_user@localhost:26257?sslcert=certs/client.cert_user.crt&sslkey=certs/client.cert_user.key&sslmode=verify-full&sslrootcert=certs/ca.crt` This discrepancy arose because CockroachDB previously did not check if the client-provided username was a valid database user, leading to an automatic overwrite with the mapped database user. In version 23.1, an attempt to align with PostgreSQL's behavior was made by verifying that the client-provided username matches at least one of the mappings for the system identity inferred from the certificate, as detailed in PR #94915. This introduced a backward incompatible change, requiring the connection string to specify a valid database user while presenting a certificate issued to CN=cert_user, like so: `postgresql://db_user@localhost:26257?sslcert=certs/client.cert_user.crt&sslkey=certs/client.cert_user.key&sslmode=verify-full&sslrootcert=certs/ca.crt` However, this approach failed because we did not correctly extract and use the CN from the presented certificate as the system identity, preventing successful authentication. This commit resolves this issue by implementing the correct logic to extract the CN from presented certificates and utilize it as the system identity, ensuring the intended authentication flow works as expected. Fixes #120034 , CRDB-36439 Release note (bug fix): Client Certificate Auth combined with identity maps(server.identity_map.configuration) was broken in 23.1. This bug is fixed; note that the feature to work correctly, the client must specify a valid db user in connection string.
blathers-crl
bot
added
blathers-backport
This is a backport that Blathers created automatically.
O-robot
Originated from a bot.
labels
Apr 20, 2024
blathers-crl
bot
requested review from
DarrylWong and
renatolabs
and removed request for
a team
April 20, 2024 07:47
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
blathers-crl
bot
added
the
backport
Label PR's that are backports to older release branches
label
Apr 20, 2024
bdarnell
approved these changes
Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
backport
Label PR's that are backports to older release branches
blathers-backport
This is a backport that Blathers created automatically.
O-robot
Originated from a bot.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport 1/1 commits from #120432 on behalf of @BabuSrithar.
/cc @cockroachdb/release
ccl, cli,server, sql, testutils: fix IdMap setting + client cert auth
This update fixes the logic for matching certificate names in the identity map.
In version 22.2, client certificate authentication combined with
auth-username-maps diverged from PostgreSQL's implementation. For instance,
PostgreSQL required the connection string
postgresql://db_user@localhost:26257?sslcert=certs/client.cert_user.crt&sslkey=certs/client.cert_user.key&sslmode=verify-full&sslrootcert=certs/ca.crt
for authentication with a certificate issued to CN=cert_user, whereas
CockroachDB 22.2 accepted
postgresql://cert_user@localhost:26257?sslcert=certs/client.cert_user.crt&sslkey=certs/client.cert_user.key&sslmode=verify-full&sslrootcert=certs/ca.crt
This discrepancy arose because CockroachDB previously did not check if the
client-provided username was a valid database user, leading to an automatic
overwrite with the mapped database user.
In version 23.1, an attempt to align with PostgreSQL's behavior was made by
verifying that the client-provided username matches at least one of the mappings
for the system identity inferred from the certificate, as detailed in PR
#94915. This introduced a backward incompatible change, requiring the
connection string to specify a valid database user while presenting a
certificate issued to CN=cert_user, like so:
postgresql://db_user@localhost:26257?sslcert=certs/client.cert_user.crt&sslkey=certs/client.cert_user.key&sslmode=verify-full&sslrootcert=certs/ca.crt
However, this approach failed because we did not correctly extract and use the
CN from the presented certificate as the system identity, preventing successful
authentication.
This commit resolves this issue by implementing the correct logic to extract the
CN from presented certificates and utilize it as the system identity, ensuring
the intended authentication flow works as expected.
Fixes #120034 , CRDB-36439
Release note (bug fix): Client Certificate Auth combined with identity
maps(server.identity_map.configuration) was broken in 23.1. This bug is fixed;
note that the feature to work correctly, the client must specify a valid db user
in connection string.
Release justification: the fix needs to go in for addressing bug related to the issue