Remove Diffie-Hellman examples #80

yanesca · 2019-03-05T11:17:02Z

These examples rely on the NET module, which we want to remove. In
theory we could remove just the dependency, but we decided to remove the
whole example because:

They showcase some bad crypto: custom, undocumented protocol (not
obviously broken though, apart from authenticating only one side);
hard-coded limit of 512-bit size for the DH modulus (2048 is the
recommended minimum these days); direct use of the shared secret as a
key (instead of applying a KDF); encryption with ECB, custom
parameters and the client not having the ability to verify them.
The programs use the DH API in the same way that TLS does, so they
have limited demonstration value.
The programs only show finite-field DH, which is not used all that
much these days. What people want to see is mostly ECDH.

Patater · 2019-03-05T11:22:02Z

Are there any CMakeLists or Makefile or visual studio files to update?

gilles-peskine-arm · 2019-03-05T11:28:43Z

All of these files need updating:

$ git grep -l dh_client
ChangeLog
programs/.gitignore
programs/Makefile
programs/README.md
programs/pkey/CMakeLists.txt
programs/pkey/dh_client.c
visualc/VS2010/dh_client.vcxproj
visualc/VS2010/mbedTLS.sln

yanesca · 2019-03-05T11:35:11Z

I remember talking about it, but I don't remember how we decided, do we want to keep updating ChangeLog in mbed-crypto?

gilles-peskine-arm · 2019-03-05T11:43:33Z

We've talked about it but not decided anything. In the meantime, I propose to keep updating ChangeLog with an “Mbed Crypto” entry for anything that impacts the non-PSA API (but not for PSA API stuff because we have a huge backlog and no strong promise of backward compatibility yet).

Patater · 2019-03-05T13:40:56Z

Please don't add any Mbed Crypto entries to the ChangeLog. Feel free to start a wiki page for the next version's release notes, though. Until we have a solution to ChangeLog maintenance and automatic merging, it's not worth the effort. We can bring it back later if it makes sense.

yanesca · 2019-03-06T11:52:55Z

The PR is ready for review again.

gilles-peskine-arm

Looks ok except for the ChangeLog entry. Also, Jaeden wanted to keep the changelog entry somewhere else (I have no strong feelings about that point).

gilles-peskine-arm · 2019-03-06T12:21:12Z

ChangeLog

@@ -64,6 +64,9 @@ Features
   * Add a new function mbedtls_asn1_write_named_bitstring() to write ASN.1
     named bitstring in DER as required by RFC 5280 Appendix B.

+Mbed Crypto
+   * Removed two Diffie-Hellman examples which were relying on the Net module.


Changelog entries are primarily for users of the library. Most users of the library don't care much about module dependencies, but they might care about the example programs. So we should tell them what to look at instead. Proposal:

Remove the Diffie-Hellman examples which implemented a toy protocol inspired by TLS DH key exchange. For an example of how to use the DHM module, see the code that calls mbedtls_dhm_xxx in ssl_tls.c and ssl_cli.c in Mbed TLS.

Fixing it. (I had removed it before the review, just forgot to push.)

Patater · 2019-03-06T12:59:24Z

programs/Makefile

@@ -49,8 +49,7 @@ endif

 APPS =	aes/aescrypt2$(EXEXT)		aes/crypt_and_hash$(EXEXT)	\
 	hash/hello$(EXEXT)		hash/generic_sum$(EXEXT)	\
-					pkey/dh_client$(EXEXT)		\
-	pkey/dh_genprime$(EXEXT)	pkey/dh_server$(EXEXT)		\
+	pkey/dh_genprime$(EXEXT)		\


Check alignment of \.

Fixed in ff43390.

Patater · 2019-03-06T13:02:34Z

ChangeLog

@@ -64,6 +64,9 @@ Features
   * Add a new function mbedtls_asn1_write_named_bitstring() to write ASN.1
     named bitstring in DER as required by RFC 5280 Appendix B.

