netty: Per-rpc call option authority verification against peer cert subject names by kannanjgithub · Pull Request #11724 · grpc/grpc-java

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

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

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

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

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

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

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

netty/build.gradle

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

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

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

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

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

…ler instead of the Protocol negotiator.

…et the attribute for the hostname verifier.

# Conflicts: # examples/example-tls/src/main/java/io/grpc/examples/helloworldtls/HelloWorldClientTls.java

…oritychecktls # Conflicts: # core/src/main/java/io/grpc/internal/CertificateUtils.java # okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java

…ychecktls

…nto authoritychecktls" This reverts commit 8576a4e, reversing changes made to da19b28.

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

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

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

core/src/main/java/io/grpc/internal/CertificateUtils.java

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

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

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

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

kannanjgithub · 2025-02-19T13:19:54Z

I'm now only doing the authority verification if the transport authority is different from the one in the headers.

I do think we should set the attribute in most of the built-in negotiators, but ALTS and S2A would probably be without it for now.

As a subsequent work I assume.

After the changes to disallow authority override without having a verifier in the attributes, the FakeControlPlaneIntegrationTest which doesn't use Tls but plain text transport is failing. Is it ok to introduce a system property to allow authority override without a verifier and set it from the integration test?

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

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

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

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

ejona86

Before we merge, let's run some tests inside Google to make sure this doesn't break anything. (Since the risk is high, it is better to run them before merging to avoid needing to revert the PR.) I'll start them today.

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

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

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

ejona86

Test run inside Google looked good

…grpc#11724) Per-rpc verification of authority specified via call options or set by the LB API against peer cert's subject names.

kannanjgithub added 17 commits November 18, 2024 16:51

In-progress changes.

fa36f83

In-progress changes that used ExtendedSSLSession.

f31b8bc

Authority verify in Netty transport.

0152478

In-progress changes for Authority verify in okhttp transport.

c31995e

Netty authority verify against peer host in server cert.

5e2e22e

unit test.

b0f86cf

in-progress changes.

dd95f8f

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

commit

4d71cce

Fixes.

94172de

Fixes.

0ddd42c

Fixes.

909b863

Fixes.

5109115

nit

d5f3968

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

Changes.

24500f4

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

Merge branch 'master' into authoritychecktls

b8b70bf

ejona86 reviewed Jan 21, 2025

View reviewed changes

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/NettyClientTransport.java Outdated Show resolved Hide resolved

kannanjgithub and others added 14 commits January 22, 2025 15:25

Ignore animal sniffer errors via annotation in tests.

2af1cca

In-progress changes to move authority verification to the ChannelHand…

15c8161

…ler instead of the Protocol negotiator.

Save sslEngine and use it later after the handshake is complete, to s…

7941abc

…et the attribute for the hostname verifier.

temp testing changes

7fcd98d

Changes.

4c50e71

Merge branch 'authoritychecktls-fork' into authoritychecktls

da19b28

# Conflicts: # examples/example-tls/src/main/java/io/grpc/examples/helloworldtls/HelloWorldClientTls.java

Merge branch 'grpc:master' into authoritychecktls

4e24446

Merge remote-tracking branch 'origin/authorityverifyokhttp' into auth…

8576a4e

…oritychecktls # Conflicts: # core/src/main/java/io/grpc/internal/CertificateUtils.java # okhttp/src/main/java/io/grpc/okhttp/OkHttpChannelBuilder.java

Merge remote-tracking branch 'origin/authoritychecktls' into authorit…

0627bf4

…ychecktls

Revert "Merge remote-tracking branch 'origin/authorityverifyokhttp' i…

02a1041

…nto authoritychecktls" This reverts commit 8576a4e, reversing changes made to da19b28.

Fix style

1d6f46f

Revert unintended changes.

42058ea

Revert unintended changes.

cd93d4e

Revert unintended changes.

1ff1f0b

ejona86 reviewed Feb 12, 2025

View reviewed changes

Review comments.

671dee0

ejona86 reviewed Feb 18, 2025

View reviewed changes

Review comments

671444b

ejona86 reviewed Feb 19, 2025

View reviewed changes

Fix review comments

7ecbf56

ejona86 approved these changes Feb 20, 2025

View reviewed changes

netty/src/main/java/io/grpc/netty/GrpcHttp2OutboundHeaders.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/X509AuthorityVerifier.java Outdated Show resolved Hide resolved

netty/src/main/java/io/grpc/netty/NettyClientHandler.java Outdated Show resolved Hide resolved

Fix test and some mistakes.

e7c7913

ejona86 approved these changes Feb 21, 2025

View reviewed changes

kannanjgithub merged commit cdab410 into grpc:master Feb 24, 2025
15 of 16 checks passed

kannanjgithub deleted the authoritychecktls branch February 24, 2025 14:58

github-actions bot locked as resolved and limited conversation to collaborators May 26, 2025

Conversation

kannanjgithub commented Dec 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kannanjgithub commented Dec 9, 2024

Uh oh!

ejona86 commented Dec 9, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kannanjgithub commented Feb 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ejona86 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants