gh-ARMmbed/mbed-crypto

Watch

Remove domain parameters from API 1.0 #112

Merged

gilles-peskine-arm merged 6 commits from psa-remove_domain_parameters into psa-api-1.0-beta

2019-05-17 03:55:14 -05:00

gilles-peskine-arm commented

2019-05-16 07:06:33 -05:00

(Migrated from github.com)

Domain parameters aren't quite working yet. So remove them from the API, but keep them as an experimental extension in our reference implementation. At this point, we plan to fix the domain parameter API and add them to the specification in a point release, possibly in a modified form.

This has the following impact for applications:

There is no longer a way to generate an RSA key with e≠65537.
Diffie-Hellman keys must use one of the groups from RFC 7919.
DSA is no longer referenced and cannot be implemented without some API extension.

Domain parameters aren't quite working yet. So remove them from the API, but keep them as an experimental extension in our reference implementation. At this point, we plan to fix the domain parameter API and add them to the specification in a point release, possibly in a modified form. This has the following impact for applications: * There is no longer a way to generate an RSA key with e≠65537. * Diffie-Hellman keys must use one of the groups from RFC 7919. * DSA is no longer referenced and cannot be implemented without some API extension.

yanesca (Migrated from github.com) reviewed

2019-05-16 08:38:53 -05:00

include/psa/crypto.h

Shouldn't we remove psa_reset_key_attributes() from the API too?

Shouldn't we remove `psa_reset_key_attributes()` from the API too?

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 09:01:43 -05:00

include/psa/crypto.h

No. It stays. You can get away without it now, but it's extremely likely that a future API version will require it. You are supposed to call it after psa_get_key_attributes.

No. It stays. You can get away without it now, but it's extremely likely that a future API version will require it. You are supposed to call it after `psa_get_key_attributes`.

yanesca (Migrated from github.com) reviewed

2019-05-16 09:10:15 -05:00

tests/suites/test_suite_psa_crypto_metadata.function

Copypasta: this should be PSA_KEY_TYPE_DH_PUBLIC_KEY. Same goes for the macros in the lines below.

Copypasta: this should be `PSA_KEY_TYPE_DH_PUBLIC_KEY`. Same goes for the macros in the lines below.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 09:15:52 -05:00

tests/suites/test_suite_psa_crypto_metadata.function

I made a fixup commit to fix this and add KEY_TYPE_IS_DH.

I made a fixup commit to fix this and add `KEY_TYPE_IS_DH`.

yanesca (Migrated from github.com) reviewed

2019-05-16 09:17:07 -05:00

include/psa/crypto.h

Do you say that it is extremely likely, because of the reintroduction of key_attributes or do you have some other use case in mind as well?

Do you say that it is extremely likely, because of the reintroduction of `key_attributes` or do you have some other use case in mind as well?

yanesca (Migrated from github.com) reviewed

2019-05-16 09:27:16 -05:00

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 09:27:40 -05:00

include/psa/crypto.h

Domain parameters are the definite thing I have in mind. But also I'd be surprised if nothing else came up, especially when we consider more server and other rich platform use cases.

Domain parameters are the definite thing I have in mind. But also I'd be surprised if nothing else came up, especially when we consider more server and other rich platform use cases.

k-stachowiak (Migrated from github.com) reviewed

2019-05-16 10:06:58 -05:00

k-stachowiak (Migrated from github.com) left a comment

I found a small mistake in one of the comments.

I found a small mistake in one of the comments.

include/psa/crypto_extra.h

"Either" is duplicated in this sentence.

"Either" is duplicated in this sentence.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 10:25:16 -05:00

include/psa/crypto_extra.h

Fixed here and in other places that had the same copypasted sentence.

Fixed here and in other places that had the same copypasted sentence.

gilles-peskine-arm commented

2019-05-16 12:32:17 -05:00

(Migrated from github.com)

Rebased on top of the target branch. The previous version is at https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-remove_domain_parameters-1

Rebased on top of the target branch. The previous version is at https://github.com/gilles-peskine-arm/mbed-crypto/tree/psa-remove_domain_parameters-1

