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

Fully Pluggable TLSv1.3 Key Exchange #11914

Closed
wants to merge 31 commits into from

Conversation

mattcaswell
Copy link
Member

This PR started life as an attempt to ensure that we are actually able to support all the groups that a client offers or a server accepts in a TLS handshake. If the default provider is not loaded it is possible that we have a different set of groups available. This turned out to be quite tricky and the easiest solution was to ask the available providers what groups they can support. From there it was a short step to having support for fully pluggable (TLSv1.3) key exchange. Pluggable TLSv1.2 key exchange is quite a bit trickier so I've not attempted that.

With this PR it is possible to write a provider that adds new TLS groups and TLSv1.3 key exchange algorithms. It is marked as WIP because it is dependent on #11898 and includes the commits from there.

This PR also adds the concept of "Capabilities". This is information a provider can supply about things that it can do. At the moment there is support for just one capability (the "TLS-GROUP" capability), but I envisage it could be easily extended to other things in the future.

@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer labels May 22, 2020
@mattcaswell mattcaswell mentioned this pull request May 22, 2020
2 tasks
* Top secret. This algorithm only works if no one knows what this number is.
* Please don't tell anyone what it is.
*/
const unsigned char private_constant[XOR_KEY_SIZE] = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please also fix the comment to make it obvious that it is humorous.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and added an extra warning about not really using this.

* implementation you will get back. Theoretically this could be
* different every time...we assume here that you'll always get the
* same one back if you repeat the exact same fetch. Is this a reasonable
* assumption to make (in which case perhaps we should document this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its a pluggable provider that had priority then I dont think that assumption would be a good one - but maybe in that case it should error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not :(

A PKCS #11 provider could offer something one second and not the next.

I could add code to randomise the returns. That would be interesting...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A PKCS #11 provider could offer something one second and not the next.

Libssl just won't work with such a provider at the moment. That's a much more significant overahaul that I don't plan to do for 3.0.

@mattcaswell mattcaswell changed the title WIP: Fully Pluggable TLSv1.3 Key Exchange Fully Pluggable TLSv1.3 Key Exchange Jun 5, 2020
@mattcaswell
Copy link
Member Author

I've rebased this (which was a nightmare), and taken it out of WIP since all its dependencies are now in. I've not yet addressed the feedback.

@mattcaswell
Copy link
Member Author

Pushed...addressing feedback comments above.

@mattcaswell
Copy link
Member Author

Fixups pushed to address some Travis issues.

Ping?

@openssl-machine openssl-machine added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jun 18, 2020
@openssl-machine
Copy link
Collaborator

This pull request is ready to merge

@mattcaswell
Copy link
Member Author

Pushed! Thanks!

openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
With capabilities we can query a provider about what it can do.
Initially we support a "TLS-GROUP" capability.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
Provide a function to applications to query the capabilities that a
provider can perform.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
Now that we have added the TLS-GROUP capability to the default provider
we can use that to discover the supported group list based on the loaded
providers.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
We rename these function to EVP_PKEY_CTX_get_group_name and
EVP_PKEY_CTX_set_group_name so that they can be used for other algorithms
other than EC.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
The previous commit added the EVP_PKEY_CTX_[get|set]_group_name
functions to work with EC groups. We now extend that to also work for
DH.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
The previous commits made EVP_PKEY_CTX_[get|set]_group_name work for
EC and DH keys. We now extend this to ECX. Even though that keys with
these key types only have one group we still allow it to be explicitly
set so that we have only one codepath for all keys. Setting the group
name for these types of keys is optional, but if you do so it must have
the correct name.

Additionally we enable parameter generation for these keys. Parameters
aren't actually needed for this key type, but for the same reasons as
above (to ensure a single codepath for users of these algorithms) we
enable it anyway.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
Document the OSSL_PROVIDER_get_capabilities() function as well as the
provider side support for capabilities.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
A number of these functions returned a NID or an array of NIDs for the
groups. Now that groups can come from the providers we do not necessarily
know the NID. Therefore we need to handle this in a clean way.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
If a provider had a "copy" function in the its keymgmt definition we
were ignoring it.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
If EVP_PKEY_copy_parameters() failed in libssl we did not provide a very
helpful error message. We provide a better one.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
The supported_groups extension only supported EC groups in DTLS.
Therefore we shouldn't send it in a no-ec build.

Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
openssl-machine pushed a commit that referenced this pull request Jun 19, 2020
Reviewed-by: Shane Lontis <[email protected]>
(Merged from #11914)
@romen
Copy link
Member

romen commented Jun 26, 2020

@mattcaswell this seems like quite a significant new feature for libssl (I am pretty excited about it and its potential!): should we add a CHANGES and a NEWS entry for it?

@@ -332,6 +344,72 @@ pointing at the string "foo,bar"
For more information on handling parameters, see L<OSSL_PARAM(3)> as
L<OSSL_PARAM_int(3)>.

=head1 CAPABILITIES

Capabilties describe some of the services that a provider can offer.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Capabilties

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As previously mentioned to @slontis, this is entirely the fault of my keyboard:

I think there's a problem with my keyboard. It can't seem to spell that word.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

singer instead of signer is one of my favorites.

@mattcaswell
Copy link
Member Author

@mattcaswell this seems like quite a significant new feature for libssl (I am pretty excited about it and its potential!): should we add a CHANGES and a NEWS entry for it?

Yes - it should have that. I'd actually like to blog about it at some point with some kind of tutorial type approach to explain how to use it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants