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.

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

advancedtls package does not set VerifiedChains in TLSInfo.State #7176

Closed
mudhireddy opened this issue May 1, 2024 · 5 comments
Closed

advancedtls package does not set VerifiedChains in TLSInfo.State #7176

mudhireddy opened this issue May 1, 2024 · 5 comments
Assignees

Comments

@mudhireddy
Copy link
Contributor

NOTE: if you are reporting is a potential security vulnerability or a crash,
please follow our CVE process at
https://github.com/grpc/proposal/blob/master/P4-grpc-cve-process.md instead of
filing an issue here.

Please see the FAQ in our main README.md, then answer the questions below
before submitting your issue.

What version of gRPC are you using?

1.60.0

What version of Go are you using (go version)?

go1.22.1

What operating system (Linux, Windows, …) and version?

linux/amd64

What did you do?

If possible, provide a recipe for reproducing the error.

// The function buildVerifyFunc is used when users want root cert reloading,
// and possibly custom verification check.
// We have to build our own verification function here because current
// tls module:
// 1. does not have a good support on root cert reloading.
// 2. will ignore basic certificate check when setting InsecureSkipVerify
// to true.

When using buildVerifyFunc() here https://github.com/grpc/grpc-go/blob/e4a6ce3a5489/security/advancedtls/advancedtls.go#L516
it verifies the client certificate chain correctly as expected but it doesn't expose the verifiedChains to the upper layer to make use of them like how tls package does.

What did you expect to see?

I expect it to set the VerifiedChains in TLSInfo.State structure.

patch for this is at: mudhireddy/grpc-go@9d07858#diff-e8cdf2d70ced71ce723591cf60921a380bd6f007888f8883cbb20df4ef406680L444

it sets TLSInfo.State.VerifiedChains so that application can read the necessary information from VerifiedChains.

https://github.com/grpc/grpc-go/blob/e4a6ce3a5489/security/advancedtls/advancedtls.go#L514
514: func (c *advancedTLSCreds) ServerHandshake(rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
cfg := credinternal.CloneTLSConfig(c.config)
- cfg.VerifyPeerCertificate = buildVerifyFunc(c, "", rawConn)
+ peerVerifiedChains := [][]*x509.Certificate{}
+ cfg.VerifyPeerCertificate = buildVerifyFunc(c, "", rawConn, &peerVerifiedChains)

https://github.com/grpc/grpc-go/blob/e4a6ce3a5489/security/advancedtls/advancedtls.go#L528
info.SPIFFEID = credinternal.SPIFFEIDFromState(conn.ConnectionState())
+ info.State.VerifiedChains = peerVerifiedChains
return credinternal.WrapSyscallConn(rawConn, conn), info, nil

https://github.com/grpc/grpc-go/blob/e4a6ce3a5489/security/advancedtls/advancedtls.go#L553
func buildVerifyFunc(c *advancedTLSCreds,
serverName string,
- rawConn net.Conn) func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+ rawConn net.Conn,
+ peerVerifiedChains *[][]*x509.Certificate) func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {

https://github.com/grpc/grpc-go/blob/e4a6ce3a5489/security/advancedtls/advancedtls.go#L613 (set the peerVerifiedChains)
leafCert = rawCertList[0]
+ *peerVerifiedChains = chains

What did you see instead?

info.State.VerifiedChains is empty.

@mudhireddy
Copy link
Contributor Author

sorry gave the incorrect link earlier, actual patch is at. mudhireddy@54a29d7

@mudhireddy
Copy link
Contributor Author

also created a pull request #7181 appreciate any help to get that moving

@purnesh42H
Copy link
Contributor

also created a pull request #7181 appreciate any help to get that moving

Thanks @mudhireddy. I will take a look

@purnesh42H purnesh42H added the P1 label May 5, 2024
@purnesh42H purnesh42H assigned easwars and unassigned purnesh42H May 7, 2024
@purnesh42H purnesh42H added P1 and removed P1 labels May 7, 2024
@dfawley dfawley assigned gtcooke94 and unassigned easwars May 7, 2024
@mudhireddy
Copy link
Contributor Author

PR is merged but this is still open, do I need to close this manually ?

#7181

@gtcooke94
Copy link
Contributor

Thanks for the report and the PR @mudhireddy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants