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

Add LocalChannelCredentials and LocalServerCredentials, with UDS support #9900

Open
carl-mastrangelo opened this issue Feb 17, 2023 · 13 comments
Milestone

Comments

@carl-mastrangelo
Copy link
Contributor

Using version gRPC 1.52.1

When looking at the transport attributes, from a ServerInterceptor, on a Netty Server, with a KQueue (or Epoll) channel, I see the following:

{io.grpc.internal.GrpcAttributes.securityLevel=NONE, io.grpc.Grpc.TRANSPORT_ATTR_REMOTE_ADDR=null, io.grpc.Grpc.TRANSPORT_ATTR_LOCAL_ADDR=null}

This issue is actually two related requests:

  1. When using a domain socket with KQueue or Epoll, the security level should be PRIVACY_AND_INTEGRITY. I don't believe it is possible to snoop on the data going back and forth without root privileges. There doesn't appear to be a lot of information about how these values should be interpreted. For context, I am calling my gRPC Java server from the grpc_cli tool, which treats UDS channels as a credentialed connection.

  2. The remote and local addresses should be set from the parent socket for domain. Unlike regular Inet sockets, Netty doesn't set the address on the child channel for domain sockets on the server side. Netty only sets the addresses on bind and connect calls, which don't happen on the server side (the corresponding Epoll Socket channels, and Nio Socket Channels set the values more eagerly). Arguably this is a Netty bug, but it would be reasonable to work around this on the gRPC side.

One other passing thought: Connections over the loopback adapter for Inet sockets could also be reasonably treated as secure as well. gRPC C-core treats connections where both sides are loopback as special, which is nice. However, unlike domain sockets, they aren't "authenticated" IMO, which is mentioned in the docs for SecurityLevel.

@ejona86
Copy link
Member

ejona86 commented Feb 23, 2023

For (1), yeah the security level should be PRIVACY_AND_INTEGRITY, although that is determined by the credential. So for plain-text I'd expect it to be insecure cross-language. I think in C you have to use the "local" credential[1] to get the "is it UDS" logic (as well as "is it localhost"). Having such a credential would be helpful for in-process as well.

For (2), are you wanting the local or remote address? For remote address on server-side, there really isn't one except maybe the remote process id? The full peer information (PeerCredentials in Netty) is what you'd probably use instead and we would want to expose that, although you could see that as authentication information. This too should probably be part of "local" credentials. Dunno which fields it'd use for what, but it'd be worth checking other language's behavior.

  1. grpc::experimental::LocalCredentials and LocalServerCredentials

@ejona86 ejona86 added this to the Next milestone Feb 23, 2023
@carl-mastrangelo
Copy link
Contributor Author

Re (1): I think that matches expectation. I think later versions of Java have UDS support built in, so having gRPC treat conns as PRIVACY_AND_INTEGRITY would be good. I only saw a TLS (and ALTS) credential when I looked through IntelliJ. If there were PRIVACY_AND_INTEGRITY Credentials for:

  • Localhost / 127.0.0.1 / [::1]. as a LocalCredential
  • Netty Domain Sockets / Java Domain sockets as LocalCredential
  • Inprocess be a InprocessCredential

That would help a lot.

One other request: ServerCall.getSecurityLevel. doesn't appear to be implemented. If this is expected to stabilize over time, I'd expect this to fetch it from the transport attributes. Currently the Security level can be fetched from the transport attributes, but the Attribute key is in io.grpc.internal.

Re (2): remote / local is kinda vague, but as long as one of them is set. For me at least, it is for debugging mainly. Having the remote user / pid be part of that would be interesting, esp as part of the Credential check. But just getting anything would be good enough for me.

@ejona86
Copy link
Member

ejona86 commented Feb 23, 2023

ServerCall.getSecurityLevel. doesn't appear to be implemented.

Seems like it is:

public SecurityLevel getSecurityLevel() {
final Attributes attributes = getAttributes();
if (attributes == null) {
return super.getSecurityLevel();
}
final SecurityLevel securityLevel = attributes.get(ATTR_SECURITY_LEVEL);
return securityLevel == null ? super.getSecurityLevel() : securityLevel;
}

@ejona86 ejona86 changed the title Transport Attributes for Domain sockets missing Add LocalChannelCredentials and LocalServerCredentials, with UDS support Feb 23, 2023
@ejona86
Copy link
Member

