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.

Sign up for GitHub

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

Jump to bottom

server: unauthenticated http requests running as root by default? #45018

Open
andreimatei opened this issue Feb 12, 2020 · 5 comments
Open

server: unauthenticated http requests running as root by default? #45018

andreimatei opened this issue Feb 12, 2020 · 5 comments
Labels
A-authentication Pertains to authn subsystems A-security A-server-architecture Relates to the internal APIs and src org for server code C-investigation Further steps needed to qualify. C-label will change. T-server-and-security DB Server & Security

Comments

@andreimatei
Copy link
Contributor

andreimatei commented Feb 12, 2020 •
edited by cockroach-jira-scripts
Loading

I was looking into the /health endpoint requiring authentication since recently, and wondering how come I still seem able to access it from my browser.

It turns out it's because of this code that says that if there's an "incoming context" with metadata but no user, then the user is root by default because it must be " a gRPC / internal SQL connection which has root on the cluster" - which is not true; I'm doing an https request from outside the cluster.

This happens to save us for the /health request, which doesn't actually want authentication, but is it good for all other endpoints?

I've tested this on an --insecure cluster. Perhaps that plays a role?

Jira issue: CRDB-5180
@andreimatei andreimatei added the C-investigation Further steps needed to qualify. C-label will change. label Feb 12, 2020
@andreimatei
Copy link
Contributor Author

andreimatei commented Feb 12, 2020 •
edited
Loading

I've tested this on an --insecure cluster. Perhaps that plays a role?

Nvm, same on secure.

But /_status/health refuses my request and wants a cooking. I've got no idea what's going on.

@knz
Copy link
Contributor

knz commented Feb 12, 2020

thanks for finding this out. Will look into it.

@andreimatei andreimatei mentioned this issue Feb 12, 2020
Closed
@knz knz added the A-security label Feb 12, 2020
@knz
Copy link
Contributor

knz commented Feb 14, 2020

Found it: the alias /health is manually opted out from authentication elsewhere in the code. Further discussion here: #45020 (comment)

Let's close this issue and discuss further there.

@knz knz closed this as completed Feb 14, 2020
@andreimatei andreimatei mentioned this issue Feb 14, 2020
Closed
@knz
Copy link
Contributor

knz commented Feb 14, 2020

discussed with andrei: non-auth requests should not report they are runnign as root

@knz knz reopened this Feb 14, 2020
@knz knz mentioned this issue Feb 14, 2020
Closed
@jlinder jlinder added the T-server-and-security DB Server & Security label Jun 16, 2021
@knz knz added A-server-architecture Relates to the internal APIs and src org for server code A-authentication Pertains to authn subsystems labels Jul 29, 2021
@knz knz mentioned this issue Feb 2, 2023
Merged
@knz
Copy link
Contributor

knz commented Feb 2, 2023

We've clarified the mechanics that motivate the current behavior in this tech note: #96427.

The code should still be changed/improved, but at least this way we know where we are starting from.

craig bot pushed a commit that referenced this issue Feb 3, 2023
@knz
Merge #96451
682ad1b 
96451: server: only forward the SQL identity in gRPC metadata r=andreimatei a=knz

Requested by `@andreimatei` .
Informs #96427.
Informs #45018.

Prior to this patch, we were forwarding any and all gRPC metdata during a RPC fanout. This was creating doubt and confusion, about how much data is really important/useful to forward.

Analysis suggests we only care about the SQL user identity resulting from HTTP authentication. So this patch limits the forwarding to just that information.

This specialization makes the forwarding logic easier to understand.


This patch additionally renames functions as follows:

| Old name                        | New name                              |
|---------------------------------|---------------------------------------|
| `userFromContext`               | `userFromIncomingRPCContext`          |
| `getSQLUsername`                | `userFromHTTPAuthInfoContext`         |
| `apiToOutgoingGatewayCtx`       | `forwardHTTPAuthInfoToRPCCalls`       |
| `forwardAuthenticationMetadata` | `translateHTTPAuthInfoToGRPCMetadata` |
| `propagateGatewayMetadata`      | `forwardSQLIdentityThroughRPCCalls`   |


Release note: None
Epic: None

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@gtr gtr mentioned this issue Jul 12, 2023
Open
@celiala celiala unassigned knz Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-authentication Pertains to authn subsystems A-security A-server-architecture Relates to the internal APIs and src org for server code C-investigation Further steps needed to qualify. C-label will change. T-server-and-security DB Server & Security
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

server: avoid considering non-authenticated reqs as root-authenticated knz/cockroach
3 participants
@jlinder @andreimatei @knz