advancedtls: Use the New tls.Config.VerifyConnection callback

[ZhenLian]

Use case(s) - what problem will this feature solve?

A new verification method was recently introduced in PR https://go-review.googlesource.com/c/go/+/229122/ , with the initiative to have all the connection information(such as ServerName, PeerCertificate, etc) in verify callbacks. The original issue is golang/go#36736.

We might also want to switch to use this method in advancedtls, to improve internal code quality and reduce duplicate code with main tls library of gRPC. Right now we are using a workaround of building verification callback in a closure, which could be improved after this function is used.

No API Changes are expected. This is intended for internal code quality enhancement.

Proposed Solution

Use tls.Config.VerifyConnection(s ConnectionState) in advancedtls.

@jiangtaoli2016 FYI.

@ZhenLian I'm looking into this issue and I have one question at the moment. I don't understand the statement here https://github.com/grpc/grpc-go/blob/master/security/advancedtls/advancedtls.go#L446-L447.

//   2. will ignore basic certificate check when setting InsecureSkipVerify
//   to true.

I understand that buildVerifyFunc's purpose is to support root cert reloading and the verifyFunc callback but I'm not sure how ...will ignore basic certificate check when setting InsecureSkipVerify is relevant to buildVerifyFunc.

May you elaborate on this?

@ZhenLian I'm looking into this issue and I have one question at the moment. I don't understand the statement here https://github.com/grpc/grpc-go/blob/master/security/advancedtls/advancedtls.go#L446-L447.

//   2. will ignore basic certificate check when setting InsecureSkipVerify
//   to true.

I understand that buildVerifyFunc's purpose is to support root cert reloading and the verifyFunc callback but I'm not sure how ...will ignore basic certificate check when setting InsecureSkipVerify is relevant to buildVerifyFunc.

May you elaborate on this?

Since we are reloading the root CA certs in buildVerifyFunc is InsecureSkipVerify being set to true so that we may verify the client/server certificates using the reloaded root CA certs?

[ZhenLian]

@kadenlnelson
Yes, the reason we are settingInsecureSkipVerify to true on tls.Config is for root credential reloading and custom verification. If this field is set to true, the underlying stack would bypass both the certificate check and the hostname check.

@ZhenLian May you take a look at my work in progress PR? https://github.com/grpc/grpc-go/pull/3963/files

Also in reference to;

No API Changes are expected. This is intended for internal code quality enhancement.

Not sure how this is going to be possible since the function signature is changing to use;

	VerifyPeer func(tls.ConnectionState) error

@ZhenLian before I go ahead and add tests/docs, does my draft pull request assess the issue accordingly?

[ZhenLian]

I left some comments. Feel free to let me know if you have any questions. Thanks!

[easwars]

The PR mentioned here was closed a while back due to inactivity. Hence, closing this one as well.