Dispatch MAC operations through the driver interface #4247
Conversation
|
CI failures on 29421f1 (several other jobs are failing with the same errors):
|
bensze01
added
HwDrivers
needs-review
ronald-cron-arm
removed
the
needs-reviewer
There was a problem hiding this comment.
Mostly looked at the final code organization to start with.
I think things would be better organized with a file for hmac, a file for cmac both exposing a MAC driver interface and finally the dispatch being kept in psa_crypto.c in *_internal functions (like for sign/verify hash). Eventually (not in this PR), the *_internal functions will be merged into psa_crypto_driver_wrappers.c when it is auto-generated. I don't think we are too far from that. What do you think?
library/psa_crypto.c
Outdated
| operation->iv_required = 0; | ||
| operation->has_input = 0; | ||
| operation->is_sign = 0; | ||
| operation->ctx.mbedtls_ctx.alg = PSA_ALG_FULL_LENGTH_MAC( alg ); |
There was a problem hiding this comment.
Instead of changing it this way and then back to just "operation->" when moving the code to psa_crypto_mac.c please just change the name of the operation argument to mac_operation for example and introduce the local variable "operation = &operation->ctx.mbedtls_ctx". That way the code movement later will be easier to check with"git show --color-moved".
There was a problem hiding this comment.
I'll keep it in mind for future PRs
There was a problem hiding this comment.
You have to rebase anyway thus please do it in this PR. Otherwise, I will have to check the code move line by line to be sure that nothing like below (line 2560) happened.
There was a problem hiding this comment.
There's quite a difference in workload between a rebase to resolve merge conflicts and a fairly extensive rewrite (what you're asking for). It sounds like you care deeply, so I guess I'll do it, but I don't really see the point.
There was a problem hiding this comment.
The point is to facilitate review. Generally speaking, a commit should either do a single piece of refactoring or do a behavior change. Renaming identifiers and moving code are two different kinds of refactoring, so they shouldn't be done in the same commit.
|
I don't think I'm on board with creating overarching The final goal would be (as indicated) to also substitute the internal HMAC API usage with calls to the overarching MAC dispatch, since currently an accelerator capable of performing HMAC will not be used for doing HKDF / PRF, and instead potential acceleration would go through the hash layer instead, which is sub-optimal. I also assume that the end goal for PSA would be to build its own cipher-MAC (CMAC / CBC-MAC) codebase on top of the cipher API, since the current mbed TLS CMAC abstraction is needlessly complex, and relying on it requires fallback for accelerators which support AES but not AES-CMAC to implement the mbed TLS AES_ALT hooks in addition to the PSA Driver's cipher hooks. Ergo, I'd rather make a case for keeping the MAC 'software driver' implementation grouped together in a single file, much the same as they were grouped together previously in the PSA Core |
That would be temporary (as it is currently the case for psa_sign/verify_hash_internal()). Eventually the dispatch will be done in the driver wrappers. Saying that if you prefer we could probably even do it right now in the driver wrappers.
The path I see is to change the internal HMAC API to be the MAC driver API. Then the direct call to the HMAC driver would be changed to a call to the MAC driver wrappers APIs that would do the dispatch. No need for an generic MAC extra layer.
A generic MAC 'software driver' layer would make completely sense to me if there was a significant amount of shared code between HMAC and CMAC. But looking at the code in psa_crypto_mac.c I don't see that. That's why I am more in favour of having two separate MAC driver files, one for HMAC and one for CMAC. |
But that means added logic in the driver_wrappers file for the fallback case, since suddenly it would be wrapping two different drivers. And if it does that for the fallback case, then why aren't the HW accelerator drivers allowed the same flexibility? You could debate pulling out the respective implementations into psa_crypto_hmac and psa_crypto_cmac, and have psa_crypto_mac be the dispatch to those two, but IMO that brings little to the table other than needless fragmentation. |
Hardware accelerator drivers are allowed this flexibility, or at least will be when we finish implementing the driver interface. The dispatch takes the key type and algorithm into account. It's perfectly sensible to have an RSA driver that does both RSA signature and encryption, and an ECC driver that does both ECDSA and ECDH. I don't have a strong feeling on what the code structure should be now, but it would make sense to me to group things related to block ciphers together (so including MAC, cipher and AEAD functionality), and things related to hashes together (hash, HMAC, KMAC when we get around to merging it, …), and things related to RSA (signature and encryption), etc. |
stevew817
force-pushed
the
dispatch_mac_operations
branch
from
30d0452 to
ca5124a
Compare
|
First force-push: the commit flow changes requested by @ronald-cron-arm . This is a force-push without any changes (i.e. the end result is identical to what was in 30d0452, just the way to get there is different). |
stevew817
force-pushed
the
dispatch_mac_operations
branch
from
ca5124a to
19e4243
Compare
|
Second force-push: rebase to resolve merge conflicts with what has happened on upstream in the meantime. Resolutions:
Previous state: https://github.com/stevew817/mbedtls/tree/dispatch_mac_operations_1 |
|
Back on this, this morning. Regarding whether we should have a psa_crypto_mac.c or rather a psa_crypto_hmac.c and a psa_crypto_cmac.c or none of them and just psa_crypto_cipher.c and psa_crypto_hash.c (as mentioned by Gilles) I reckon there is more thinking to do. Let's forget about that aspect for this PR and stick with psa_crypto_mac.c. In my review I am stuck with the re-organization of the operation and driver contexts definitions for |
|
It's going to be a generic issue when multi-part algorithm implementations need to rely on other underlying multi-part primitives that already have a PSA implementation. It points to a certain hierarchy in the algorithms:
And since the built-with-cipher algorithms don't have a software implementation using PSA primitives yet (but rely on Mbed TLS' The reorganisation I did was what I thought to be the best way to go about fulfilling that dependency for the time being. Other approaches could be splitting them in e.g. Let me know which way you want to go, or if you have a better suggestion. |
|
I also think that introducing new headers seems a reasonable solution. Currently (in development) we have crypto_builtin.h that defines the operation structures for Mbed TLS software-based PSA drivers, crypto_driver_contexts.h that includes crypto_builtin.h and defines the operation structures for hardware drivers linked to the library and finally crypto_struct.h that includes crypto_driver_contexts.h and define the PSA client operation structures. By the way, looking at those headers and the headers they include I can see that crypto_driver_contexts.h includes crypto.h which is probably a mistake. We could introduce crypto_builtin_primitive.h, crypto_driver_contexts_primitive.h and crypto_struct_primitive.h where the operation structures for cipher and hash would be defined with the same inclusion structure as described above. Then crypto_builtin.h would include crypto_struct_primitive.h and crypto.h would include crypto_struct_primitive.h along with crypto_struct.h. I don't feel the need to add a _composite suffix to the name of the existing headers but don't have a strong opinion about it. If you decide to try this re-organization, please do it as preparatory commits and then rebase the MAC delegation on top it and not as additional patches. The history will be easier to understand as we will avoid crypto_builtin_mac.h being introduced and then removed. |
stevew817
force-pushed
the
dispatch_mac_operations
branch
from
19e4243 to
08d356d
Compare
|
Rebased with the fundamental header changes as prep work as requested. Steps:
|
There was a problem hiding this comment.
Thanks for the changes. I've done a complete review this time. I've been able to leverage the commit history to ease the review, thank you for that. It was not really easy with the three "Migrate ..." commits though, as they both move and modify code. It looks overall good to me but for two things:
- We miss a test_driver_mac.c (one as simple as test_driver_aead.c is enough) and corresponding tests in test_suite_psa_crypto_driver_wrappers.
- The HMAC internal interface is so close to be just a MAC PSA driver interface that is a bit of a pity not to switch to it.
library/psa_crypto_mac.h
Outdated
| @@ -23,15 +23,93 @@ | |||
|
|
|||
| #include <psa/crypto.h> | |||
|
|
|||
| /** Internal API for starting an HMAC operation, using PSA hash primitives. | |||
There was a problem hiding this comment.
It seems we are quite close for this interface to be the MAC PSA driver interface (but for clone). Thus, what about to do it ? For sign setup the API would be:
psa_status_t mbedtls_psa_hmac_sign_setup(
mbedtls_psa_hmac_operation_t *operation,
const psa_key_attributes_t *attributes,
const uint8_t *key_buffer, size_t key_buffer_size,
psa_algorithm_t alg )Then in psa_crypto.c instead of calling this HMAC PSA driver interface, call the driver wrapper interface and you get acceleration support if any.
There was a problem hiding this comment.
I thought about that while I was doing the original work, and then decided against doing it here because it would be a pretty invasive change. Doing it for HKDF is pretty straight-forward, but the way TLS 1.2 PRF is implemented requires being able to copy the HMAC context, and the PSA MAC API did not provide for that functionality. Meaning that on driver-accelerated (H)MAC, we don't currently have a way to copy the context structure, so for TLS 1.2 PRF it would regardless have meant that we need to stick to a pure-software implementation on top of PSA hash.
And given that outcome, I believe it doesn't make sense to do it for HKDF either. Meaning that if the need is gone, it doesn't quite make sense to put in the work either.
The alternative would be to move HKDF onto the MAC API, and move TLS 1.2 PRF directly onto the hash API (without mentioning HMAC). Is that preferable?
There was a problem hiding this comment.
I did end up converting the calls to the internal HMAC interface to regular MAC calls, see the last two commits
f9cc658 (converting HKDF) and 1878c0d (converting TLS 1.2 PRF and making the HMAC implementation static to the MAC driver).
The downside is that the context structure for TLS 1.2 PRF key derivation now contains the secret instead of the hashed secret. I don't consider this a risk, since an attacker can do as much with the hashed secret as with the actual secret. It also opens up a path for doing these operations with actual key as secret input instead of plaintext buffers, which would be good for eventually supporting e.g. an HKDF operation on an opaque key in an accelerator which only supports HMAC, or for storing the TLS 1.2 PRF secret as an opaque key.
If you don't like these, give the feedback and I can just drop them (these two commits) from the PR, in which case 562247a contains all other changes you requested (besides unification of HMAC with MAC API).
stevew817
force-pushed
the
dispatch_mac_operations
branch
from
08d356d to
197e0f8
Compare
|
Changes:
|
|
Thanks @ronald-cron-arm, fixed now. |
It makes sense to do the length checking in the core rather than expect each driver to deal with it themselves. This puts the onus on the core to dictate which algorithm/key combinations are valid before calling a driver. Additionally, this commit also updates the psa_mac_sign_finish function to better deal with output buffer sanitation, as per the review comments on Mbed-TLS#4247. Signed-off-by: Steven Cooreman <[email protected]>
The PSA core checks the key type and algorithm combination before calling the driver, so the driver doesn't have to do this once more. The PSA core will also not start an operation with a requested length which is larger than the full MAC output size, so the output length check in the driver isn't needed as long as the driver returns an error on mac_setup if it doesn't support the underlying hash algorithm. Signed-off-by: Steven Cooreman <[email protected]>
Signed-off-by: Steven Cooreman <[email protected]>
Signed-off-by: Steven Cooreman <[email protected]>
Signed-off-by: Steven Cooreman <[email protected]>
Signed-off-by: Steven Cooreman <[email protected]>
Signed-off-by: Steven Cooreman <[email protected]>
stevew817
force-pushed
the
dispatch_mac_operations
branch
from
eaa8d4e to
ae3ec52
Compare
|
I believe I addressed all of your feedback, @ronald-cron-arm , with the exception of the zero-check which I believe makes sense to leave there as a note for static analysers. |
library/psa_crypto.c
Outdated
|
|
||
| status = psa_mac_finish_internal( operation, mac, mac_size ); | ||
| /* Sanity checks on output buffer length. */ | ||
| if( mac_size == 0 || mac_size < operation->mac_size ) |
There was a problem hiding this comment.
As Ronald noted in https://github.com/ARMmbed/mbedtls/pull/4247/files#r629107039, it's surprising to check for mac_size == 0 when we know that it would imply mac_size < operation->mac_size since the operation setup guarantees that operation->mac_size > 0. You make a good point that this avoids false positives from static analyzers about cases that can only happen if mac_size == 0. However, it still warrants a comment, otherwise the next person who works on the code might decide to remove this apparently superfluous check.
How about changing the code to this?
/* Sanity check. This will guarantee that mac_size != 0 (and so mac != NULL) once all the error checks are done. */
if( operation->mac_size == 0 )
return( PSA_ERROR_BAD_STATE );
if( mac_size < operation->mac_size )
return( PSA_ERROR_BUFFER_TOO_SMALL );
A static analyzer should be just as happy, a compiler will only have a couple of instructions more to generate, this adds a bit of robustness, and the code looks less surprising.
library/psa_crypto_mac.c
Outdated
| return( mac_setup( operation, attributes, key_buffer, key_buffer_size, alg, | ||
| 1 ) ); | ||
| return( mac_setup( operation, | ||
| attributes, key_buffer, key_buffer_size, alg ) ); | ||
| } | ||
|
|
||
| static psa_status_t mac_verify_setup( |
There was a problem hiding this comment.
mac_verify_setup and mac_sign_setup are now identical. Why not call mac_setup directly?
Since a valid mac operation context would guarantee that the stored mac size is >= 4, it wasn't immediately obvious that the zero-length check is meant for static analyzers and a bit of robustness. Signed-off-by: Steven Cooreman <[email protected]>
Since they became equivalent after moving the is_sign checking back to the PSA core, they're now redundant, and the generic mac_setup function can just be called directly. Signed-off-by: Steven Cooreman <[email protected]>
stevew817
removed
the
needs-review
|
@stevew817 Forgot about it before to merge again, please create the backport to development_2.x. |
It makes sense to do the length checking in the core rather than expect each driver to deal with it themselves. This puts the onus on the core to dictate which algorithm/key combinations are valid before calling a driver. Additionally, this commit also updates the psa_mac_sign_finish function to better deal with output buffer sanitation, as per the review comments on Mbed-TLS#4247. Signed-off-by: Steven Cooreman <[email protected]>
It makes sense to do the length checking in the core rather than expect each driver to deal with it themselves. This puts the onus on the core to dictate which algorithm/key combinations are valid before calling a driver. Additionally, this commit also updates the psa_mac_sign_finish function to better deal with output buffer sanitation, as per the review comments on Mbed-TLS#4247. Signed-off-by: Steven Cooreman <[email protected]>
Description
Builds on the specification from #3933 and prior art from #4151, #4157, #4229 and #4240. Supersedes #4049.
psa_mac_xxxAPIs are now dispatched through thepsa_driver_wrappersinterface. There are a couple of to-be-done mentions in this PR, but they concern features that weren't present before this PR either. Namely:Status
READY
Requires Backporting
NO
(new functionality / internal refactoring)
Migrations
NO
(new functionality / internal refactoring)
Todos
Steps to test or reproduce
Tested by existing test suites