Patater (Migrated from github.com) reviewed

2019-05-16 13:28:21 -05:00

include/psa/crypto.h

					
				@ -109,3 +108,3 @@

				 *   for some key type additional domain parameters.

				 * - Information about the key itself: the key type and its size.

				 * - Implementations may define additional attributes.

				 *

Commit message for "Remove domain parameters from the official API " says "This leaves DH and DSA effectively unusable.", but isn't it usable with implementation-specific extensions?

Our RSA implementation also uses "domain parameters". Should we mention that RSA is effectively unusable?
...minutes later...
I see in a later commit we now require e=65537, so RSA should be perfectly usable. No problem for RSA here.

Commit message for "Remove domain parameters from the official API " says "This leaves DH and DSA effectively unusable.", but isn't it usable with implementation-specific extensions? Our RSA implementation also uses "domain parameters". Should we mention that RSA is effectively unusable? ...minutes later... I see in a later commit we now require e=65537, so RSA should be perfectly usable. No problem for RSA here.

include/psa/crypto.h

I'm not sure we should make this optional. Certain implementations might require psa_reset_key_attributes() to be called to free implementation-specific resources like domain parameters. It'd also be nice if applications would continue to work when domain parameters become part of the official API.

I'm not sure we should make this optional. Certain implementations might require `psa_reset_key_attributes()` to be called to free implementation-specific resources like domain parameters. It'd also be nice if applications would continue to work when domain parameters become part of the official API.

include/psa/crypto_values.h

Do we need to provide an implementation specific PSA_ALG_IS_HASH_AND_SIGN() that understands PSA_ALG_IS_DSA()?

Do we need to provide an implementation specific `PSA_ALG_IS_HASH_AND_SIGN()` that understands `PSA_ALG_IS_DSA()`?

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 14:19:36 -05:00

include/psa/crypto_values.h

Yes, if we implement DSA, which we don't.

The current tooling doesn't allow us to document a macro without giving a definition for it. When we move away from Doxygen we'll be able to do it.

Yes, if we implement DSA, which we don't. The current tooling doesn't allow us to document a macro without giving a definition for it. When we move away from Doxygen we'll be able to do it.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 14:19:52 -05:00

include/psa/crypto.h

We debated this internally and concluded that we might as well allow it.

We debated this internally and concluded that we might as well allow it.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 14:20:19 -05:00

include/psa/crypto.h

					
				@ -109,3 +108,3 @@

				 *   for some key type additional domain parameters.

				 * - Information about the key itself: the key type and its size.

				 * - Implementations may define additional attributes.

				 *

Sure, everything is usable with implementation-specific extensions.

Sure, everything is usable with implementation-specific extensions.

Patater (Migrated from github.com) reviewed

2019-05-16 14:39:49 -05:00

include/psa/crypto.h

