search

bcgit / bc-java

Bouncy Castle Java Distribution (Mirror)
https://www.bouncycastle.org/java.html
MIT License
2.31k stars 1.14k forks source link

Allow configuration of supported_groups TLS extension #234

Open scantisani opened 7 years ago

scantisani commented 7 years ago

My colleague and I are really missing a way to configure which elliptic curves are sent in the supported_groups extension of a ClientHello. I can see there's a comment in the relevant section of the code that mentions "needing configuration options".

https://github.com/bcgit/bc-java/blob/3aff181c4db4c2d4ccd7a88fc1f6d1d9d996fa6c/tls/src/main/java/org/bouncycastle/tls/AbstractTlsClient.java#L187

Is this functionality likely to be added soon (version 1.59)?

I had originally wanted to submit a pull request for this instead of creating an issue, but I really couldn't figure out where to start (like which class it would make sense to add the configuration methods to); I'm sorry about that.

Thank you for all your hard work so far!

peterdettman commented 7 years ago

No problem, it's an easy one - I've pushed a fix, but it may take a while to mirror here.

AbstractTlsClient.getClientExtensions now delegates to getSupportedGroups method, which you can override to choose different groups.

BTW, all "configuration" in the org.bouncycastle.tls API is handled in this programmatic fashion, so no need to go searching for separate config code.

scantisani commented 7 years ago

Thank you!

Apologies, but I'm not entirely sure how I would make use of the fix. How would I go about getting the BouncyCastleJsseProvider to use a new subclass of AbstractTlsClient with getSupportedGroups overridden?

Realistically, I'd subclass ProvTlsClient (to retain as much of the built-in TLS behaviour as possible, but still inherit from AbstractTlsClient to override getSupportedGroups). But in every class that a TlsClient is provided to TlsClientProtocol#connect, a new ProvTlsClient is directly instantiated.

https://github.com/bcgit/bc-java/blob/ca4ab830719de87b4f9131cc76aa19e8269aa280/tls/src/main/java/org/bouncycastle/jsse/provider/ProvSSLEngine.java#L96

https://github.com/bcgit/bc-java/blob/ca4ab830719de87b4f9131cc76aa19e8269aa280/tls/src/main/java/org/bouncycastle/jsse/provider/ProvSSLSocketDirect.java#L283

https://github.com/bcgit/bc-java/blob/ca4ab830719de87b4f9131cc76aa19e8269aa280/tls/src/main/java/org/bouncycastle/jsse/provider/ProvSSLSocketWrap.java#L447

I can't see a way to give it different implementation of TlsClient, since all the classes in question are package-protected.

I don't have the best grasp of how Providers and Provider.Services work, so please tell me if I'm missing something obvious.

peterdettman commented 7 years ago

Ah, I didn't understand that you were using the BCJSSE provider, sorry. My early responses assumed you were using the org.bouncycastle.tls package directly.

SunJSSE uses the system property "jdk.tls.namedGroups" to allow this configuration. We can support that too in BCJSSE, I'll try to add it shortly.

scantisani commented 7 years ago

Much appreciated! Thank you. :-)

scantisani commented 7 years ago

Would it be possible for this property to be read dynamically, as opposed to just once in a static initializer, as with the SunJSSE? We need a way to configure what's sent in this extension semi-programmatically. Sorry for the repeated unusual requests! We have a hard requirement that there's no getting around.

peterdettman commented 7 years ago

Well, it would usually be better to have the behaviour consistent with SunJSSE. If you are happy for your code to not be portable b/w SunJSSE and BCJSSE, then perhaps a better way to achieve what you need is a BC extension?

There are a few classes already in org.bouncycastle.jsse package, intended as BC-only extensions to the JSSE API. e.g. We used this to expose TLS channel bindings functionality. We expect to also need it for backporting of functionality from later JDKs (i.e. when the JSSE API has expanded from version to version). The basic idea is that you test e.g. an SSLSocket to see if it also implements a BC extension interface, then you can access whatever extended functionality might be there.

I gather you might like a method that can programmatically configure the named groups separately for each connection?

scantisani commented 7 years ago

We're fine with our code not being portable--the BCJSSE is already our only option when it comes to using Brainpool TLS certificates.

What you've suggested with BC extensions seems like it could work for us! A method that can programmatically configure named groups per connection is exactly what we need.

If I've understood you correctly, you're suggesting adding a new method (like getChannelBinding in BCSSLConnection) to a BCJSSE-specific interface?