Skip to content
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-23.1: sql: fix read-only SSL var #100506

Merged
merged 1 commit into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/sql/conn_executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,7 @@ func newSessionData(args SessionArgs) *sessiondata.SessionData {
},
LocalUnmigratableSessionData: sessiondata.LocalUnmigratableSessionData{
RemoteAddr: args.RemoteAddr,
IsSSL: args.IsSSL,
},
LocalOnlySessionData: sessiondatapb.LocalOnlySessionData{
ResultsBufferSize: args.ConnResultsBufferSize,
Expand Down
1 change: 1 addition & 0 deletions pkg/sql/exec_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -2119,6 +2119,7 @@ type SessionDefaults map[string]string
type SessionArgs struct {
User username.SQLUsername
IsSuperuser bool
IsSSL bool
SystemIdentity username.SQLUsername
SessionDefaults SessionDefaults
CustomOptionSessionDefaults SessionDefaults
Expand Down
48 changes: 48 additions & 0 deletions pkg/sql/pgwire/auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import (
"github.com/cockroachdb/errors"
"github.com/cockroachdb/errors/stdstrings"
"github.com/cockroachdb/redact"
pgx "github.com/jackc/pgx/v4"
"github.com/lib/pq"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -773,4 +774,51 @@ func TestClientAddrOverride(t *testing.T) {
}
}

// TestSSLSessionVar checks that the read-only SSL session variable correctly
// reflects the state of the connection.
func TestSSLSessionVar(t *testing.T) {
defer leaktest.AfterTest(t)()
sc := log.ScopeWithoutShowLogs(t)
defer sc.Close(t)

// Start a server.
s, db, _ := serverutils.StartServer(t, base.TestServerArgs{})
s.(*server.TestServer).Cfg.AcceptSQLWithoutTLS = true
ctx := context.Background()
defer s.Stopper().Stop(ctx)

// Ensure the test user exists.
if _, err := db.Exec(fmt.Sprintf(`CREATE USER %s WITH PASSWORD 'abc'`, username.TestUser)); err != nil {
t.Fatal(err)
}

pgURLWithCerts, cleanupFuncCerts := sqlutils.PGUrlWithOptionalClientCerts(
t, s.ServingSQLAddr(), "TestSSLSessionVarCerts" /* prefix */, url.User(username.TestUser), true,
)
defer cleanupFuncCerts()

pgURLWithoutCerts, cleanupFuncWithoutCerts := sqlutils.PGUrlWithOptionalClientCerts(
t, s.ServingSQLAddr(), "TestSSLSessionVarNoCerts" /* prefix */, url.UserPassword(username.TestUser, "abc"), false,
)
defer cleanupFuncWithoutCerts()
q := pgURLWithoutCerts.Query()
q.Set("sslmode", "disable")
pgURLWithoutCerts.RawQuery = q.Encode()

// Connect with certs.
connWithCerts, err := pgx.Connect(ctx, pgURLWithCerts.String())
require.NoError(t, err)
var result string
err = connWithCerts.QueryRow(ctx, "SHOW ssl").Scan(&result)
require.NoError(t, err)
require.Equal(t, "on", result)

// Connect without certs.
connWithoutCerts, err := pgx.Connect(ctx, pgURLWithoutCerts.String())
require.NoError(t, err)
err = connWithoutCerts.QueryRow(ctx, "SHOW ssl").Scan(&result)
require.NoError(t, err)
require.Equal(t, "off", result)
}

var sessionTerminatedRe = regexp.MustCompile("client_session_end")
1 change: 1 addition & 0 deletions pkg/sql/pgwire/pre_serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@ func (s *PreServeConnHandler) PreServe(
st.Reserved.Close(ctx)
return conn, st, s.sendErr(ctx, conn, err)
}
st.clientParameters.IsSSL = st.ConnType == hba.ConnHostSSL

st.State = PreServeReady
return conn, st, nil
Expand Down
3 changes: 1 addition & 2 deletions pkg/sql/pgwire/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -746,8 +746,7 @@ func (s *Server) ServeConn(
return s.sendErr(ctx, st, conn, newAdminShutdownErr(ErrDrainingNewConn))
}

sArgs, err := finalizeClientParameters(ctx, preServeStatus.clientParameters,
&st.SV)
sArgs, err := finalizeClientParameters(ctx, preServeStatus.clientParameters, &st.SV)
if err != nil {
preServeStatus.Reserved.Close(ctx)
return s.sendErr(ctx, st, conn, err)
Expand Down
7 changes: 5 additions & 2 deletions pkg/sql/sessiondata/session_data.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,12 +169,15 @@ type LocalUnmigratableSessionData struct {
// dependencies. Temporary tables are not supported in session migrations.
DatabaseIDToTempSchemaID map[uint32]uint32

///////////////////////////////////////////////////////////////////////////
// IsSSL indicates whether the session is using SSL/TLS.
IsSSL bool

// ////////////////////////////////////////////////////////////////////////
// WARNING: consider whether a session parameter you're adding needs to //
// be propagated to the remote nodes or needs to persist amongst session //
// migrations. If so, they should live in the LocalOnlySessionData or //
// SessionData protobuf in the sessiondatapb package. //
///////////////////////////////////////////////////////////////////////////
// ////////////////////////////////////////////////////////////////////////
}

// IsTemporarySchemaID returns true if the given ID refers to any of the temp
Expand Down
3 changes: 1 addition & 2 deletions pkg/sql/vars.go
Original file line number Diff line number Diff line change
Expand Up @@ -1330,8 +1330,7 @@ var varGen = map[string]sessionVar{
`ssl`: {
Hidden: true,
Get: func(evalCtx *extendedEvalContext, _ *kv.Txn) (string, error) {
insecure := evalCtx.ExecCfg.RPCContext.Config.Insecure || evalCtx.ExecCfg.RPCContext.Config.AcceptSQLWithoutTLS
return formatBoolAsPostgresSetting(!insecure), nil
return formatBoolAsPostgresSetting(evalCtx.SessionData().IsSSL), nil
},
},

Expand Down