Add a util function to check if a transport is secure enough to transfer CallCredentials #6616

yihuazhang · 2020-01-17T00:16:06Z

This PR adds a util function that checks if the security level of transport is higher than or equal to that of CallCredentials. The util function is called GoogleAuthLibraryCallCredentials and FileBasedPluginCredential for which I believe privacy+integrity should be required for the transport. After this PR gets merged, I will update the internal CallCredentials implementation to call the util function (for the auditing purpose).

gRFC for security level negotiation: grpc/proposal#167

sanjaypujare · 2020-01-17T00:41:14Z

xds/src/main/java/io/grpc/xds/sds/FileBasedPluginCredential.java

@@ -90,6 +91,12 @@ private static DataSource buildDataSourceFromConfigStruct(Struct secretValueStru
  @Override
  public void applyRequestMetadata(
      @Nullable RequestInfo requestInfo, Executor appExecutor, final MetadataApplier applier) {
+    if (!checkSecurityLevel(requestInfo, SecurityLevel.PRIVACY_AND_INTEGRITY)) {


So this is currently used for SDS which is over an UDS channel. Will this check now break the SDS communication or is UDS considered at SecurityLevel.PRIVACY_AND_INTEGRITY ?

UDS should be considered secure (i.e., SecurityLevel.PRIVACY_AND_INTEGRITY). Do you know if the security level is correctly set for the UDS channel?

Don't know. If not, we will have to do that before merging this change to ensure this feature doesn't break. Let me know if you want to discuss

Agreed. If I read the code correctly, it seems UDS channel is implemented as a Netty plaintext channel whose security level is NONE and I believe it is the cause of test failure. Is it possible to associate the security level PRIVACY_AND_INTEGRITY to the plaintext channel when used for UDS? Let me schedule a meeting to discuss about it if it is not trivial.

First of all, is it appropriate to associate PRIVACY_AND_INTEGRITY to FileBasedPluginCredential?

I believe that's the only way to use UDS in grpc-java/Netty. Not sure about your question "Is it possible to associate the security level PRIVACY_AND_INTEGRITY to the plaintext channel when used for UDS?" but I suppose it should be possible. CC @ejona86

First of all, is it appropriate to associate PRIVACY_AND_INTEGRITY to FileBasedPluginCredential?

Good question. This FileBasedPluginCredential mimics Envoy's similar credential https://github.com/envoyproxy/envoy/blob/master/source/extensions/grpc_credentials/file_based_metadata/config.cc which is received by Istio Node Agent (https://github.com/istio/istio/blob/master/security/pkg/nodeagent/sds/sdsservice.go#L509:6) . I think this is loaded from a secure and privileged file location so it is fair to assume it needs a secure channel (but we need to think about it).

sanjaypujare · 2020-01-17T00:43:52Z

One test failed:

io.grpc.xds.sds.SdsClientUdsFileBasedMetadataTest > testSecretWatcher_tlsCertificate FAILED
    expected to be true
        at io.grpc.xds.sds.SdsClientUdsFileBasedMetadataTest.testSecretWatcher_tlsCertificate(SdsClientUdsFileBasedMetadataTest.java:105)

This indicates the code change will also break prod behavior. Did the test pass in your testing?

ejona86 · 2020-01-17T19:03:34Z

api/src/main/java/io/grpc/CallCredentials.java

+   * @param requestInfo request-related information
+   * @param minSecurity minimum security level required by {@link CallCredentials}
+   */
+  public final boolean checkSecurityLevel(RequestInfo requestInfo, SecurityLevel minSecurity) {


check makes it seem like this will throw. Maybe name it allowedSecurityLevel or fulfullsSecurityLevel or similar that makes it more obvious it returns a boolean?

CC @jiangtaoli2016

ejona86 · 2020-01-17T19:03:49Z

api/src/main/java/io/grpc/CallCredentials.java

+   * @param requestInfo request-related information
+   * @param minSecurity minimum security level required by {@link CallCredentials}
+   */
+  public final boolean checkSecurityLevel(RequestInfo requestInfo, SecurityLevel minSecurity) {


yihuazhang · 2020-01-17T23:00:49Z

@ejona86 @sanjaypujare

Is it POR for using a plaintext channel for UDS? If that's the case, I have a following plan to associate the security level PRIVACY_AND_INTEGRITY to a UDS channel. PLMK what you think.

Add a new NegotiationType - UDS.
In usePlaintext(), check if the server's SocketAddress is an instance of DomainSocketAddress and if it is, set the negotiation type to UDS. We do not allow users to specify if the channel is UDS or not and by doing that, we can avoid their intentional (or unintentional) mistakes.
Create a new API - uds() in ProtocolNegotiator and add a new constructor for PlaintextProtocolNegotiator - PlaintextProtocolNegotiator(SecurityLevel). For UDS, the security level will be PRIVACY_AND_INTEGRITY.
Update security level attribute accordingly.

sanjaypujare · 2020-01-19T19:31:42Z

@ejona86 @sanjaypujare

Is it POR for using a plaintext channel for UDS?

This is dictated by the other end - the SDS server (Citadel agent) only supports UDS based plaintext channel. See https://istio.io/docs/tasks/security/citadel-config/auth-sds/#securing-sds-with-pod-security-policies (instead of Envoy sidecar, read it as proxyless gRPC client).

If that's the case, I have a following plan to associate the security level PRIVACY_AND_INTEGRITY to a UDS channel. PLMK what you think.

Comments below:

Add a new NegotiationType - UDS.

NegotiationType is meant to indicate 'negotiation used for starting up HTTP/2' so it is not appropriate to include transport characteristics here.

In usePlaintext(), check if the server's SocketAddress is an instance of DomainSocketAddress and if it is, set the negotiation type to UDS. We do not allow users to specify if the channel is UDS or not and by doing that, we can avoid their intentional (or unintentional) mistakes.

What if user called forTarget so there is no directServerAddress (SocketAddress) set? You might be able to check unix: prefix in the target string but the bigger problem is using negotiationType to convey transport.

Create a new API - uds() in ProtocolNegotiator and add a new constructor for PlaintextProtocolNegotiator - PlaintextProtocolNegotiator(SecurityLevel). For UDS, the security level will be PRIVACY_AND_INTEGRITY.

Update security level attribute accordingly.

Looking at this attribute code it might be better to use the ChannelHandlerContext that is passed in fireProtocolNegotiationEvent (for example) and you call ChannelHandlerContext.channel() and on that channel get remoteAddress() to see what kind of server address it is. This is better than overloading NegotiationType .

ejona86 · 2020-07-16T18:25:37Z

Add a new NegotiationType - UDS.

No, I don't think we want to do that. It would be strictly better to check for DomainSocketAddress at NettyChannelBuilder.createProtocolNegotiatorByType(). We can either pass in the SocketAddress to the function or make it non-static.

That said, I'd really prefer a solution that works with a future UnixNameResolver. We can move the DomainSocketAddress check to PlaintextProtocolNegotiator. Unfortunately, it wouldn't be guaranteed to be authenticated in that case, since maybe a MyCustomNameResolver returns unauthenticated DomainSocketAddresses. We could require the filename also matches the authority (and use INSECURE if it is not).

xds/src/main/java/io/grpc/xds/sds/FileBasedPluginCredential.java

jiangtaoli2016 · 2020-07-17T15:31:35Z

@yihuazhang Could you please resolve the conflict

yihuazhang · 2020-08-07T15:00:31Z

@ejona86 @sanjaypujare PTALA

ejona86 · 2020-08-07T15:05:15Z

api/src/main/java/io/grpc/CallCredentials.java

+   * @param requestInfo request-related information
+   * @param minSecurity minimum security level required by {@link CallCredentials}
+   */
+  public static final boolean allowedSecurityLevel(


Thinking about it more, let's make this protected. Only extending classes will need to call this. It wouldn't really hurt anything by having it public, but pollutes what consumers of CallCredentials would see.

larry-safran · 2023-09-28T17:53:35Z

@yihuazhang Would you please sign the EasyCLA so that this can be merged?

yihuazhang requested a review from ejona86 January 17, 2020 00:16

sanjaypujare reviewed Jan 17, 2020

View reviewed changes

ejona86 reviewed Jan 17, 2020

View reviewed changes

yihuazhang force-pushed the security_level_negotiation branch from cb5daa4 to 1a27000 Compare January 17, 2020 22:43

ejona86 approved these changes Jul 16, 2020

View reviewed changes

ejona86 requested changes Jul 16, 2020

View reviewed changes

xds/src/main/java/io/grpc/xds/sds/FileBasedPluginCredential.java Outdated Show resolved Hide resolved

Add a util function to check security level

2db80cd

yihuazhang force-pushed the security_level_negotiation branch from 1a27000 to 2db80cd Compare July 17, 2020 16:10

ejona86 approved these changes Aug 7, 2020

View reviewed changes

larry-safran added the Waiting on reporter there was a request for more information without a response or answer or advice has been provided label Sep 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a util function to check if a transport is secure enough to transfer CallCredentials #6616

Add a util function to check if a transport is secure enough to transfer CallCredentials #6616

yihuazhang commented Jan 17, 2020

sanjaypujare Jan 17, 2020

yihuazhang Jan 17, 2020

sanjaypujare Jan 17, 2020

yihuazhang Jan 17, 2020

yihuazhang Jan 17, 2020

sanjaypujare Jan 17, 2020

sanjaypujare Jan 17, 2020

sanjaypujare commented Jan 17, 2020

ejona86 Jan 17, 2020

ejona86 Jan 17, 2020

yihuazhang commented Jan 17, 2020

sanjaypujare commented Jan 19, 2020

ejona86 commented Jul 16, 2020

jiangtaoli2016 commented Jul 17, 2020

yihuazhang commented Aug 7, 2020

ejona86 Aug 7, 2020

larry-safran commented Sep 28, 2023

Add a util function to check if a transport is secure enough to transfer CallCredentials #6616

Are you sure you want to change the base?

Add a util function to check if a transport is secure enough to transfer CallCredentials #6616

Conversation

yihuazhang commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjaypujare commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuazhang commented Jan 17, 2020

sanjaypujare commented Jan 19, 2020

ejona86 commented Jul 16, 2020

jiangtaoli2016 commented Jul 17, 2020

yihuazhang commented Aug 7, 2020

Choose a reason for hiding this comment

larry-safran commented Sep 28, 2023