Skip to content

Commit c7a69de

Browse files
craig[bot]souravcrl
andcommitted
Merge #143554
143554: cli, security: add support for configurable TLS cipher suites r=souravcrl a=souravcrl fixes #136999 Epic CRDB-45351 Release Note(security,ops): We will be providing a new cockroach start command cli flag `tls-cipher-suites` which is a string of comma separated list of cipher suites to be used for all incoming TLS connections to the node. For TLS 1.2, this should strictly be a subset of suites defined in security/tls_ciphersuites.go as RecommendedCipherSuites or OldCipherSuites. For TLS 1.3, this should be configured to a subset of ciphers in crypto/tls/cipher_suites.go. The flag will restrict TLS connections to the node for all 3 types of connection(i.e. sql, rpc, http) where node acts as the server for the connections and close non-conforming ones. The updated cockroach start command with the ciphers argument is as follows: ``` cockroach start --certs-dir=certs \ --listen-addr=localhost:26257 \ --http-addr=localhost:8080 \ --tls-cipher-suites=TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256 ``` Co-authored-by: souravcrl <[email protected]>
2 parents 7c6ffa8 + ed7ea52 commit c7a69de

File tree

10 files changed

+400
-3
lines changed

10 files changed

+400
-3
lines changed

pkg/cli/cliflags/flags.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -927,6 +927,17 @@ provided for node user if this flag is set.
927927
`,
928928
}
929929

930+
TLSCipherSuites = FlagInfo{
931+
Name: "tls-cipher-suites",
932+
Description: `
933+
A string of comma separated list of cipher suites to be used for all incoming
934+
TLS connections to the node. For TLS 1.2, this should strictly be a subset of
935+
suites defined in security/tls_ciphersuites.go as RecommendedCipherSuites or
936+
OldCipherSuites. For TLS 1.3, this should be configured to a subset of ciphers
937+
in crypto/tls/cipher_suites.go, e.g. TLS_AES_256_GCM_SHA384.
938+
`,
939+
}
940+
930941
CAKey = FlagInfo{
931942
Name: "ca-key",
932943
EnvVar: "COCKROACH_CA_KEY",

pkg/cli/context.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,7 @@ var startCtx struct {
483483
serverListenAddr string
484484
serverRootCertDN string
485485
serverNodeCertDN string
486+
serverTLSCipherSuites []string
486487

487488
// The TLS auto-handshake parameters.
488489
initToken string

pkg/cli/flags.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,9 @@ func init() {
555555
// Node cert distinguished name
556556
cliflagcfg.StringFlag(f, &startCtx.serverNodeCertDN, cliflags.NodeCertDistinguishedName)
557557

558+
// TLS Cipher Suites configured
559+
cliflagcfg.StringSliceFlag(f, &startCtx.serverTLSCipherSuites, cliflags.TLSCipherSuites)
560+
558561
// Cluster name verification.
559562
cliflagcfg.VarFlag(f, clusterNameSetter{&baseCfg.ClusterName}, cliflags.ClusterName)
560563
cliflagcfg.BoolFlag(f, &baseCfg.DisableClusterNameVerification, cliflags.DisableClusterNameVerification)
@@ -1161,6 +1164,12 @@ func extraServerFlagInit(cmd *cobra.Command) error {
11611164
if err := security.SetNodeSubject(startCtx.serverNodeCertDN); err != nil {
11621165
return err
11631166
}
1167+
// Currently we don't handle the case where we are setting the --insecure flag
1168+
// as well as providing the --tls-cipher-suites, we should probably error out
1169+
// if both are set, issue: #144935.
1170+
if err := security.SetTLSCipherSuitesConfigured(startCtx.serverTLSCipherSuites); err != nil {
1171+
return err
1172+
}
11641173
serverCfg.User = username.NodeUserName()
11651174
serverCfg.Insecure = startCtx.serverInsecure
11661175
serverCfg.SSLCertsDir = startCtx.serverSSLCertsDir

pkg/rpc/context.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ func NewServerEx(
115115
if err != nil {
116116
return nil, nil, sii, err
117117
}
118-
grpcOpts = append(grpcOpts, grpc.Creds(credentials.NewTLS(tlsConfig)))
118+
grpcOpts = append(grpcOpts, grpc.Creds(newTLSCipherRestrictCred(tlsConfig)))
119119
}
120120

121121
// These interceptors will be called in the order in which they appear, i.e.

pkg/rpc/tls.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"github.com/cockroachdb/cockroach/pkg/util/log"
2626
"github.com/cockroachdb/cockroach/pkg/util/log/severity"
2727
"github.com/cockroachdb/errors"
28+
"google.golang.org/grpc/credentials"
2829
)
2930

3031
type lazyHTTPClient struct {
@@ -325,3 +326,33 @@ func certAddrs(cert *x509.Certificate) string {
325326
strings.Join(cert.DNSNames, ","),
326327
cert.Subject.CommonName)
327328
}
329+
330+
// tlsCipherRestrictCred allows to set transport credentials for grpc
331+
// connections. It takes the security.TLSCipherRestrict function which can be
332+
// used to intercept the connection and validate requirements for TLS like
333+
// ciphers used.
334+
type tlsCipherRestrictCred struct {
335+
credentials.TransportCredentials
336+
}
337+
338+
// ServerHandshake performs TLS handshake for the connection
339+
func (cred *tlsCipherRestrictCred) ServerHandshake(
340+
conn net.Conn,
341+
) (c net.Conn, authInfo credentials.AuthInfo, err error) {
342+
// TODO(souravcrl): we need to provide a timebound context for handshake as it
343+
// ensures client failures are properly handled, issue: #144754
344+
if c, authInfo, err = cred.TransportCredentials.ServerHandshake(conn); err == nil {
345+
if err := security.TLSCipherRestrict(c); err != nil {
346+
_ = c.Close()
347+
return nil, nil, err
348+
}
349+
}
350+
return
351+
}
352+
353+
// newTLSCipherRestrictCred initializes a new tlsCipherRestrictCred credentials
354+
func newTLSCipherRestrictCred(config *tls.Config) *tlsCipherRestrictCred {
355+
return &tlsCipherRestrictCred{
356+
TransportCredentials: credentials.NewTLS(config),
357+
}
358+
}

pkg/security/BUILD.bazel

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ go_library(
5353
"@com_github_go_ldap_ldap_v3//:ldap",
5454
"@org_golang_x_crypto//bcrypt",
5555
"@org_golang_x_crypto//ocsp",
56+
"@org_golang_x_exp//slices",
5657
"@org_golang_x_sync//errgroup",
5758
],
5859
)
@@ -96,10 +97,12 @@ go_test(
9697
"//pkg/util/leaktest",
9798
"//pkg/util/log",
9899
"//pkg/util/randutil",
100+
"//pkg/util/syncutil",
99101
"//pkg/util/timeutil",
100102
"//pkg/util/uuid",
101103
"@com_github_cockroachdb_errors//:errors",
102104
"@com_github_go_ldap_ldap_v3//:ldap",
105+
"@com_github_jackc_pgx_v4//:pgx",
103106
"@com_github_stretchr_testify//require",
104107
] + select({
105108
"@io_bazel_rules_go//go/platform:aix": [

pkg/security/tls_ciphersuites.go

Lines changed: 169 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,14 @@
55

66
package security
77

8-
import "crypto/tls"
8+
import (
9+
"crypto/tls"
10+
"net"
11+
12+
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
13+
"github.com/cockroachdb/errors"
14+
"golang.org/x/exp/slices"
15+
)
916

1017
// RecommendedCipherSuites returns a list of enabled TLS 1.2 cipher
1118
// suites. The order of the list is ignored; prioritization of cipher
@@ -69,3 +76,164 @@ func OldCipherSuites() []uint16 {
6976
tls.TLS_RSA_WITH_AES_256_CBC_SHA,
7077
}
7178
}
79+
80+
type tlsRestrictConfiguration struct {
81+
syncutil.RWMutex
82+
c []string
83+
restrictFn func(*tls.Conn) (error net.Error)
84+
}
85+
86+
var tlsRestrictConfig = tlsRestrictConfiguration{
87+
c: []string{},
88+
restrictFn: func(tlsConn *tls.Conn) (error net.Error) { return },
89+
}
90+
91+
type allowedTLSCiphers struct {
92+
ciphersMapByName map[string]uint16
93+
ciphersMapByID map[uint16]string
94+
}
95+
96+
// getAllowedCiphersMapByName returns map of allowed cipher names to cipher ID.
97+
func (ciphers *allowedTLSCiphers) getAllowedCiphersMapByName() map[string]uint16 {
98+
return ciphers.ciphersMapByName
99+
}
100+
101+
// getAllowedCiphersMapByID returns map of allowed cipher ID to cipher names.
102+
func (ciphers *allowedTLSCiphers) getAllowedCiphersMapByID() map[uint16]string {
103+
return ciphers.ciphersMapByID
104+
}
105+
106+
// newAllowedTLSCiphers instantiates allowedTLSCiphers with all the ciphers
107+
// which golang implements and have been allowed for cockroach in
108+
// RecommendedCipherSuites, OldCipherSuites or as part of TLS 1.3 ciphers in
109+
// crypto/tls.
110+
func newAllowedTLSCiphers() (ciphers *allowedTLSCiphers) {
111+
ciphers = &allowedTLSCiphers{}
112+
ciphers.ciphersMapByName = map[string]uint16{}
113+
ciphers.ciphersMapByID = map[uint16]string{}
114+
cockroachEnabledCiphers := append(RecommendedCipherSuites(), OldCipherSuites()...)
115+
for _, cipher := range tls.CipherSuites() {
116+
if slices.Contains(cockroachEnabledCiphers, cipher.ID) || slices.Contains(cipher.SupportedVersions, tls.VersionTLS13) {
117+
ciphers.ciphersMapByName[cipher.Name] = cipher.ID
118+
ciphers.ciphersMapByID[cipher.ID] = cipher.Name
119+
}
120+
}
121+
for _, cipher := range tls.InsecureCipherSuites() {
122+
if slices.Contains(cockroachEnabledCiphers, cipher.ID) || slices.Contains(cipher.SupportedVersions, tls.VersionTLS13) {
123+
ciphers.ciphersMapByName[cipher.Name] = cipher.ID
124+
ciphers.ciphersMapByID[cipher.ID] = cipher.Name
125+
}
126+
}
127+
return
128+
}
129+
130+
var allowedCiphers = newAllowedTLSCiphers()
131+
132+
// getCipherID verifies if provided cipher is implemented by crypto/tls and
133+
// return the corresponding cipherID
134+
func getCipherNameFromID(cid uint16) (cipher string, ok bool) {
135+
allowedCiphersMap := allowedCiphers.getAllowedCiphersMapByID()
136+
cipher, ok = allowedCiphersMap[cid]
137+
return
138+
}
139+
140+
// SetTLSCipherSuitesConfigured sets the global TLS cipher suites for all
141+
// incoming connections(sql/rpc/http, etc.) of a node. The entries in the list
142+
// should be a subset of RecommendedCipherSuites or OldCipherSuites in case of
143+
// TLS 1.2. For TLS 1.3, they should be a subset of ciphers list defined at
144+
// https://github.com/golang/go/blob/4aa1efed4853ea067d665a952eee77c52faac774/src/crypto/tls/cipher_suites.go#L676-L679
145+
// for TLS 1.3.
146+
func SetTLSCipherSuitesConfigured(ciphers []string) error {
147+
allowedCiphersMap := allowedCiphers.getAllowedCiphersMapByName()
148+
for _, cipher := range ciphers {
149+
if _, ok := allowedCiphersMap[cipher]; !ok {
150+
return &cipherRestrictError{errors.Errorf("invalid cipher provided in tls cipher suites: %s", cipher)}
151+
}
152+
}
153+
154+
tlsRestrictConfig.configureTLSRestrict(ciphers)
155+
return nil
156+
}
157+
158+
func (*tlsRestrictConfiguration) configureTLSRestrict(ciphers []string) {
159+
tlsRestrictConfig.Lock()
160+
defer tlsRestrictConfig.Unlock()
161+
tlsRestrictConfig.restrictFn = func(tlsConn *tls.Conn) (error net.Error) { return }
162+
tlsRestrictConfig.c = ciphers
163+
if len(ciphers) == 0 {
164+
return
165+
}
166+
167+
tlsRestrictConfig.restrictFn = func(tlsConn *tls.Conn) (error net.Error) {
168+
if !tlsConn.ConnectionState().HandshakeComplete {
169+
// TODO(souravcrl): we need to provide a timebound context for handshake as it
170+
// ensures client failures are properly handled, issue: #144754
171+
if err := tlsConn.Handshake(); err != nil {
172+
// we don't want to close the connection for handshake errors
173+
return nil //nolint:returnerrcheck
174+
}
175+
}
176+
selectedCipherID := tlsConn.ConnectionState().CipherSuite
177+
cName, ok := getCipherNameFromID(selectedCipherID)
178+
if !ok {
179+
return &cipherRestrictError{errors.Errorf("cipher id %v does match implemented tls ciphers", selectedCipherID)}
180+
}
181+
if !slices.Contains(tlsRestrictConfig.c, cName) {
182+
return &cipherRestrictError{errors.Newf("presented cipher %s not in allowed cipher suite list", cName)}
183+
}
184+
return
185+
}
186+
}
187+
188+
// TLSCipherRestrict restricts the cipher suites used for tls connections to
189+
// ones specified by tls-cipher-suites cli flag. If the flag is not set, we do
190+
// not check for used ciphers in the connection. It returns an error if the used
191+
// cipher is not present in the configured ciphers for the node.
192+
var TLSCipherRestrict = func(conn net.Conn) (err net.Error) {
193+
var tlsRestrictFn func(*tls.Conn) (error net.Error)
194+
{
195+
tlsRestrictConfig.Lock()
196+
defer tlsRestrictConfig.Unlock()
197+
tlsRestrictFn = tlsRestrictConfig.restrictFn
198+
}
199+
// we always expect a TLS connection here, since this is executed on the
200+
// tls.Listener or post applying tls.Server on the incoming connection
201+
tlsConn, _ := conn.(*tls.Conn)
202+
return tlsRestrictFn(tlsConn)
203+
}
204+
205+
// cipherRestrictError implements net.Error interface so that we can override
206+
// the error handling in net/http package as it only considers a net.Error type
207+
// for error rule matching. The cipher restrict error is overridden by net.Error
208+
// in TLSCipherRestrict fn.
209+
type cipherRestrictError struct {
210+
err error
211+
}
212+
213+
// Error implements net.Error.
214+
func (e *cipherRestrictError) Error() string { return e.err.Error() }
215+
216+
// Unwrap implements net.Error.
217+
func (e *cipherRestrictError) Unwrap() error { return e.err }
218+
219+
// Timeout implements net.Error.
220+
func (e *cipherRestrictError) Timeout() bool { return false }
221+
222+
// Temporary implements net.Error. We need to set this to true since
223+
// http/server.go:func Serve(l net.Listener) error
224+
// https://github.com/golang/go/blob/go1.23.7/src/net/http/server.go#L3329-L3349
225+
// uses this value to resume serving on the connection without explicitly
226+
// registering an error. As mentioned in net/net.go Error interface temporary
227+
// errors are deprecated and may not be supported in the future:
228+
// https://github.com/golang/go/blob/go1.23.7/src/net/net.go#L419, hence we need
229+
// a check that cipherRestrictError always implements the net.Error interface
230+
// fully.
231+
// TODO(souravcrl): update the accept handler to just log an error and continue
232+
// processing new connection requests
233+
func (e *cipherRestrictError) Temporary() bool { return true }
234+
235+
var _ error = (*cipherRestrictError)(nil)
236+
237+
// We implement net.Error the same way that context.DeadlineExceeded does, so
238+
// that people looking for net.Error attributes will still find them.
239+
var _ net.Error = (*cipherRestrictError)(nil)

0 commit comments

Comments
 (0)