Refactor brainpool TLSv1.3 support #19315
Conversation
mattcaswell
added
branch: master
github-actions
bot
added
the
severity: fips change
| @@ -914,7 +914,7 @@ my @tests_tls_1_3_non_fips = ( | |||
| #We only configured brainpoolP256r1 on the client side, but TLSv1.3 | |||
| #is enabled and this group is not allowed in TLSv1.3. Therefore this | |||
| #should fail | |||
| "ExpectedResult" => "ServerFail" | |||
| "ExpectedResult" => "ClientFail" | |||
There was a problem hiding this comment.
Why does this result change?
There was a problem hiding this comment.
This test should actually succeed, but unfortunatley I did not spot that earlier.
For reference #19325 just fixes the table in the provider that prevented the
test case to succeed in the first place,, Actually I found 3 more test cases,
which succeed once the above mentioned issues are resolved.
There was a problem hiding this comment.
In the original implementation the name "brainpoolP256r1" is the name for both the TLSv1.2 group and the TLSv1.3 group. In my new implementation there is a distinction made between these two because they have different group ids assigned by IANA and therefore are treated as two different groups. "brainpoolP256r1" refers to the TLSv1.2 group and "brainpoolP256r1tls13" refers the TLSv1.3 group (matching the IANA names). In this test we configured the client with only a TLSv1.2 group, but the max supported protocol is TLSv1.3. Since we don't have any suitable groups configured for TLSv1.3 and we require to have at least one, we expect the client to abort.
This is not the case in the original implementation since the single group name covers both groups and hence we expect it to succeed.
There was a problem hiding this comment.
Yes, I see, but my original intention was to keep this error prone distinction away from the API,
since there is already a well-known way to select the min and max TLS versions,
this creates just lots more ways to use the API in an inconsistent way.
There was a problem hiding this comment.
Unfortunately, IMO, having a single name for 2 groups ids creates more problems than it solves. It also breaks somewhat the provider capability mechanism where providers signal via capabilities which groups they support and tell libssl what the group ids and names it should be using are. Suddenly libssl needs to know about "special" groups that need to be handled differently to what the provider tells us about. In the event of more groups being added that work the same way as the brainpool groups then this adds additional confusion which will be dependent on libssl version rather than provider version. It actually has the potential to cause some chaos in the event that existing third party providers have already added support for the TLSv1.3 brainpool curves and are already using them in 3.0 (which is technically possible). Added to this the extra complexity in libssl is just not worth it. I spent most of a day playing whack-a-mole with bugs related to this complexity when trying to address an unrelated group code bug.
All of these problems disappear when we simply use the TLS capabilities mechanism in the way that it was originally designed (which is the approach taken in this PR).
Doesn't this break backwards compatibility with possible existing third party providers? What if they put only the tls1.2 brainpool groups in the capabilities? They will not then support the brainpool curves in TLS-1.3 where they did with the original approach. Could we just special-case this somehow in libssl so if the provider capability gives a TLS-1.2 brainpool group it also adds the corresponding TLS-1.3 one? |
No - this shouldn't break anything. The existing TLSv1.3 brainpool support is in master only - not in 3.0. |
This partially reverts commit 0a10825 in order to reimplement it in a simpler way in the next commit. The reverted aspects are all related to the TLSv1.3 brainpool curves in the supported_groups extension. Rather than special casing the handling of these curves we simply add new entries to the groups table to represent them. They can then be handled without any additional special casing. This makes the code simpler to maintain.
Create new TLS_GROUP_ENTRY values for these groups.
Mention the brainpool curves in the documentation
mattcaswell
force-pushed
the
refactor-brainpool
branch
from
ac60d35
to
dbc11d7
Compare
|
Feedback has been addressed and I have rebased this to resolve a conflict with master. Please take another look. |
| @@ -914,7 +914,7 @@ my @tests_tls_1_3_non_fips = ( | |||
| #We only configured brainpoolP256r1 on the client side, but TLSv1.3 | |||
| #is enabled and this group is not allowed in TLSv1.3. Therefore this | |||
| #should fail | |||
There was a problem hiding this comment.
I still dont see why this should fail.
Why does the client not simple use tls1.2 when tls1.3 is not compatible with the
brainpoolP256r1 group?
There was a problem hiding this comment.
Endpoints don't automatically fallback to a lower protocol version if their initial configuration is inconsistent. So if you configure a client to use TLSv1.3, but you don't configure any groups that are suitable for use in TLSv1.3 you get the "No groups enabled for max supported SSL/TLS version" error message. Similarly the same happens with ciphersuites, i.e. if you enable TLSv1.3 but don't configure any TLSv1.3 capable ciphersuites you get the "No ciphers enabled for max supported SSL/TLS version" error message. This was a deliberate design decision, i.e. abort if your configuration is inconsistent in preference to falling back to a lower protocol version.
| nid = NID_brainpoolP384r1; | ||
| break; | ||
| case NID_brainpoolP512r1tls13: | ||
| nid = NID_brainpoolP512r1; |
There was a problem hiding this comment.
And what is this good for?
There was a problem hiding this comment.
This is to handle the 14-curves.cnf tests. Specifically the tests that specify the "ExpectedTmpKeyType". Previously "ExpectedTmpKeyType" expects a name for a curve. But we were passing the name for a TLS group. Until now this has been the same thing. Now we have some TLS groups that use a different name for the underlying curve.
t8m
removed
the
approval: otc review pending
|
Ping for second review |
beldmit
added
approval: done
openssl-machine
removed
the
approval: done
|
This pull request is ready to merge |
openssl-machine
added
the
approval: ready to merge
|
Pushed. Thanks. |
This partially reverts commit 0a10825 in order to reimplement it in a simpler way in the next commit. The reverted aspects are all related to the TLSv1.3 brainpool curves in the supported_groups extension. Rather than special casing the handling of these curves we simply add new entries to the groups table to represent them. They can then be handled without any additional special casing. This makes the code simpler to maintain. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #19315)
Create new TLS_GROUP_ENTRY values for these groups. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #19315)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #19315)
Mention the brainpool curves in the documentation Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #19315)
This partially reverts commit 0a10825 in order to reimplement it in a simpler way in the next commit. The reverted aspects are all related to the TLSv1.3 brainpool curves in the supported_groups extension. Rather than special casing the handling of these curves we simply add new entries to the groups table to represent them. They can then be handled without any additional special casing. This makes the code simpler to maintain. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#19315)
Create new TLS_GROUP_ENTRY values for these groups. Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#19315)
Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#19315)
Mention the brainpool curves in the documentation Reviewed-by: Tomas Mraz <tomas@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#19315)
Commit 0a10825 added support for brainpool curves in TLSv1.3. This only exists in the master branch at the current time. The supported_groups aspect of that change adds special case handling for the TLSv1.3 brainpool curves. This worked by converting the TLSv1.3 group id for the brainpool curve into the equivalent TLSv1.2 brainpool group id internally - and then back again when necessary. This unfortunately confuses the two different groups and turns out to be quite fragile when trying to make changes in this area.
In fact this special case handling should be unnecessary. It should simply be a matter of extending providers/common/capabilities.c with knowledge of the new groups.
This PR removes the special case handling and treats these groups in the same way as all other groups. It does mean that there are now 2 names for each of the brainpool curves - one for use up to TLSv1.2 and one for use from TLSv1.3 onwards. But this is consistent with the IANA supported_groups registry and so is probably the correct way to handle this. We also extend some of the testing of the brainpool groups.