PSA: example programs for HMAC and AEAD vs legacy #5436

mpg · 2022-01-14T12:12:30Z

Description

Resolves #5208 "There are differences in how MD/Cipher and PSA represent information. Create two simple example programs showing the differences by performing the same operation with both APIs, with extensive comments."

Actually I didn't feel the need for that many comments, I thought just looking at the functions side by side is enough, with a summary comment at the top of the file. Reviewers are welcome to disagree of course.

Status

READY

Requires Backporting

NO

Steps to test or reproduce

make -C programs psa/hmac_md_psa && programs/psa/hmac_md_psa 

make -C programs psa/aead_cipher_psa
programs/psa/aead_cipher_psa gcm128
programs/psa/aead_cipher_psa gcm256
programs/psa/aead_cipher_psa gcm128_8
programs/psa/aead_cipher_psa chachapoly

This is meant to highlight similarities and differences in the multi-part HMAC APIs. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

This is meant to highlight similarities and differences in the APIs. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Existing example programs in this directory are already incompatible with that option, so this is probably acceptable here too. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

gilles-peskine-arm · 2022-01-17T23:40:40Z

programs/psa/aead_cipher_psa.c

+                         : mode == MBEDTLS_MODE_CHACHAPOLY ? "ChachaPoly"
+                         : "???";
+
+    printf( "cipher: %s, %d, %s, %zu\n", ciph, key_bits, mode_str, tag_len );


CI says we're still testing with a version of mingw that doesn't support %zu. (And grep says we do use it but only in memory_buffer_alloc). We use MBEDTLS_PRINTF_SIZET instead which has a workaround for old mingw.

Also this should be mbedtls_printf, not printf.

Also this should be mbedtls_printf, not printf.

I had a look at the other programs in programs/psa and they're using printf, so I decided to follow the flow. I'm happy to change to mbedtls_printf if you insist, though.

Regarding zu, I'm inclined to just cast to unsigned there's really no risk of overflow here, and to a reader who it's already familiar with it MBEDTLS_PRINTF_SIZET may be a distraction. (We are casting to unsigned in other programs, and so far the only uses of MBEDTLS_PRINTF_SIZET are in the library.)

I had a look at the other programs in programs/psa and they're using printf

That dates back from ARMmbed/mbed-crypto#74, which was motivated by the way we were trying to do crypto/tls separation at the time. I'm not sure that's the right decision today.

I do think it would be nicer to stick to standard functions in example code. On the other hand, this limits the code's portability. So the question is, to we care about being able to run sample program as-is on ultra-constrained or exotic platforms that lack e.g. printf or malloc or exit?

I do think it would be nicer to stick to standard functions in example code.

I agree. Although it reduces the sample programs portability, I think it is more useful like that for users. As a user, reading the sample code I would definitely wonder what these mbedtls_ prefixed functions do in the background. I wouldn't be certain if some library functions depend on that behaviour and if I am allowed to use standard functions.

Users of that ultra-constrained or exotic platforms need to hook up their implementations in the platform abstraction layer anyway in the first place and that couple of find and replace to make the sample code work, would be a negligible effort.

Keeping in mind that “exotic” platforms include not-that-ancient Windows, e.g. this discussion started with an error from using standard functionality on MinGW.

For these cases where we do care and the unportability is minor, we could define a standard name on platforms where it's missing. We don't do that in library code because it can be disruptive to invade the standard namespace (and this absolutely must not be done in a public header), but that's less of a concern in a sample program. For example, here, how about using PRIuPTR and defining it if it's unset and the implementation is MinGW?

Tbh, I never understood why mbedtls_printf() and all were introduced in the example programs in the first place.

Most of the programs are mostly educational: the goal is mainly to read them, run them on you development platform if it helps understanding how they work (perhaps with some editing etc.), and perhaps copy-paste bits and pieces to you own program. I can't see why you'll need or want to run them as-is on an embedded device.

Some of the the programs are not just educational, but could also be used as actual utilities, for example the x509 ones. Even then, I'm not convinced running them as-is on embedded devices is a reasonable goal: these programs take command-line options, which hardly make sense in an embedded context, so at least some adaptation will be needed anyway.

Some other programs are for tests (ssl_xxx2.c) - this is perhaps the thing you're most likely to want to run on target, but you also have the issue of command-line options, currently the program depend on the sockets API, which I'd say is much more of a constraint the things from libc...

Anyway, at some point we might want to rationalize that, but I hope we can get this PR merged before that :)

