kvserver: authN and authZ for incoming tenant connections

[tbg]

See here for background

SQL tenant servers will make outgoing connections, most prominently to the KV layer. We need to come up with way to encrypt and authenticate these connections.

This is still in the design phase but generally will be based on some crypto. The design will apply more broadly than to the SQL tenant connections - this issue tracks only the work to apply it to these connections.

An internal, early, work in progress strawman is here.

The auth broker is tracked in #49105.

[nvanbenschoten]

Per our discussion today, it does look like gRPC supports authentication on connection startup. From https://grpc.io/docs/guides/auth/:

Credentials can be of two types:

• Channel credentials, which are attached to a Channel, such as SSL credentials.
• Call credentials, which are attached to a call (or ClientContext in C++).

I think you're aware of this because you use both in https://github.com/cockroachdb/cockroach/compare/master...tbg:poc/tokenauth?expand=1#diff-14fd680bac02bef9b58a5fd4fed0ff8bR141. So it seems like it should be possible to include the tenant auth token during connection startup and avoid passing an extra 64 bytes on each individual RPC.

There would be some complication around wrapping a tlsCreds (return value of credentials.NewTLS) with our own TransportCredentials implementation that adds the auth token to the initial handshake, but this all seems tractable.

[tbg]

Oh, nice. I will give that a try. Thanks!

[tbg]

That looks like it should work, but it goes back to "why don't we just use client certificates"? It really starts smelling like we're getting into a rebuild of client certs. It looks like maybe we should - the real difficulty with client certs (or equivalently your proposal) is that the expiration check needs to happen per-RPC (plus a little work on the RPCContext to proactively close conns that are close to expiring). I think we can make information from the conn available to the per-RPC interceptors though. Everything there is marked as experimental, but let's assume that's fine:

The per-RPC interceptor can call

https://pkg.go.dev/google.golang.org/grpc/credentials?tab=doc#RequestInfoFromContext

which should really be a *TLSInfo:

https://pkg.go.dev/google.golang.org/grpc/credentials?tab=doc#TLSInfo

which contains AuthInfo which contains TLSConnectionState

https://pkg.go.dev/crypto/tls?tab=doc#ConnectionState

which has the VerifiedChains which are x509.Certificates:

https://pkg.go.dev/crypto/x509?tab=doc#Certificate

and those have the NotBefore and NotAfter. The tenantID can be put into https://pkg.go.dev/crypto/x509/pkix?tab=doc#Name

I tried this out just now and immediately got a reality check. We're on gRPC v1.21.2, but all of this API is just four months old at the time of writing, and (I think, judging from the milestone they gave the PR) is in gRPC v1.27. Possibly good news is that the current release is v1.29 and there's already a v1.27.1 (so v1.27 is possibly a reasonable candidate to upgrade to) but it leaves me uneasy to have a gRPC upgrade in the critical path here.

@petermattis I would be curious how anxious you are about upgrading gRPC. It is about the right time in the cycle to do so, yet it's unclear that we have a strong incentive (aside from this issue). There are a few bug fixes/improvements possibly worth picking up:

• client: fix race between client-side stream cancellation and compressed server data arriving (client: fix race between client-side stream cancellation and compressed server data arriving grpc/grpc-go#3054)
• clientconn: fix potential deadlock caused by ResetConnectBackoff (clientconn: fix potential deadlock caused by ResetConnectBackoff grpc/grpc-go#3051)
• internal: fix context leak when stream is not created successfully (internal: fix context leak when stream is not created successfully grpc/grpc-go#2985)
• server: set and advertise max frame size of 16KB (server: set and advertise max frame size of 16KB grpc/grpc-go#3018)
• transport: block reading frames when too many transport control frames are queued (transport: block reading frames when too many transport control frames are queued grpc/grpc-go#2970)
  Addresses CVE-2019-9512 (Ping Flood), CVE-2019-9514 (Reset Flood), and CVE-2019-9515 (Settings Flood).
• server: avoid an unnecessary allocation per-RPC for OK status (server: avoid an unnecessary allocation per-RPC for OK status grpc/grpc-go#2920)
  Special Thanks: @dzbarsky
• server: avoid call to trace.FromContext and resulting allocations when tracing is disabled (server: avoid call to trace.FromContext and resulting allocations when tracing is disabled grpc/grpc-go#2926)
  Special Thanks: @dzbarsky
• http2client: remove unnecessary allocations for header fields (http2client: remove unnecessary allocations for header fields grpc/grpc-go#2925)
  Special Thanks: @dzbarsky
• status: avoid allocations when returning an OK status (status: avoid allocations when returning an OK status grpc/grpc-go#2929)
  Special Thanks: @dzbarsky
• server: avoid allocations related to tracking excessive pings (server: avoid allocations related to tracking excessive pings grpc/grpc-go#2923)
  Special Thanks: @dzbarsky
• transport: call Unlock in defer to avoid data race (transport: call Unlock in defer to avoid data race grpc/grpc-go#2953)
• client: fix canceled vs deadline exceeded double-check logic (client: fix canceled vs deadline exceeded double-check logic grpc/grpc-go#2906)
• client: fix race between transport draining and new RPCs (client: fix race between transport draining and new RPCs grpc/grpc-go#2919)

[tbg]

Ha! Double-good news. I missed another API that gives us this already, which we are already using today:

cockroach/pkg/rpc/context.go

Lines 122 to 123 in f80ec9c

	} else if peer, ok := peer.FromContext(ctx); ok {
	if tlsInfo, ok := peer.AuthInfo.(credentials.TLSInfo); ok {

So it doesn't seem like we need to upgrade after all.

[petermattis]

@petermattis I would be curious how anxious you are about upgrading gRPC. It is about the right time in the cycle to do so, yet it's unclear that we have a strong incentive (aside from this issue). There are a few bug fixes/improvements possibly worth picking up:

I have no objections to upgrading grpc. This would be the right time to do an upgrade.

[tbg]

Ok, I opened a PR: #49114

Note that we don't care about it for this issue.

[tbg]

I added a commit to https://github.com/tbg/cockroach/tree/poc/tokenauth that prototypes the use of client certs using mostly cobbled together pieces of our existing security package. I verified that the *Peer lets us see the tenant ID and expiration in the server-side interceptors.

I must say that the security package is pretty hostile to reuse, but that can be fixed. After that, we'll be using pretty much our existing infra throughout, which is nice.

[tbg]

Reminder to self: as we do this, also make sure the RPC heartbeats between SQL and KV stop checking the cluster name, and can never bring down a KV node due to clock mismatch.