I’m proposing stronger than allowing it, but to maintain requiring it. (We removed the word "must" in this PR, but I'd recommend keeping the word "must" in.)

I’m proposing stronger than allowing it, but to maintain requiring it. (We removed the word "must" in this PR, but I'd recommend keeping the word "must" in.)

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 14:46:31 -05:00

include/psa/crypto_values.h

But I actually did preserve the definition with DSA support in crypto_extra.h, anyway.

But I actually did preserve the definition with DSA support in `crypto_extra.h`, anyway.

Patater (Migrated from github.com) reviewed

2019-05-16 14:48:48 -05:00

include/psa/crypto.h

					
				@ -109,3 +108,3 @@

				 *   for some key type additional domain parameters.

				 * - Information about the key itself: the key type and its size.

				 * - Implementations may define additional attributes.

				 *

The commit message is all lies!

The commit message is all lies!

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-16 14:50:08 -05:00

include/psa/crypto.h

See internal discussion https://github.com/ARMmbed/psa-crypto/issues/140#issuecomment-486767289 and https://github.com/ARMmbed/mbedtls-psa/pull/263#discussion_r279832614

See internal discussion https://github.com/ARMmbed/psa-crypto/issues/140#issuecomment-486767289 and https://github.com/ARMmbed/mbedtls-psa/pull/263#discussion_r279832614

Patater (Migrated from github.com) reviewed

2019-05-17 02:09:04 -05:00

include/psa/crypto.h

I've read those threads.

My recommendation is to encourage future-proof use of the API by always requiring a reset.

-  You may call psa_reset_key_attributes(), although this is optional...
+ You must call psa_reset_key_attributes() to free any resources used by the attributes structure.

I can see your chosen approach is to only require calling reset when it might be actually necessary; and without key domain parameters, Mbed Crypto doesn't allocate resources when handling attributes. I think this assumes too much about the implementation, as some implementations could require the user calling reset for the attributes currently specified, although Mbed Crypto wouldn't. Applications written for an implementation that doesn't require calling reset will not be portable to implementations that require reset.

If we dont't want to always require it, I'd suggest removing reset from the API for now, so that no implementations that require it appear and cause application portability issues. Then, when we add domain parameters back or other attributes that we'd expect implementations to allocate resources for, we can take the "call it if you set any of these attributes" approach again. New applications that use these new attributes will have to call reset, but old applications that don't use those attributes will remain correct.

I've read those threads. My recommendation is to encourage future-proof use of the API by always requiring a reset. ```diff - You may call psa_reset_key_attributes(), although this is optional... + You must call psa_reset_key_attributes() to free any resources used by the attributes structure. ``` I can see your chosen approach is to only require calling reset when it might be actually necessary; and without key domain parameters, Mbed Crypto doesn't allocate resources when handling attributes. I think this assumes too much about the implementation, as some implementations could require the user calling reset for the attributes currently specified, although Mbed Crypto wouldn't. Applications written for an implementation that doesn't require calling reset will not be portable to implementations that require reset. If we dont't want to always require it, I'd suggest removing reset from the API for now, so that no implementations that require it appear and cause application portability issues. Then, when we add domain parameters back or other attributes that we'd expect implementations to allocate resources for, we can take the "call it if you set any of these attributes" approach again. New applications that use these new attributes will have to call reset, but old applications that don't use those attributes will remain correct.

yanesca (Migrated from github.com) reviewed

2019-05-17 02:49:15 -05:00

include/psa/crypto.h

I don't have strong feelings about this, but for me the removal seems the best solution. Mainly because I am secretly hoping that we can get away without adding domain parameters back:

I think we have a reasonable compromise for RSA, it has less significance and it's popularity is decreasing.
I don't think anybody will mourn DSA
For FFDH named parameters are the safest (if you are worried about logjam, don't use small modulus) and we can use an ECC-like mechanism as we do it now.
For PQ schemes to come: fiddling with the domain parameters has usually non-trivial impact on security strength and anything else than named parameters seems impractical to me. (The exception is hash based PQ, but there the parameters are as simple as with symmetric crypto.)

I don't have strong feelings about this, but for me the removal seems the best solution. Mainly because I am secretly hoping that we can get away without adding domain parameters back: - I think we have a reasonable compromise for RSA, it has less significance and it's popularity is decreasing. - I don't think anybody will mourn DSA - For FFDH named parameters are the safest (if you are worried about logjam, don't use small modulus) and we can use an ECC-like mechanism as we do it now. - For PQ schemes to come: fiddling with the domain parameters has usually non-trivial impact on security strength and anything else than named parameters seems impractical to me. (The exception is hash based PQ, but there the parameters are as simple as with symmetric crypto.)

athoelke (Migrated from github.com) reviewed

2019-05-17 03:14:08 -05:00

include/psa/crypto.h

I'm also feeling that this halfway house is not ideal. There's still the issue with how to extract a variable sized key attributes in an efficient way when we reintroduce them. I think the real options are:

Extended attributes are not part of the psa_key_attributes_t.
- When we re-introduce domain parameters and other similar 'large' attributes, we create a different type to hold them.
- If you need to extract them, you call an extra API to retrieve them and then release them later.
- When creating keys, we either allow the extended attributes to be referenced by the key_attributes (but not owned), or add extra parameters to the creation APIs. (I prefer the former)
- Retain reset() to zero the attributes if apps want to do this, is is not required to be called. Releasing memory is an operation on the extended attributes.
Make reset() mandatory for applications to use in this version of the spec. When domain params are added to the attributes, reset() starts checking if there is memory to be released. This adds cost to all apps as many won't touch the zero'd attributes after reset and many compilers will not elide the pointless memset().
Remove reset() from the API right now. Reintroduce it as "conditional mandatory" - if you use domain parameters then you must call reset()

I think I actually prefer approach 1. It moves the tricky memory management into a separate structure and allows psa_key_attributes_t to always be a simple plain-old-data struct.

I'm also feeling that this halfway house is not ideal. There's still the issue with how to extract a variable sized key attributes in an efficient way when we reintroduce them. I think the real options are: 1. Extended attributes are not part of the psa_key_attributes_t. * When we re-introduce domain parameters and other similar 'large' attributes, we create a different type to hold them. * If you need to extract them, you call an extra API to retrieve them and then release them later. * When creating keys, we either allow the extended attributes to be referenced by the key_attributes (but not owned), or add extra parameters to the creation APIs. (I prefer the former) * Retain `reset()` to zero the attributes if apps want to do this, is is not required to be called. Releasing memory is an operation on the extended attributes. 2. Make `reset()` mandatory for applications to use in this version of the spec. When domain params are added to the attributes, `reset()` starts checking if there is memory to be released. This adds cost to all apps as many won't touch the zero'd attributes after reset and many compilers will not elide the pointless memset(). 3. Remove `reset()` from the API right now. Reintroduce it as "conditional mandatory" - **if you use domain parameters then you must call `reset()`** I think I actually prefer approach 1. It moves the tricky memory management into a separate structure and allows `psa_key_attributes_t` to always be a simple plain-old-data struct.

Patater (Migrated from github.com) reviewed

2019-05-17 03:52:21 -05:00

include/psa/crypto.h

The risk of some implementations requiring free to come up for the currentlt defined parameters is fairly low. Reset just zeros in this version, and is optional.

The risk of some implementations requiring free to come up for the currentlt defined parameters is fairly low. Reset just zeros in this version, and is optional.

Patater (Migrated from github.com) approved these changes

2019-05-17 03:52:38 -05:00

Patater commented

2019-05-17 03:54:21 -05:00

(Migrated from github.com)

CI failure is ABI job (known to fail until https://github.com/ARMmbed/mbedtls/pull/2636 lands in the development branch) and USE_PSA_CRYPTO, which doesn't work on the API branch.

CI failure is ABI job (known to fail until https://github.com/ARMmbed/mbedtls/pull/2636 lands in the development branch) and `USE_PSA_CRYPTO`, which doesn't work on the API branch.

gilles-peskine-arm (Migrated from github.com) reviewed

2019-05-17 03:54:49 -05:00

include/psa/crypto.h

With my gatekeeper hat for the API, I say: this stays as is in beta3. It's out of scope of this PR anyway. Let's discuss it further for the next beta: we can still make reset officially mandatory or completely optional. This commits us to keeping the function, but it's useful anyway to zero the structure.

With my gatekeeper hat for the API, I say: this stays as is in beta3. It's out of scope of this PR anyway. Let's discuss it further for the next beta: we can still make reset officially mandatory or completely optional. This commits us to keeping the function, but it's useful anyway to zero the structure.

Sign in to join this conversation.

No reviewers

No labels

good first issue

HEADER FORMATTING ISSUE

Jira status: AWAITING REVIEW

Jira status: CLOSED

Jira status: DONE

Jira status: IN PROGRESS

Jira status: IN REVIEW

Jira status: OPEN

Jira status: REOPENED

Jira status: RESOLVED

mirror internally

needs: backports

needs: design review

needs: preceding PR

open for community contribution

Product Backlog

ready for merge

type: enhancement

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-ARMmbed/mbed-crypto!112

No description provided.