For example, here, how about using PRIuPTR and defining it if it's unset and the implementation is MinGW?

I think that's a reasonable option, but in this case I think using "%u", (unsigned) size works just as well, and it's also what we already do in several other places.

programs/psa/aead_cipher_psa.c

Both functions use int. Using size_t results is a warning from MSVC. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2022-01-18T08:34:18Z

@gilles-peskine-arm Thanks for taking a look. Sorry for not getting a clean CI before requesting a review. I'll wait for the CI results before requesting a re-review. (It's taking me more rounds than I expected to fully pass CI on these seemingly rather simple programs.)

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2022-01-25T11:29:51Z

@gilles-peskine-arm Ok, I finally passed the CI, so this is ready for you to review again.

@yanesca Do you intend to review this, or were you just commenting the %zu thing? (Just to know if this should still be labeled "needs: reviewer".)

yanesca · 2022-01-25T11:39:11Z

@mpg I was just commenting, didn't intend to be a reviewer.

programs/psa/aead_cipher_psa.c

programs/psa/hmac_md_psa.c

gilles-peskine-arm · 2022-01-26T18:12:38Z

programs/psa/aead_cipher_psa.c

+    psa_set_key_usage_flags( &attributes, PSA_KEY_USAGE_ENCRYPT );
+    psa_set_key_algorithm( &attributes, *alg );
+    psa_set_key_type( &attributes, key_type );
+    psa_set_key_bits( &attributes, key_bits );


You don't have to call set_key_bits when importing a key: this is automatically deduced from the imported data. (In theory there could be key types for which this is not true, but it's true for all of the currently defined types.) I don't know whether it's better to be explicit or terse here, given that this is an exampleprogram.

programs/psa/hmac_md_psa.c

programs/psa/aead_cipher_psa.c

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

- avoid hardcoded sizes when there's a macro for that - avoid mutable global variables - zeroize potentially-sensitive local buffer on exit Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Visual Studio and CMake didn't like having targets with the same name, albeit in different directories. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg · 2022-02-02T09:53:47Z

Passing CI again and ready for another round of review.

gilles-peskine-arm

Looks mostly good, just some minor issues and some optional suggestions.

programs/cipher/cipher_aead_demo.c

programs/psa/aead_demo.c

programs/cipher/cipher_aead_demo.c

programs/psa/aead_demo.c

gilles-peskine-arm · 2022-02-07T20:23:42Z

programs/cipher/cipher_aead_demo.c

+/* Dummy key material - never do this in production!
+ * 32-byte is enough to all the key size supported by this program. */
+const unsigned char key_bytes[32] = { 0x2a };


I'm not suggesting this for now, but for a follow-up: how about generating a random key instead? This would actually be good practice, and it would demonstrate how to generate a key (classic API: set up an entropy context, set up a DRBG, generate a byte string of a length matching the key size; PSA: call psa_generate_key).

key_ladder_demo has an example of using psa_import_key, so we wouldn't lose that. Or we could keep a static key in the HMAC programs, for variety.

I agree this would be a nice follow-up, though perhaps it could be its own example so as to not distract from the main purpose of comparing cipher APIs.

programs/cipher/cipher_aead_demo.c

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Used to print "cipher:" when it was the cipher part of a program that had both cipher and PSA. Now it doesn't really make sense. Align the output to match the PSA version of this program. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

gilles-peskine-arm · 2022-02-08T12:04:40Z

programs/cipher/cipher_aead_demo.c

-    const char *ciph = "???";
+    mbedtls_cipher_type_t type = mbedtls_cipher_get_type( ctx );
+    const mbedtls_cipher_info_t *info = mbedtls_cipher_info_from_type( type );
+    const char *ciph = mbedtls_cipher_info_get_name( info );


Minor: this results in output like

AES-128-GCM, 128, GCM, 16

Either AES, 128, GCM, 16 or AES-128-GCM, 16 would make more sense.

tom-daubney-arm

I re-ran my 2nd review of this PR yesterday after the comments had come in from the other review.This all LGTM now. Thanks.

mpg added 2 commits January 7, 2022 12:20

Add example program psa/hmac_md_psa

667b556

This is meant to highlight similarities and differences in the multi-part HMAC APIs. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add example program psa/aead_cipher_psa

398d459

This is meant to highlight similarities and differences in the APIs. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jan 14, 2022

mpg self-assigned this Jan 14, 2022

mpg mentioned this pull request Jan 14, 2022

Example programs for HMAC and AEAD PSA vs legacy #5435

