-
Notifications
You must be signed in to change notification settings - Fork 824
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
Retrieve the peer certificate from the certificate array #1079
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks hood to me. But can you add/prepare a test (even if manual/temporal) to reproduce the bug and verify the fix?
@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: grpc-spring/tests/src/test/java/net/devh/boot/grpc/test/setup/SelfSignedMutualSetupTest.java Lines 40 to 45 in de71ce3
|
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? |
...et/devh/boot/grpc/server/security/authentication/SSLContextGrpcAuthenticationReaderTest.java
Outdated
Show resolved
Hide resolved
Yes, I played with your tests a bit to ensure, that we don't just test the mock certificate arrays. |
@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? |
@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. |
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.grpc-spring/grpc-server-spring-boot-starter/src/main/java/net/devh/boot/grpc/server/security/authentication/SSLContextGrpcAuthenticationReader.java
Line 56 in de71ce3
The Javadoc of
javax.net.ssl.SSLSession#getPeerCertificates
specifies that it returns: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).