netty: Per-rpc call option authority verification against peer cert subject names #11724

kannanjgithub · 2024-12-04T07:50:08Z

No description provided.

examples/example-tls/build.gradle

util/src/main/java/io/grpc/util/CertificateUtils.java

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

ejona86 · 2024-12-05T15:40:20Z

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

+      }
+      if (!peerVerified) {
+        return new FailingClientStream(Status.INTERNAL.withDescription(
+            "Peer hostname verification failed for authority"), tracers);


At the very least, we will want to include the failed authority in this error message. It'd probably be good to include e from that exception above as well.

ejona86 · 2024-12-05T15:45:27Z

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

@@ -105,6 +112,8 @@ class NettyClientTransport implements ConnectionClientTransport {
  private final ChannelLogger channelLogger;
  private final boolean useGetForSafeMethods;
  private final Ticker ticker;
+  private final ConcurrentHashMap<String, Boolean> authoritiesAllowedForPeer =


We'll want to limit the maximum size of this map. That may mean we swap it to a LinkedHashMap and protect it with a lock, so we can drop LRU.

Should the max size be hardcoded to 10000? We don't need it to be configurable via channel params, right?

I think hard-coding it to 100 would be fine. 10 is probably sufficient, so 100 should be plenty. If it is exceeded, it is almost certainly an unbounded number of authorities.

ejona86 · 2024-12-05T15:50:21Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+          x509ExtendedTrustManager = tlsCreds.getTrustManagers().stream().filter(
+              trustManager -> trustManager instanceof X509ExtendedTrustManager).findFirst();
+        } else if (tlsCreds.getRootCertificates() != null) {
+          builder.trustManager(new ByteArrayInputStream(tlsCreds.getRootCertificates()));


I'm very wary of assuming we're using the same implementation as the builder. I suggest have all these cases produce TrustManager[] tms, then after the if-elses call builder.trustManager(tms) and filter tms for x509ExtendedTrustManager.

I didn't understand this comment. When you say assuming same implementation as the builder are you referring to this line

builder.trustManager(new ByteArrayInputStream(tlsCreds.getRootCertificates()));

since the other 2 lines don't reference the builder. This is an existing statement. Implementation of TrustManager as the builder?

builder.trustManager() is having the builder use some certs for its trust. But then you use a different trust manager implementation for the per-RPC verification. I'm wary of them being different. There could be a performance reason to do so; I've not checked recently if tcnative has a fast path for simple file-based verification (to use boringssl/openssl instead of calling back into Java). But in general, creating two different instances (the code you added, and the existing builder.trustManager()) and hoping they behave the same is worrisome.

kannanjgithub · 2024-12-09T18:56:52Z

I don't know what this error is about:

java/netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java:621:18: 'public' modifier out of order with the JLS suggestions. [ModifierOrder]

ejona86 · 2024-12-09T22:17:45Z

synchronized public boolean mayBeVerifyAuthority should have "public" first: public synchronized boolean mayBeVerifyAuthority.

JLS == Java Language Specification

The relevant part of the style guide:
https://google.github.io/styleguide/javaguide.html#s4.8.7-modifiers
But checkstyle also links to some useful things:
https://checkstyle.sourceforge.io/checks/modifier/modifierorder.html

ejona86 · 2024-12-09T22:22:29Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+          peerVerified = false;
+        }
+        authoritiesAllowedForPeer.put(authority, peerVerified);
+        if (authoritiesAllowedForPeer.size() > MAX_AUTHORITIES_CACHE_SIZE) {


Override removeEldestEntry()?

ejona86 · 2024-12-09T23:18:34Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiator.java

+   * @throws CertificateException if certificates have a problem
+   */
+  default boolean mayBeVerifyAuthority(String authority)
+      throws SSLPeerUnverifiedException, CertificateException {


We don't want to pollute this API with SSL-isms. In general, our internal APIs don't use exceptions. It'd be better to return a Status here.

ejona86 · 2024-12-09T23:19:01Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiator.java

+   */
+  default boolean mayBeVerifyAuthority(String authority)
+      throws SSLPeerUnverifiedException, CertificateException {
+    return true;


If we are going to have a default, it should fail.

ejona86 · 2024-12-09T23:22:34Z

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

+    } catch (UnsupportedOperationException ex) {
+      logger.log(Level.FINE,
+          "Can't allow authority override in rpc when X509ExtendedTrustManager is not available.",
+          callOptions.getAuthority());


This argument is unused.

ejona86 · 2024-12-09T23:23:00Z

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

+      }
+    } catch (UnsupportedOperationException ex) {
+      logger.log(Level.FINE,
+          "Can't allow authority override in rpc when X509ExtendedTrustManager is not available.",


This log doesn't seem to have a purpose, as we're about to fail the RPC itself. So the error is being handled.

ejona86 · 2024-12-09T23:23:52Z

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

+        logger.log(Level.FINE, errMsg);
+        return new FailingClientStream(Status.INTERNAL.withDescription(errMsg), tracers);
+      }
+    } catch (UnsupportedOperationException ex) {


ex is being lost

No longer throwing exception.

ejona86 · 2024-12-09T23:26:56Z

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

+        String errMsg = String.format("Peer hostname verification failed for authority '%s'.",
+            callOptions.getAuthority());
+        logger.log(Level.FINE, errMsg);
+        return new FailingClientStream(Status.INTERNAL.withDescription(errMsg), tracers);


UNAVAILABLE would be better here. INTERNAL is when we detect a bug in gRPC and can't communicate or such. TLS failures normally show up as UNAVAILABLE.

ejona86 · 2024-12-09T23:28:38Z

netty/src/main/java/io/grpc/netty/NettyClientTransport.java

+    try {
+      if (callOptions.getAuthority() != null && !negotiator.mayBeVerifyAuthority(
+          callOptions.getAuthority())) {
+        String errMsg = String.format("Peer hostname verification failed for authority '%s'.",


Nit: we don't include ending punctuation in status error messages.

ejona86 · 2024-12-09T23:32:33Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

@@ -1105,4 +1209,245 @@ protected final void fireProtocolNegotiationEvent(ChannelHandlerContext ctx) {
      ctx.fireUserEventTriggered(pne);
    }
  }
+
+  static final class SslEngineWrapper extends SSLEngine {


This is a lot of boilerplate. It'd be good to move these to their own files. You can have it just be the "wrapper", and then still override getPeerHost()/getHandshakeSession() in the file here.

It's interesting that only getSSLParameters() uses sslEngine. Maybe the separate file could be a NoopSslEngine?

ejona86 · 2024-12-09T23:33:00Z

netty/build.gradle

@@ -18,7 +18,7 @@ tasks.named("jar").configure {
 dependencies {
    api project(':grpc-api'),
            libraries.netty.codec.http2
-    implementation project(':grpc-core'),
+            implementation project(':grpc-core'),


netty/src/main/java/io/grpc/netty/NettyClientTransport.java

ejona86 · 2024-12-13T16:41:21Z

netty/src/main/java/io/grpc/netty/NoopSslEngine.java

+ * A no-op implementation of SslEngine, to facilitate overriding only the required methods in
+ * specific implementations.
+ */
+public class NoopSslEngine extends SSLEngine {


Package-private

netty/src/main/java/io/grpc/netty/ProtocolNegotiator.java

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

ejona86 · 2024-12-13T16:48:56Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+    }
+
+    public void setSslEngine(SSLEngine sslEngine) {
+      this.sslEngine = sslEngine;


If all we are using this engine for is SSLParameters, let's copy the parameters here and not try to use the sslEngine from another thread.

It is also used for the SSLSession for getting the peer certificate chain.

ejona86 · 2024-12-13T16:51:03Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+
+    @Override
+    public synchronized Status verifyAuthority(@Nonnull String authority) {
+      if (!canVerifyAuthorityOverride()) {


Inline the check here. That makes it more obvious that the error message returned in incorrect some of the time, and it doesn't seem having the check be named is helping anything. If you do want to give the check a name, assign it to a boolean canVerifyAuthorityOverride local variable.

ejona86 · 2024-12-13T16:54:05Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+        try {
+          verifyAuthorityAllowedForPeerCert(authority);
+          peerVerificationStatus = Status.OK;
+        } catch (SSLPeerUnverifiedException | CertificateException e) {


Don't throw away e. status.withCause(e)

ejona86 · 2024-12-13T16:54:30Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+          peerVerificationStatus = Status.OK;
+        } catch (SSLPeerUnverifiedException | CertificateException e) {
+          peerVerificationStatus = Status.UNAVAILABLE.withDescription(
+                  String.format("Peer hostname verification failed for authority '%s'",


This looks the same as what someone might see if the connection failed. Give a hint it was a per-RPC override?

ejona86 · 2024-12-13T16:59:39Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+    private void verifyAuthorityAllowedForPeerCert(String authority)
+        throws SSLPeerUnverifiedException, CertificateException {
+      SSLEngine sslEngineWrapper = new SslEngineWrapper(sslEngine, authority);
+      // The typecasting of Certificate to X509Certificate should work because this method will only


Just because a X509ExtendedTrustManager is available, doesn't mean it was used. An array of TrustManagers are available and the implementation used one of them, but maybe not the X509 one.

Now, we do have a good understanding we are using TLS and thus X509. But that's a very different assumption.

ejona86 · 2024-12-13T17:05:02Z

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

+          TrustManagerFactory tmf = TrustManagerFactory.getInstance(
+              TrustManagerFactory.getDefaultAlgorithm());
+          tmf.init((KeyStore) null);
+          x509ExtendedTrustManager = Arrays.stream(tmf.getTrustManagers())


Netty can be used on Android, although we discourage it. That means no java.util.streams.

That should apply to Optional as well... how is that already a dependency? Ah, it was added recently in #11600. We'll remove that.

kannanjgithub added 13 commits November 18, 2024 16:51

In-progress changes.

fa36f83

In-progress changes that used ExtendedSSLSession.

f31b8bc

Authority verify in Netty transport.

0152478

Netty authority verify against peer host in server cert.

5e2e22e

unit test.

b0f86cf

nit changes and drop unintended changes.

5ba5431

nit changes and drop unintended changes.

e42492b

Cache the peer verification result.

5285353

Move extraction of X509ExtendedTrustManager to utils.

ea969ef

Fixes.

94172de

Fixes.

0ddd42c

Fixes.

5109115

nit

d5f3968

kannanjgithub requested a review from ejona86 December 4, 2024 13:23

ejona86 reviewed Dec 5, 2024

View reviewed changes

kannanjgithub added 4 commits December 9, 2024 12:18

In-progress review comments.

1e072d4

Address review comments.

32104ce

revert unintended

95aae4a

revert unintended

c0327b5

ejona86 reviewed Dec 9, 2024

View reviewed changes

kannanjgithub added 2 commits December 13, 2024 20:44

Address review comments.

b24a605

Fix warning.

862b95b

ejona86 reviewed Dec 13, 2024

View reviewed changes

kannanjgithub added 5 commits December 14, 2024 14:52

Address review comments.

8902dbd

Address review comments.

01b0eb2

Remove duplicate definitions of createTrustManager.

8dd8749

Review comments.

3d744d9

Move NoopSslSession to io.grpc.internal under grpc-core project.

4ef0fdb

Added flag with default false for the per-rpc authority check.

fdc6e94

netty: Per-rpc call option authority verification against peer cert subject names #11724

Are you sure you want to change the base?

netty: Per-rpc call option authority verification against peer cert subject names #11724

Conversation

kannanjgithub commented Dec 4, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannanjgithub commented Dec 9, 2024

ejona86 commented Dec 9, 2024

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

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

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

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

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

Choose a reason for hiding this comment