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: unable to configure TLS config #5667

Closed
ghost opened this issue Sep 23, 2022 · 21 comments
Closed

advancedtls: unable to configure TLS config #5667

ghost opened this issue Sep 23, 2022 · 21 comments
Assignees

Comments

@ghost
Copy link

ghost commented Sep 23, 2022

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.

@ghost ghost added the Type: Bug label Sep 23, 2022
@easwars
Copy link
Contributor

easwars commented Sep 27, 2022

@ZhenLian : Do you mind taking a look at this?

@dfawley
Copy link
Member

dfawley commented Oct 4, 2022

Ping @ZhenLian

@RonanMacF
Copy link

RonanMacF commented Oct 5, 2022

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.

@ZhenLian
Copy link
Contributor

ZhenLian commented Oct 7, 2022

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.

@ZhenLian
Copy link
Contributor

ZhenLian commented Oct 7, 2022

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!

@RonanMacF
Copy link

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

@dfawley
Copy link
Member

dfawley commented Oct 25, 2022

Ping @ZhenLian

@ZhenLian
Copy link
Contributor

ZhenLian commented Nov 1, 2022

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!

@RonanMacF
Copy link

RonanMacF commented Nov 1, 2022

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

@ZhenLian
Copy link
Contributor

ZhenLian commented Nov 1, 2022

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!

@rockspore
Copy link
Contributor

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?

@ZhenLian
Copy link
Contributor

I made the PR to add min/max version options to advanced tls: #5797
There should be a separate PR for ClientAuth.
Let me know how it goes. Thanks all for the discussion!

@RonanMacF
Copy link

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.

@ZhenLian
Copy link
Contributor

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

@dfawley dfawley removed their assignment Nov 28, 2022
@stanleymho
Copy link

+1 on supporting some important TLS attributes such as MinVersion and CipherSuites

@dfawley dfawley added the P2 label Dec 13, 2022
@joeljeske
Copy link
Contributor

+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?

joeljeske pushed a commit to joeljeske/grpc-go that referenced this issue Feb 4, 2023
Adding an "TlsVersionOption" for users to select their desired min/max TLS versions, if advanced TLS is used, per request by grpc#5667

RELEASE NOTES:

security/advancedtls: add min/max TLS version selection options
@joeljeske
Copy link
Contributor

@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 :)

holyspectral pushed a commit to holyspectral/grpc-go that referenced this issue Feb 2, 2024
Adding an "TlsVersionOption" for users to select their desired min/max TLS versions, if advanced TLS is used, per request by grpc#5667

RELEASE NOTES:

security/advancedtls: add min/max TLS version selection options
@mudhireddy
Copy link
Contributor

Is there any plan to support CipherSuites in advancedtls package similar to the min/max version?

@matthewstevenson88
Copy link
Contributor

@mudhireddy Added CipherSuites in #7269.

@dfawley
Copy link
Member

dfawley commented Jul 9, 2024

@matthewstevenson88 Can this be closed? Or is there more to do here?

@matthewstevenson88
Copy link
Contributor

Thanks @dfawley, yes this can be closed.

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