grpc-go
grpc-go copied to clipboard
grpc
advancedtls: unable to configure TLS config
grpc/credentials/tls.go provides a function which takes a *tls.Config and returns TransportCredentials. This allows the server to configure some important TLS attributes such as MinVersion and CipherSuites.
I am now looking to use advancedtls to give CRL capabilities, however the config field here is private and create only by the NewServerCreds function. Following how this config is made I do not see any way to configure these fields in the advancedtls tls.Config field.
I think it would be good to have this exposed or have a function which takes this as a parameter.
I have identified an issue that I would like to ensure if looked after here.
If RequireClientCert is true then we set the tls.clientAuth to tls.RequireAnyClientCert. This means that the client chain may not be validated and there may be issues down the line in trying to extract TLS information from client certificates, e.g.
if TLSInfo, ok := peer.AuthInfo.(credentials.TLSInfo); ok {
return TLSInfo.State.VerifiedChains[0][0].Subject.CommonName << this will panic as verified chain is nil
}
I would like to use the options RequireAndVerifyClientCert to get around this but the current library provides no means to do this.
Sorry I was just back from a long vacation.
@RonanMacF The CRL features is configured using RevocationConfig(https://github.com/grpc/grpc-go/blob/master/security/advancedtls/advancedtls.go#L207). It seems public. Do you mind giving me a pointer showing which part is private?
Normally, you put your CRL files into a directory, and then point RevocationConfig to that direction. Then it should start to work. Note that your CRL files need to meet the format in https://www.openssl.org/docs/man1.1.1/man3/X509_LOOKUP_hash_dir.html, otherwise it's not guaranteed to work.
For the second issue, it sounds like a bug there but would you mind raising another issue in Github, so I could keep track of it? I will take a closer look when I get a chance. Thank you so much!
Hi Zhen,
Sorry for the delay.
Revocation is on problem, works as expected and all good is there. The problem is the *tls.Config. here you can see the *tls.Config is private. here you can see it is initialised within the advancedtls package. here is where it is created in the package.
To summarise:
gRPC can take a credentials.TransportCredentials through grpc.Creds. Previously we could pass in a credentials.NewTLS which takes a *tls.Config. This allows us to configure this configuration to our needs. If we want CRL capabilities then we must use advancedtls.NewServerCreds. This function takes a *advancedtls.ServerOptions. This does not have a field which allows us to set the *tls.Config. This TLS configuration is instead set in (o *ServerOptions) config using values the package thinks is right. For obvious reasons people have different requirements so this is not good.
The second issue is tightly related to the above and will likely be fixed by it, do you think it is still required? Overriding user set values with less secure values doesn't seem like something that would happen (or shouldn't at least).
Sorry for the late response. I wanted to raise this in an internal design meeting but never got a chance, so I will put my thoughts here:
The Advanced TLS API is intended to be a wrapper on top of tls.Config that supports a few advanced security features, e.g. creds reloading, custom verification, CRL checking, etc. We hided the details of tls.Config on purpose and provided a limited amount of features so that users can choose among a few options. If we expose tls.Config, the API might gain some flexibility but it is hard to get a correct set-up. For example, for users having cred reloading settings, if they try to update the certificates in tls.Config manually while the reloading thread is running, the end result would be undetermined.
@RonanMacF are MinVersion and CipherSuites the only two things you want in tls.Config? We can probably add them into advancedtls as well.
@dfawley @erm-g Any thought is much appreciated. Thanks!
Hiding configurable attributes in an 'advanced' package seems like an odd way to go about things. Seems like it should be the opposite where the advanced package gives enhanced customizability which most users don't need.
ClientAuth is also used by me, in particular I set tls.RequireAndVerifyClientCert which the advancedtls package actually downgrades to tls.RequireAnyClientCert which breaks TLSInfo.State.VerifiedChains
Yes, currently in advancedtls we only support RequireClientCert field which will downgrade the option to RequireAnyClientCert, but we can change it or even just use ClientAuth to keep that consistent.
@dfawley I will put together a PR once you get a chance to reply. Thank you!
Just a thought, would it be useful or too confusing if the pkg provides a variant of NewServerCreds that takes a *tls.Config and respects all the fields in it unless superseded by the options?
I made the PR to add min/max version options to advanced tls: https://github.com/grpc/grpc-go/pull/5797
There should be a separate PR for ClientAuth.
Let me know how it goes. Thanks all for the discussion!
Thanks Zhen, this is a step in the right direction but still requires me to use a forked version of the package to get proper TLS usage. Is a PR underway for the other issues, in particular the RequireAndVerifyClientCert issue as this breaks retreiving information from the TLS.Info validatedChains. Are you expecting a PR to come from someone else, if so is there an agreed approach and so I can try to implement it. I would like to minimize the amount of time I have to use a forked version.
I see. Under the current structure, we can't add RequireAndVerifyClientCert, as we want to make use of VerifyPeerCertificate in tls.Config. We could change VerifyPeerCertificate to VerifyConnection, so sure, we can make that work.
In short, I will work on adding RequireAndVerifyClientCert but that requires some internal changes in our codebase, so it might take a while...
+1 on supporting some important TLS attributes such as MinVersion and CipherSuites
+1 to support MinVersion and CipherSuites in an advanced package 😄
If I understand, it looks like #5797 is 98% done..? @ZhenLian do you foresee yourself picking the PR back up, else I can find some time to polish it up.
Related to this, I'd like to support RequestClientCert, to allow servers to optionally support mTLS connections. Would that be feasible with the current VerifyPeerCertificate implementation?
@dfawley I pushed up #6007 to add the remaining integration test cases for #5797, feel free to push into the original PR if desired. Just trying to help get this landed :)
Is there any plan to support CipherSuites in advancedtls package similar to the min/max version?
