search

grpc-ecosystem / grpc-spring

Spring Boot starter module for gRPC framework.
https://grpc-ecosystem.github.io/grpc-spring/
Apache License 2.0
3.41k stars 808 forks source link

Retrieve the peer certificate from the certificate array #1079

Open jazdw opened 3 months ago

jazdw commented 3 months ago

Fixes #1076

The SSLContextGrpcAuthenticationReader reads the last certificate from the peer certificates array, however I believe the intent was probably to retrieve the peer certificate, not an intermediate certificate.

https://github.com/grpc-ecosystem/grpc-spring/blob/de71ce3deaa48c220bcade928268806c5e971656/grpc-server-spring-boot-starter/src/main/java/net/devh/boot/grpc/server/security/authentication/SSLContextGrpcAuthenticationReader.java#L56

[The Javadoc](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/javax/net/ssl/SSLSession.html#getPeerCertificates()) of javax.net.ssl.SSLSession#getPeerCertificates specifies that it returns:

an ordered array of peer certificates, with the peer's own certificate first followed by any certificate authorities.

If there are no intermediate CA then the array with have length 1, and there will be no difference in behavior. This is probably why this bug has not been reported before (I don't think).

jazdw commented 3 months ago

@ST-DDT I've added tests, it was a bit of a pain because there were no tests for the certificates/TLS yet

ST-DDT commented 3 months ago

@ST-DDT I've added tests, it was a bit of a pain because there were no tests for the certificates/TLS yet

There are integration tests:

https://github.com/grpc-ecosystem/grpc-spring/blob/de71ce3deaa48c220bcade928268806c5e971656/tests/src/test/java/net/devh/boot/grpc/test/setup/SelfSignedMutualSetupTest.java#L40-L45

jazdw commented 3 months ago

There are integration tests:

Ah I didn't see those, but there are no tests or certificates with an intermediate AFAIK. Are you OK with the approach I took?

ST-DDT commented 3 months ago

Ah I didn't see those, but there are no tests or certificates with an intermediate AFAIK. Are you OK with the approach I took?

Yes, I played with your tests a bit to ensure, that we don't just test the mock certificate arrays. All seems to be working as expected. (Although I must admit, that I know very little about certificates especially in Java).

jazdw commented 3 months ago

@ST-DDT is there something else we need to do to get this merged?

ST-DDT commented 3 months ago

@ST-DDT is there something else we need to do to get this merged?

More reviews.

jazdw commented 2 months ago

@ST-DDT is there something else we need to do to get this merged?

More reviews.

This has been sitting here a few weeks now, what is required to get more people to review this? Can we tag someone?

jazdw commented 2 months ago

@yidongnan @stanley-cheung @fengli79 Could one of you review this so it can be merged? I am not sure who to tag, just found you guys as recent reviewers of closed PRs.