Closed

Silence compiler warning in example program

ecffd96

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg force-pushed the prog-hmac-cipher-psa branch from 5501e24 to ecffd96 Compare January 14, 2022 12:25

mpg added 2 commits January 17, 2022 11:57

Declare incompatibility in new programs

9efbf53

Existing example programs in this directory are already incompatible with that option, so this is probably acceptable here too. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Rm use of non-standard __func__ in example programs

763641a

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

gilles-peskine-arm added needs-work needs-ci Needs to pass CI tests and removed needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review labels Jan 17, 2022

gilles-peskine-arm reviewed Jan 17, 2022

View reviewed changes

programs/psa/aead_cipher_psa.c Outdated Show resolved Hide resolved

Fix type of temporary variable

24e82de

Both functions use int. Using size_t results is a warning from MSVC. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Avoid using %zu, not supported everywhere yet.

aab5258

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg mentioned this pull request Jan 19, 2022

TLS Cipher 4: clean up #5207

Closed

2 tasks

mpg force-pushed the prog-hmac-cipher-psa branch from 72a9fb6 to aab5258 Compare January 19, 2022 09:37

mpg added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-work needs-ci Needs to pass CI tests labels Jan 25, 2022

mpg requested a review from gilles-peskine-arm January 25, 2022 11:27

gilles-peskine-arm requested changes Jan 26, 2022

View reviewed changes

Improve introductory comments.

0e725c3

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg added 6 commits January 31, 2022 12:30

Demonstrate better practices in HMAC examples

6349794

- avoid hardcoded sizes when there's a macro for that - avoid mutable global variables - zeroize potentially-sensitive local buffer on exit Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add comments to AEAD (non-PSA) examples

248b385

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Move hmac_non_psa out of psa/ directory

69bb3f5

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Move aead_non_psa out of the psa/ directory

6fdc9e8

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Avoid duplicate program names

29088a4

Visual Studio and CMake didn't like having targets with the same name, albeit in different directories. Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Fix bug in md_hmac_demo

12ec571

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg force-pushed the prog-hmac-cipher-psa branch from 4f7eba5 to 12ec571 Compare February 1, 2022 08:53

Work around bug in PSA_MAC_LENGTH()

f6ea19c

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg mentioned this pull request Feb 1, 2022

PSA macros: initializer element is not constant #4320

Open

mpg added needs-review Every commit must be reviewed by at least two team members, and removed needs-work labels Feb 2, 2022

mpg requested a review from gilles-peskine-arm February 2, 2022 09:53

tom-daubney-arm self-requested a review February 2, 2022 10:42

tom-daubney-arm removed the needs-reviewer This PR needs someone to pick it up for review label Feb 2, 2022

gilles-peskine-arm requested changes Feb 7, 2022

View reviewed changes

mpg added 7 commits February 8, 2022 10:54

Improve naming consistency

cf99beb

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Avoid hardcoding a size

48bae02

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Add comments on error codes

340808c

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Wrap long lines

64754e1

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Improve info() function in cipher_aead_demo

5e6c884

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

Give a magic constant a name

ae1bae8

Signed-off-by: Manuel Pégourié-Gonnard <manuel.pegourie-gonnard@arm.com>

mpg requested a review from gilles-peskine-arm February 8, 2022 10:38

gilles-peskine-arm approved these changes Feb 8, 2022

View reviewed changes

tom-daubney-arm approved these changes Feb 9, 2022

View reviewed changes

tom-daubney-arm removed the needs-review Every commit must be reviewed by at least two team members, label Feb 9, 2022

mpg added the approved Design and code approved - may be waiting for CI or backports label Feb 9, 2022

mpg merged commit 9193f7d into Mbed-TLS:development Feb 9, 2022

mpg deleted the prog-hmac-cipher-psa branch February 9, 2022 10:48

PSA: example programs for HMAC and AEAD vs legacy #5436

PSA: example programs for HMAC and AEAD vs legacy #5436

Uh oh!

Conversation

mpg commented Jan 14, 2022

Description

Status

Requires Backporting

Steps to test or reproduce

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanesca Jan 18, 2022 • 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!

gilles-peskine-arm Jan 18, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mpg commented Jan 18, 2022

Uh oh!

mpg commented Jan 25, 2022

Uh oh!

yanesca commented Jan 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpg commented Feb 2, 2022

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tom-daubney-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yanesca Jan 18, 2022 •

edited

Loading

gilles-peskine-arm Jan 18, 2022 •

edited

Loading