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

Iotcrypt 237 hkdf doc #1920

Merged
merged 2 commits into from
Aug 30, 2018
Merged

Conversation

yanesca
Copy link
Contributor

@yanesca yanesca commented Aug 6, 2018

Description

Add warnings to the documentation of the HKDF module to reduce the risk of misusing the mbedtls_hkdf_extract() and mbedtls_hkdf_expand() functions. Fixes #1775.

Status

READY

Requires Backporting

NO

Migrations

NO

@yanesca yanesca added mbed TLS team needs-review Every commit must be reviewed by at least two team members, component-crypto Crypto primitives and low-level interfaces needs-ci Needs to pass CI tests labels Aug 6, 2018
* \param prk A pseudorandom key of at least md.size bytes. \p prk is usually,
* the output from the HKDF extract step.
* \param prk A pseudorandom key of at least md.size bytes. \p prk is
* usually, the output from the HKDF extract step.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this comma is not needed in this sentence.

ChangeLog Outdated
@@ -6,6 +6,12 @@ Bugfix
* Fixes an issue with MBEDTLS_CHACHAPOLY_C which would not compile if
MBEDTLS_ARC4_C and MBEDTLS_CIPHER_NULL_CIPHER weren't also defined. #1890

Changes

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't usually put a newline after the section title.

Patater
Patater previously approved these changes Aug 7, 2018
Copy link
Contributor

@Patater Patater left a comment

Choose a reason for hiding this comment

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

LGTM

I'd recommend a rebase into two commits after the review completes:

  1. HKDF: Fix style issue
  2. HKDF: Add warning to partial functions

@Patater
Copy link
Contributor

Patater commented Aug 7, 2018

CI failure is test_suite_timing on FreeBSD, which is a known issue.

@Patater
Copy link
Contributor

Patater commented Aug 7, 2018

Travis CI failure is odd: filed as #1925
We may have a flaky test on our hands.

@Patater
Copy link
Contributor

Patater commented Aug 13, 2018

retest

dgreen-arm
dgreen-arm previously approved these changes Aug 13, 2018
Copy link
Contributor

@dgreen-arm dgreen-arm left a comment

Choose a reason for hiding this comment

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

LGTM

@Patater Patater requested a review from moranpeker August 14, 2018 08:39
moranpeker
moranpeker previously approved these changes Aug 14, 2018
The standard HKDF security guarantees only hold if `mbedtls_hkdf()` is
used or if `mbedtls_hkdf_extract()` and `mbedtls_hkdf_expand()` are
called in succession carefully and an equivalent way.

Making `mbedtls_hkdf_extract()` and `mbedtls_hkdf_expand()` static would
prevent any misuse, but doing so would require the TLS 1.3 stack to
break abstraction and bypass the module API.

To reduce the risk of misuse we add warnings to the function
descriptions.
@yanesca yanesca dismissed stale reviews from moranpeker, dgreen-arm, and Patater via 08a4aeb August 14, 2018 15:09
@yanesca yanesca force-pushed the iotcrypt-237-hkdf-doc branch from d680211 to 08a4aeb Compare August 14, 2018 15:09
@yanesca
Copy link
Contributor Author

yanesca commented Aug 14, 2018

I have rebased the PR, it should be identical to the original. You can find the original branch here:
https://github.com/yanesca/mbedtls/tree/iotcrypt-237-hkdf-doc-orig

@moranpeker @Patater could you please review it again?

@Patater
Copy link
Contributor

Patater commented Aug 15, 2018

retest

1 similar comment
@Patater
Copy link
Contributor

Patater commented Aug 16, 2018

retest

@Patater Patater removed the needs-review Every commit must be reviewed by at least two team members, label Aug 16, 2018
@Patater
Copy link
Contributor

Patater commented Aug 16, 2018

retest

@Patater
Copy link
Contributor

Patater commented Aug 22, 2018

retest

@Patater Patater added approved Design and code approved - may be waiting for CI or backports and removed needs-ci Needs to pass CI tests labels Aug 28, 2018
@simonbutcher
Copy link
Contributor

This is labelled as 'Mbed TLS team', and is from Arm, so doesn't need the 'CLA not applicable' label.

@simonbutcher simonbutcher merged commit 08a4aeb into Mbed-TLS:development Aug 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants