server,ui: a logged-in user without admin privileges can perform any admin-only operation

[knz]

tldr:

1. as far as we know right now, up to and including version 2.1.9, 19.1.5 and 19.2.1, any non-admin authenticated user can call any admin endpoint, even if they should be admin-only operations, as long as the endpoint is visible over HTTP (which is the case for all of them today). It makes it possible for non-admin users to shut down a node or view SQL objects on which they have no permission.

2. a fix is included in 19.2.2 (release-19.2: server: properly authenticate some admin RPC req… #42726), 19.1.6 (release-19.1: server: properly authenticate some admin RPC req… #42727) and 2.1.10 (release-2.1: server: properly authenticate some admin RPC requ… #42910).

3. release note updates: Document the fallout from crdb PR #42563 docs#6321

Details:

A CockroachDB server exposes "admin RPCs" to control the cluster, like Drain() for quitting a node or Decommission() to decommission a node.
It also exposes the SQL metadata such as the list of databases, tables, settings etc.

These RPCs are accessible over two separate protocols:

• over gRPC
• over HTTP via the /admin/_v1 prefix.

Currently, the gRPC request handler verifies that a request is issued via the root user, by checking the CN of the provided client cert (or ignoring the info and assuming root if the server is running insecure). So far, so good.

The HTTP handler however does not verify anything. It merely requires a valid web authentication cookie, but this cookie can be generated for a non-root user. It is up to the admin RPC functions to do authn/authz, and they currently don't. Instead it is assumed that all admin RPCs are allowed and run as the root user.

This is OK with an insecure server, but for a secure server it creates a possible problem: a client user who has access to the HTTPS endpoint and a valid auth cookie, even as a non-admin user, can use that to establish a HTTPS connection to the node, and run an admin-only operation.

To exploit this in practice:

• for the SQL metadata, this is pretty simple: the current code serves the UI to any authenticated user as if they were root, so the user can see pretty much everything without restriction.

• for the Drain (quit) RPC and other RPCs without a UI page, it is a bit cumbersome: a user would need to manually craft the proper request packet payload using protobuf, copy-paste their auth cookie from the browser into their custom client and send that in their HTTPS request. We do not provide a library to do that, so it would be hard for a human user to get there by accident. However a dedicated programmer can achieve this pretty easily.

This concern is relatively new because prior to non-root UI access (introduced, IIRC, in v2.0) we were telling users "Don't expose the UI to non-admin users at the network level". When we started advertising UI authentication, we did not check the admin RPCs for authn/authz.

[knz]

Comment from @mberhault :

I think we should make sure that the first thing done to an incoming request is authenticate the user and attach it to the context. Then, something like userFromContext would just say if user is not set: error out otherwise return username
This is mostly done on grpc. For the admin endpoint, I think we strictly say: "must be authenticated and must be root". But we should have this kind of check everywhere.

[knz]

Looking at the code:

• for "unary" RPCs it's possible to do what marc suggests and pick up the RPC auth username in userFromContext like suggested
• for streaming RPCs, like Drain() that's not possible because there is no context to work with. I'm not sure how to proceed to authenticate Drain().

One possible way forward would be to remove all the non-unary RPCs from the HTTP endpoint. Today that's just Drain(). I believe I can do that simply by removing the option (google.api.http) from the RPC definition.
@mberhault thoughts?

[knz]

After I wrote the previous comment, @ajwerner explains:

The idiomatic way you can change the context using a stream interceptor would be to do it by wrapping the implementation of the ServerStream with a new concrete implementation which embeds the old one and overrides the Context method.

[that said,] I really dislike this approach.

I think the better approach would be to do this context injection in the stats.Handler https://godoc.org/google.golang.org/grpc/stats#Handler
The question here is whether you at that point have adequate information to add the metadata you want to attach

[bdarnell]

I believe I can do that simply by removing the option (google.api.http) from the RPC definition.

Yeah, there's no reason to expose drain and decommission over HTTP. Leave them GRPC-only.

[knz]

I went over the code and maybe the scope of the issue is larger than initially thought?

I thought the problem was restricted to the admin RPCs because the code in status.go was already extracting the username properly.

However the status RPCs only authorize those RPCs that use SQL (via SQL privilege checks), and some RPCs do not use SQL. Those RPCs are, today, also unauthorized. This includes:

• Certificates - woops?
• Nodes, NodesWithLiveness, Gossip, Details - data leak, do we want to make IP addresses available to every authenticated user?
• EngineStats - perhaps DoS, it's a cluster-wide RPC that's resource-intensive
• Range - data leak, it exposes range start/end keys
• CancelSession - DoS
• CancelQuery - DoS

The following RPCs are gated behind the debug cluster setting, so are not enabled by default, but when they are, are also not authorized:

• LogFile, Logs, GetFiles - woops?
• Ranges, HotRanges, Allocator, AllocatorRange - data leak, it exposes range start/end keys
• Stacks, Profile - maybe DoS, they are resource-intensive

[knz]

Marc has proposed a more thorough solution:

• very early on, store the authenticated username in the context (for HTTP this is already done; for gRPC conns, we'd add this when the conn is established, ajw explained me how)

• go through all the methods and add the appropriate authorization for that method at the top regardless of whether it uses SQL.

I would really like to have a point in the code that's common to all HTTP requests, to validate the URL against a whitelist. Maybe in the cmux? Suggestions?

[mberhault]

The first point (store authenticated username) is more for our own sanity: we want to make sure every incoming request regardless of protocol (gRPC vs HTTP) has authentication information. I think reducing the scope of the admin service is probably a good idea as well.

The second point is really what Admin UI V2 would have done (and arguably what V1 should have done).

In the short-term: using the session user to execute SQL is acceptable, and we can do arbitrary authorization checks for non-sql functionality (eg: require SQL admin for something like DecommissionNode).

In the longer term, we need privileges specific to the Admin UI. eg: even if you're not a SQL admin, you may have the right to view the list of users or jobs etc... This is a much bigger set of features though.

About a few of the endpoints:

• certificates: it's fine, certificates are the public component. We never return the key. Anyone can fetch the server certificate by just asking for it, it's part of connection setup. Client certs only tell you the username it's for and there usually aren't too many of those on a node.
• anything with range information: range keys are definitely "data" and should obey SQL level privileges
• log files: eep. Yes, that has a lot of range keys listed.
• DOS risks: I don't think we've reached a multi-user deployment size where this is a huge concern, but why expose those to non-admins, they're mostly meant for operators.

[knz]

So we're lucky Decommission and DecommissionStatus were not exposed via HTTP so that's clear.

[knz]

For the Certificates the thing returns all client certs in the certs dir, including the certs for other users than the authenticated user, so I think it's not safe.

[knz]

Heads up that I have updated #42563 to close all the gaps identified above. So that PR will now get us a step further.

[mberhault]

It's not ideal if client certs are on the node but they have no reason to be, the node doesn't need them.
Still, the endpoint is only useful to operators, there's no reason to leave it open to anyone else.

[knz]

The backports to 19.1 and 19.2 have been issued.

What's left to do:

• close the gap by filling auth info as suggested by Marc and ajw
• implement whitelist-based authz (all RPCs rejected for non-admin unless opted in, as guardrail for future extensions)
• re-do the fix for 2.1 (PR server: properly authenticate some admin RPC requests #42563 cannot apply on 2.1) release-2.1: server: properly authenticate some admin RPC requ… #42910

[israellot]

Core version now has admin ui broken, since we can't read logs as the api returns it's an admin only operation, but we can't assign admin role to any user as it's an enterprise feature.

[knz]

Yes thanks we found this out yesterday. Let's continue this discussion in #43847 #43870 and #44033.

[knz]

The issue described at the top has been fixed in the linked PRs. Closing this now; the follow-up cleanup and strengthening work is described here: #44150