+Mbed Crypto


Please add release note items to our upcoming release's release notes page. We can edit them and get them into shape before publishing separately from code changes.

I have added the text suggested by @gilles-peskine-arm to the page.

yanesca · 2019-03-06T15:13:27Z

The PR is ready for review again.

Patater · 2019-03-06T15:32:18Z

programs/Makefile

-	pkey/rsa_sign$(EXEXT)		pkey/rsa_verify$(EXEXT)		\
-	pkey/rsa_sign_pss$(EXEXT)	pkey/rsa_verify_pss$(EXEXT)	\
-	psa/crypto_examples$(EXEXT)					\
+	hash/hello$(EXEXT)		hash/generic_sum$(EXEXT)			\


Could you align with tab-width 8 please?

These examples rely on the NET module, which we want to remove. In theory we could remove just the dependency, but we decided to remove the whole example because: - They showcase some bad crypto: custom, undocumented protocol (not obviously broken though, apart from authenticating only one side); hard-coded limit of 512-bit size for the DH modulus (2048 is the recommended minimum these days); direct use of the shared secret as a key (instead of applying a KDF); encryption with ECB, custom parameters and the client not having the ability to verify them. - The programs use the DH API in the same way that TLS does, so they have limited demonstration value. - The programs only show finite-field DH, which is not used all that much these days. What people want to see is mostly ECDH.

Patater

LGTM

yanesca added the needs: review The pull request is ready for review. This generally means that it has no known issues. label Mar 5, 2019

yanesca requested review from Patater and gilles-peskine-arm March 5, 2019 11:17

gilles-peskine-arm added needs: work The pull request needs rework before it can be merged. and removed needs: review The pull request is ready for review. This generally means that it has no known issues. labels Mar 5, 2019

yanesca force-pushed the iotcrypt-685-rewrite-dh-example branch 2 times, most recently from 6b8d597 to 0307707 Compare March 5, 2019 12:26

yanesca added needs: review The pull request is ready for review. This generally means that it has no known issues. and removed needs: work The pull request needs rework before it can be merged. labels Mar 5, 2019

gilles-peskine-arm requested changes Mar 6, 2019

View reviewed changes

Patater suggested changes Mar 6, 2019

View reviewed changes

yanesca force-pushed the iotcrypt-685-rewrite-dh-example branch from 0307707 to ce1e24b Compare March 6, 2019 14:27

Patater reviewed Mar 6, 2019

View reviewed changes

yanesca force-pushed the iotcrypt-685-rewrite-dh-example branch from ff43390 to bea98b4 Compare March 6, 2019 15:40

gilles-peskine-arm approved these changes Mar 6, 2019

View reviewed changes

Patater approved these changes Mar 6, 2019

View reviewed changes

Patater merged commit 98c2208 into ARMmbed:development Mar 7, 2019

This was referenced Mar 4, 2020

Experimental merge of Mbed Crypto Mbed-TLS/mbedtls#3078

Closed

[DO NOT MERGE] Unremove tls files in preparation for the repository merge #384

Closed

Remove Diffie-Hellman examples #80

Remove Diffie-Hellman examples #80

Uh oh!

Conversation

yanesca commented Mar 5, 2019

Uh oh!

Patater commented Mar 5, 2019

Uh oh!

gilles-peskine-arm commented Mar 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanesca commented Mar 5, 2019

Uh oh!

gilles-peskine-arm commented Mar 5, 2019

Uh oh!

Patater commented Mar 5, 2019

Uh oh!

yanesca commented Mar 6, 2019

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

yanesca Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

yanesca Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

Patater Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

yanesca Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

yanesca commented Mar 6, 2019

Uh oh!

Patater Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanesca Mar 6, 2019

Choose a reason for hiding this comment

Uh oh!

Patater left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm commented Mar 5, 2019 •

edited

Loading

Patater Mar 6, 2019 •

edited

Loading