Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Retrieve the peer certificate from the certificate array #1079

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Retrieve the peer certificate from the certificate array #1079

wants to merge 5 commits into from
+347 −1

Conversation

jazdw
Copy link

@jazdw jazdw commented Mar 27, 2024 •
edited

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

return fromCertificate(certs[certs.length - 1]);

The Javadoc 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 jazdw mentioned this pull request Mar 27, 2024
Open
ST-DDT
ST-DDT reviewed Mar 27, 2024
View reviewed changes
Copy link
Collaborator

@ST-DDT ST-DDT left a 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?

@jazdw
Copy link
Author

jazdw commented Mar 29, 2024

@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
Copy link
Collaborator

ST-DDT commented Mar 29, 2024

@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

"grpc.client.test.address=localhost:9090",
"grpc.client.test.security.authorityOverride=localhost",
"grpc.client.test.security.trustCertCollection=file:src/test/resources/certificates/trusted-servers-collection",
"grpc.client.test.security.clientAuthEnabled=true",
"grpc.client.test.security.certificateChain=file:src/test/resources/certificates/client1.crt",
"grpc.client.test.security.privateKey=file:src/test/resources/certificates/client1.key"})

@jazdw
Copy link
Author

jazdw commented Mar 29, 2024

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
Copy link
Collaborator

ST-DDT commented Mar 29, 2024

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).

ST-DDT
ST-DDT previously approved these changes Mar 29, 2024
View reviewed changes
@jazdw
Copy link
Author

jazdw commented Apr 2, 2024

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

@ST-DDT
Copy link
Collaborator

ST-DDT commented Apr 3, 2024

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

More reviews.

@jazdw
Copy link
Author

jazdw commented Apr 11, 2024

@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
Copy link
Author

jazdw commented Apr 22, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ST-DDT ST-DDT ST-DDT approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SSLContextGrpcAuthenticationReader reads the wrong certificate from the peer certificates array
2 participants
@jazdw @ST-DDT