-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
sql/pgwire: treat system identity as string, not SQLUsername #130979
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
6fb96b8
to
ebd5b1e
Compare
e03e150
to
f97c176
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 17 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pritesh-lahoti and @rafiss)
pkg/sql/pgwire/auth.go
line 185 at r3 (raw file):
// in. Now we can delegate to the selected AuthMethod implementation to // complete the authentication. We must pass in the systemIdentity here, // since the authenticator may use an external source to verify the
This makes it clear now. Thank you for pointing this out.
pkg/sql/pgwire/auth_methods.go
line 920 at r3 (raw file):
}) b := &AuthBehaviors{} b.SetRoleMapper(UseSpecifiedIdentity(sessionUser))
This was tricky previously with UseProvidedIdentity. This would solve it when we want to use a different identity for behaviors.MapRole
and behaviors.Authenticate
.
pkg/sql/pgwire/auth_methods.go
line 943 at r3 (raw file):
// Verify that the systemIdentity is what we expect. if ldapUserDN.String() != systemIdentity { err := errors.New("LDAP user DN mismatch")
nit: we can log the ldapUserDN and systemIdentity here incase of mismatch
Suggestion:
err := errors.Newf("LDAP user DN mismatch, expected user DN: %s, obtained systemIdentity: %s", ldapUserDN.String(), systemIdentity)
f97c176
to
a9c498c
Compare
TFTR! bors r+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @pritesh-lahoti and @souravcrl)
pkg/sql/pgwire/auth_methods.go
line 943 at r3 (raw file):
Previously, souravcrl wrote…
nit: we can log the ldapUserDN and systemIdentity here incase of mismatch
done
Build failed: |
bors retry |
Build failed (retrying...): |
Build failed (retrying...): |
Build failed: |
a9c498c
to
74c5580
Compare
The external system identity may not be a SQLUsername -- that mapping is performed at a different phase of authentication. To make this more clear, we stop using the SQLUsername type for system identity. Release note: None
74c5580
to
eb67431
Compare
bors r+ |
1 similar comment
bors r+ |
Already running a review |
The external system identity may not be a SQLUsername -- that mapping is performed at a different phase of authentication. To make this more clear, we stop using the SQLUsername type for system identity.
Epic CRDB-33829
Release note: None