ejona86 commented Feb 23, 2023

I've retitled this issue (local credential) and split out the missing addresses to #9910.

@ejona86
Copy link
Member

ejona86 commented Feb 23, 2023

At a bare minimum for this to be considered done we should have the local credential require localhost or UDS, and set the security level to PRIVACY_AND_INTEGRITY. The behavior should agree with C/Go.

In-process would be a nice-to-have, but would be fine to file an issue and make a follow-up. Once local credentials are available, it should be pretty easy though, as it is mostly just plumbing the Channel/ServerProvider which doesn't currently exist.

@carl-mastrangelo
Copy link
Contributor Author

Seems like it is:

Hmm, I just double checked, The problem is that I am using ServerInterceptors, which typically don't forward the call security. SimpleForwardingServerCall, ForwardingServerCall, and PartialForwardingServerCall don't forward it. Several interceptors don't override the method either.

@ejona86
Copy link
Member

ejona86 commented Feb 23, 2023

Yep, looks like it was missed from #8943. It should be part of PartialForwardingServerCall, so none of the others would need to care. Do you want to do that fix, or have me do it?

@ejona86
Copy link
Member

ejona86 commented Feb 24, 2023

This has been requested internally to Google (b/201682307). It was lower priority than other languages because of the very high usage of in-process for testing.

Go has helpful documentation about the behavior. (For comparison, C++'s dearth). C++ implemention.

Notably C++ compares the local address against 127.0.0.1 for IPv4, whereas Go compares the remote address against 127.x.x.x. For Java, I think we should do the same as Go; that's the behavior I expected and Inet4Address.isLoopbackAddress() is available. A quick test on my Linux machine shows 127.0.0.1 is the default source address for a connection to 127.0.0.2, so I expect for most people the difference is academic.

C++ treats both UDS and localhost as PRIVACY_AND_INTEGRITY whereas Go only treats UDS and PRIVACY_AND_INTEGRITY. We'll need that to get that aligned.

@ejona86
Copy link
Member

ejona86 commented Feb 24, 2023

Huh, the same person implemented both, but I don't see any issue to fix one of the implementations.
grpc/grpc#17370
grpc/grpc-go#3517

@carl-mastrangelo
Copy link
Contributor Author

Alignment between impl's sounds good. Remote being loopback seems like a good litmus for being local. (I could imagine if the src and dst inet addresses were the same, that might be "local" too, but meh). Maybe LoopbackCredential might be a more precise name?

@ejona86
Copy link
Member

ejona86 commented Mar 3, 2023

Assigning to erm-g to help figure out what we want the behavior to be. After we have that, someone else can pick it up.

@ejona86
Copy link
Member

ejona86 commented Mar 3, 2023

Mark noticed this on the PR for C's implementation:

Local TCP channel is associated with GRPC_PRIVACY_AND_INTEGRITY security level in order to unbreak internal use cases if exist, and I added a TODO to lower its security level to GRPC_SECURITY_NONE.

And so that explains why Go considers it no security. We should consider it no security in Java.

@carl-mastrangelo
Copy link
Contributor Author

carl-mastrangelo commented Mar 3, 2023

Hmm, I hadn't thought through it that much, but after reading the gRFC, it's not clear what the security level means from the server. As described, the security is for use by the call credentials, due to having authenticated (authorized?) the remote endpoint.

However, servers seldom do this. The vast majority of public TLS servers accept a client from anywhere (i.e. without MTLS verification). The server may be using a connection that can't be tampered with, but PRIVACY_AND_INTEGRITY doesn't imply trust. I'd rather not get into what the security level really means, and just scope this request to getting the file name of the socket.

(okay let's get into it a little) I have a gRPC Java server running on GCP's Cloud Run. Cloud Run creates a TLS LB for the container, and forwards the traffic to the container over h2c. The firewall rules are setup so that no other external connections can be made to the container. The only way for a request to show up is via the secure LB. Inside the container, the "connection" appears to be coming from a 169.254.0.0/16 address. Since the server knows it is running in Cloud Run, we can reasonably assume this connection is providing privacy and integrity. But, from a library point of view, its a plain, insecure connection. Should the server send back sensitive data? We can't tell purely from the SecurityLevel any more. Our server could do some ServerTransportFilter tricks, but to what end? It would be the same as sending sensitive data anyway and ignoring the security level, if we are the ones setting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants