sqlproxyccl: block session revival token in StartupMessage from client#76750
Conversation
|
This change is |
jaylim-crl
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rafiss)
pkg/ccl/sqlproxyccl/frontend_admitter.go, line 83 at r1 (raw file):
// The client is blocked from using session revival tokens; only the proxy // itself can. delete(startup.Parameters, sessionRevivalTokenStartupParam)
Should we return an error instead of silently removing the param to prevent confusions from the user's end (assuming that they end up using it)?
rafiss
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/sqlproxyccl/frontend_admitter.go, line 83 at r1 (raw file):
Previously, jaylim-crl (Jay Lim) wrote…
Should we return an error instead of silently removing the param to prevent confusions from the user's end (assuming that they end up using it)?
sure, sounds good
09e37bb to
44a6cf8
Compare
jaylim-crl
left a comment
There was a problem hiding this comment.
TestFrontendAdmitSessionRevivalToken will fail because we return a nil conn on error.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status:
pkg/ccl/sqlproxyccl/frontend_admitter.go, line 84 at r2 (raw file):
We need to return conn here despite an error, or else the caller will panic.
// message received from the PG SQL client. The connection returned should never
// be nil in case of error. Depending on whether the error happened before the
// connection was upgraded to TLS or not it will either be the original or the
// TLS connection.
44a6cf8 to
de22f1d
Compare
Only the SQL proxy itself should be setting this. To prevent an outsider from trying to authenticate with a token, the proxy now checks for the token in any incoming startup message. If it is present, it returns an error. Release note: None
de22f1d to
4871227
Compare
rafiss
left a comment
There was a problem hiding this comment.
tftr!
bors r=jaylim-crl
Reviewable status:
pkg/ccl/sqlproxyccl/frontend_admitter.go, line 84 at r2 (raw file):
Previously, jaylim-crl (Jay Lim) wrote…
We need to return conn here despite an error, or else the caller will panic.
// message received from the PG SQL client. The connection returned should never
// be nil in case of error. Depending on whether the error happened before the
// connection was upgraded to TLS or not it will either be the original or the
// TLS connection.
ty! fixed
|
Build succeeded: |
fixes #74643
Only the SQL proxy itself should be setting this. To prevent an outsider
from trying to authenticate with a token, the proxy now checks for the
token in any incoming startup message. If it is present, it returns an
error.
Release note: None