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.

Sign up for GitHub

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

Jump to bottom

Add ecc extensions only if ecc ciphersuite is used #1378

Merged
merged 5 commits into from
Aug 10, 2018
Merged

Add ecc extensions only if ecc ciphersuite is used #1378

merged 5 commits into from
Aug 10, 2018

Conversation

RonEld
Copy link
Contributor

@RonEld RonEld commented Feb 16, 2018 •
edited
Loading

Description

Fix compliancy to RFC4492. ECC extensions should be included
only if ec ciphersuites are used. Interoperability issue with
bouncy castle. Resolves #1157

Status

IN DEVELOPMENT

Requires Backporting

Yes
Which branch?

comments

Missing tests, ChangeLog

Todos

  • Tests
  • Documentation
  • Changelog updated
  • Backported

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

@mpg mpg added the needs-review Every commit must be reviewed by at least two team members, label Apr 17, 2018
@simonbutcher simonbutcher requested a review from mpg June 21, 2018 12:35
@simonbutcher simonbutcher added the needs-backports Backports are missing or are pending review and approval. label Jun 21, 2018
@simonbutcher simonbutcher requested a review from andresag01 June 21, 2018 12:35
Add ecc extensions only if ecc ciphersuite is used
755bb6a 
Fix compliancy to RFC4492. ECC extensions should be included
only if ec ciphersuites are used. Interoperability issue with
bouncy castle. Mbed-TLS#1157
@RonEld RonEld mentioned this pull request Jun 21, 2018
Closed
@RonEld
Copy link
Contributor Author

RonEld commented Jun 21, 2018

Rebased and added an entry in the ChangeLog

Add entry in ChangeLog
3f38cf7 
Add an entry in the ChangeLog, describing the fix.
mpg
mpg requested changes Jun 25, 2018
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Besides the suggested improvements to the ChangeLog entry, I think this PR lacks a test.

It should be easy enough to add one in ssl-opt.sh by forcing a non-ECC ciphersuite and then checking for the absence of the ECC extensions in the debug logs of the client and server. Also have a test where the client offers many suites, some of them ECC, and the server picks a non-ECC suite. (And perhaps, just to be sure, a test with an ECC suite just to be extra sure the extensions are present when they should be, and contrast with the other two tests.)

ChangeLog
= mbed TLS x.x.x branch released xxxx-xx-xx

Bugfix
* Add ecc extensions only if an ecc based ciphersuite is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think ChangeLog entries should provide more context. For example here, "In TLS, add ECC extensions to the ClientHello and ServerHello messages only if". (And ECC should be capitalized while at it.)

Also, "Affects interop" is ambiguous and not a complete sentence. How about: "This improves compliance to RFC 4492, and as a result, solves interoperability issues with BouncyCastle."?

@RonEld RonEld added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 26, 2018
Ron Eldor added 2 commits June 28, 2018 11:09
Update ChangeLog
84e62f8 
Update ChangeLog with a less ambigous description.
Add ECC extensions test in ssl-opts.sh
58093c8 
Add test to verify if an ecc based extension exists
or not if an ecc based ciphersuite is used or not.
@RonEld
Copy link
Contributor Author

RonEld commented Jun 28, 2018

@mpg I added a test i ssl-opts.sh
Do you think interop tests should be added as well?

@RonEld RonEld added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Jun 28, 2018
mpg
mpg previously approved these changes Jun 28, 2018
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test!

I don't think interop tests are necessary here, as the test in ssl-opt.sh is enough to ensure we now behave in a compliant way, and adding Bouncycastle to compat.sh would be too much work for this PR.

@mpg
Copy link
Contributor

mpg commented Jun 28, 2018

Note: CI only failed in the timing test that is known to be flaky (#1517), so not a blocker for merging.

@RonEld
Copy link
Contributor Author

RonEld commented Jun 28, 2018

@mpg

and adding Bouncycastle to compat.sh would be too much work for this PR.

I meant interop tests with openssl and GnuTLS, but I see your point. Thanks for approving!

Update ssl-opt.sh test to run condition
643df7c 
1. Update the test script to un the ECC tests only if the relevant
configurations are defined in `config.h` file
2. Change the HASH of the ciphersuite from SHA1 based to SHA256
for better example
@RonEld
Copy link
Contributor Author

RonEld commented Jun 28, 2018

@mpg I updated the test to run conditionally only if the relevant modules are defined in the configuration.
In addition, I change the ciphersuite from a SHA1 based hash to a SHA256, for a better example.
Please review

mpg
mpg approved these changes Jun 28, 2018
View reviewed changes
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Nice improvements.

This was referenced Jun 28, 2018
Merged
Merged
@RonEld
Copy link
Contributor Author

RonEld commented Jun 28, 2018

Backports available in #1814 and #1815

@andresag01 andresag01 removed needs-backports Backports are missing or are pending review and approval. needs-review Every commit must be reviewed by at least two team members, labels Jun 28, 2018
@RonEld
Copy link
Contributor Author

RonEld commented Jul 24, 2018

This PR and its backports have been fully approved

cc @sbutcher-arm

@simonbutcher simonbutcher added approved Design and code approved - may be waiting for CI or backports approved for design labels Jul 31, 2018
@Patater Patater merged commit 643df7c into Mbed-TLS:development Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andresag01 andresag01 andresag01 approved these changes

@mpg mpg mpg approved these changes

Assignees
No one assigned
Labels
approved Design and code approved - may be waiting for CI or backports bug component-tls
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RonEld @mpg @andresag01 @Patater @simonbutcher