PSA Sign Hash (Interruptible) Design #6279

paul-elliott-arm · 2022-09-13T16:43:49Z

Description

A design for the psa_sign_hash (interruptible) interface. (Implementation to follow once design agreed). Fixes #6061

Draft for an upcoming PSA Crypto specification version.

Status

READY

Requires Backporting

NO

Migrations

NO

Todos

Tests

include/psa/crypto_extra.h

paul-elliott-arm · 2022-09-15T08:06:56Z

CI Fail is due to 'missing' error code. I could fix this by defining it in file for the time being, if this is acceptable.

gilles-peskine-arm · 2022-09-15T08:26:59Z

You need to define the new error code, either in crypto_values.h or in crypto_extra.h.

include/psa/crypto_extra.h

Signed-off-by: Paul Elliott <[email protected]>

gilles-peskine-arm

I marked my comments as [T] for technical content that might affect the work in progress on implementation, and [E] for purely editorial comments.

There is quite a bit of repeated content (e.g. same note on sign and verify functions). When a comment of mine applies in multiple places, I have generally only reported it once. All comments in this review should be understood as “also applies in other places where applicable”.

I have verified that all my and @tom-cosgrove-arm 's comments from previous reviews have been addressed (or in one case, re-raised).

gilles-peskine-arm · 2022-12-04T23:23:23Z

include/psa/crypto.h

+ */
+
+/** The type of the state data structure for interruptible hash
+ *  verification operations.


[E]

Suggested change

* verification operations.

* signing operations.

gilles-peskine-arm · 2022-12-04T23:23:34Z

include/psa/crypto.h

+ * Implementation details can change in future versions without notice. */
+typedef struct psa_sign_hash_interruptible_operation_s psa_sign_hash_interruptible_operation_t;
+
+/** The type of the state data structure for interruptible hash signing


[E]

Suggested change

/** The type of the state data structure for interruptible hash signing

/** The type of the state data structure for interruptible hash verification

gilles-peskine-arm · 2022-12-04T23:26:00Z

include/psa/crypto.h

+ *                              code; or to free the relevant context if the
+ *                              operation is to be aborted.
+ *
+ * \note                        The interpretation of this maximum number is


[E] What maximum number? Notes can be read out of order.

More generally this paragraph could use a rewrite, keeping the (lack of) context in mind.

gilles-peskine-arm · 2022-12-04T23:26:30Z

include/psa/crypto.h

+ * \note                        The interpretation of this maximum number is
+ *                              obviously also implementation defined. On a
+ *                              hard real time system, this can indicate a hard
+ *                              deadline, which is good, as a real-time system


[E] This reads a bit too judgmental for a specification.

gilles-peskine-arm · 2022-12-04T23:26:50Z

include/psa/crypto.h

+ *                              operation is to be aborted.
+ *
+ * \note                        The interpretation of this maximum number is
+ *                              obviously also implementation defined. On a


[E] Avoid “obviously”: if it was obvious, it wouldn't need stating explicitly.

gilles-peskine-arm · 2022-12-04T23:58:04Z

include/psa/crypto.h

+ *         operation.
+ *
+ * \retval #PSA_ERROR_NOT_SUPPORTED
+ * \retval #PSA_ERROR_INVALID_ARGUMENT


[T] Also NOT_PERMITTED.

gilles-peskine-arm · 2022-12-04T23:59:09Z

include/psa/crypto.h

+ *         please call this function again with the same operation object.
+ *
+ * \retval #PSA_ERROR_INVALID_HANDLE
+ * \retval #PSA_ERROR_NOT_PERMITTED


[T] No, NOT_PERMITTED would be detected at the start.

gilles-peskine-arm · 2022-12-06T17:44:17Z

include/psa/crypto.h

+ *                              returned by the function performing the
+ *                              computation. It is then the caller's
+ *                              responsibility to either call again with the
+ *                              same parameters until it returns 0 or an error


[T] Same operation context, not same parameters. The other parameters can be different. For example, the output of psa_sign_hash_complete can be a stack variable in an intermediate function which is a different pointer at each call.

gilles-peskine-arm · 2022-12-06T17:47:20Z

include/psa/crypto.h

+ *                              interface stability promises, and will only
+ *                              leave beta after implementation.


[T] Remove “will only leave beta after implementation”. It's not like the interface really exists until implementation, and it will stay beta for some time after implementation. It'll leave beta when we say it's out of beta and no sooner.

gilles-peskine-arm · 2022-12-06T17:52:19Z

include/psa/crypto.h

+ *         Operation was interrupted due to the setting of \c
+ *         psa_interruptible_set_max_ops(), there is still work to be done,
+ *         please call this function again with the same operation object.


[T]

Suggested change

* Operation was interrupted due to the setting of \c

* psa_interruptible_set_max_ops(), there is still work to be done,

* please call this function again with the same operation object.

* The operation was interrupted due to the setting of

* \c psa_interruptible_set_max_ops(). There is still work to be done.

* Call this function again with the same operation object.

gilles-peskine-arm · 2022-12-06T18:08:32Z

I'm marking this as approved for design. We'll still make minor changes to the API, but the fundamentals are good.

mpg · 2022-12-12T09:37:43Z

General note about this PR: it's in the Mbed TLS repo but is written as a specification that may have multiple implementations. At some point while implementing this, we'll need to replace all places that says "XXX is implementation defined" with a description of what our implementation actually does.

mpg · 2022-12-12T10:07:27Z

I'm sorry for having such thought that late in the process, but I've just been thinking about the naming for start/complete functions: should rename start to setup for consistency with multi-part operations? setup() is usually the function that makes the operation "active" meaning from this point you need to either bring it to a successful completion or abort() it.

If this is similar enough to existing multi-part operations, the similarity should be reflected in the names chosen; just as well, if this is different enough, this should be reflected in the names. Note: currently we have similar names for the type xxx_operation_t and the function xxx_abort() but not for other functions (start() vs setup(), perhaps even complete() vs finish() though arguably that pair is more different and varies more across multi-part operations).

Also, I don't think start() actually starts computing anything, it just prepares calling complete(), so semantically that would be an argument for calling it setup() as well.

Also, since Gilles mentions that we're likely to want to have multi-part interruptible sign/verify at some point, can we quickly check how the naming scheme would work for that? [Edit: should we have interruptible_setup() to distinguish from non-interruptible multi-part?]

Btw, I've also been wondering if complete() should be called something like compute() (or perhaps progress())? Considering most of the time it will not complete the operation. (OTOH, there's logic in the fact that it only returns success when it did complete the operation, so that's consistent with the name indeed.)

mpg

Looks good to me, except perhaps the naming question, which again I'm sorry for not raising earlier. Other than that, I don't have much to add to what Gilles already pointed out (which which I agree, even if I didn't systematically put a 👍 on it).

mpg · 2022-12-12T09:34:30Z

include/psa/crypto.h

+ *                              single call is implementation defined.
+ *
+ * \warning                     With implementations that interpret this number
+ *                              as a hard limit, setting this number too small


Note: the implementation in Mbed TLS is based on a soft limit: we first do one "uninterruptible block" (which can be a rather large number of basic ops, possibly larger than the limit), and only then check if we have budget for the next block. This was done mostly because with such a soft limit approach, progress is guaranteed.

I agree that hard limit implementations would benefit from the kind of thing you suggest, but since we're a soft limit implementation, we won't really be able to test it.

include/psa/crypto.h

include/psa/crypto_values.h

gilles-peskine-arm · 2022-12-13T10:02:18Z

Btw, I've also been wondering if complete() should be called something like compute() (or perhaps progress())? Considering most of the time it will not complete the operation. (OTOH, there's logic in the fact that it only returns success when it did complete the operation, so that's consistent with the name indeed.)

That was the logic: xxx_complete() is named based on the behavior when it succeeds.

start() vs setup(), perhaps even complete() vs finish() though arguably that pair is more different and varies more across multi-part operation

complete() is pretty different from finish(), and I think this should be reflected in the naming. start() is less different from setup(), but I think it's clearer to have different names for start/complete and setup/update/finish or setup/update/verify.

paul-elliott-arm · 2022-12-18T17:56:22Z

All of the discussed changes that I saw as decided upon I have made in the implementation PR (#6737). Will revisit the ongoing discussions in the new year, and update this version if there is more review to be done here.

gilles-peskine-arm · 2023-01-25T21:32:59Z

include/psa/crypto.h

+ *
+ * In particular, calling \c psa_sign_hash_abort() after the operation has
+ * already been terminated by a call to \c psa_sign_hash_abort() or
+ * \c psa_sign_hash_complete() is safe and has no effect.


[T] That's actually not quite true: the abort function resets the value queried by get_num_ops to 0. Also applies to verify.

athoelke · 2023-01-25T21:49:31Z

Should we have an implementation note somewhere in here that ops should be key-value and data-value invariant, to avoid it becoming a side-channel?

gilles-peskine-arm · 2023-01-25T21:56:36Z

include/psa/crypto_values.h

+ *  the maximum number of ops allowed to be executed by an interruptible
+ *  function in a single call.
+ */
+#define PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED INT32_MAX


[t]

Suggested change

#define PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED INT32_MAX

#define PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED UINT32_MAX

mpg · 2023-02-16T12:03:29Z

I assume we no longer intend to merge this one, now that we have #6737.

gilles-peskine-arm · 2023-02-16T18:18:35Z

We don't, but I still have to review the differences between this and #6737. I'm happy that the API #6737 is good enough to release as a beta, but I want to check for differences and re-read some of the discussions with the benefit of having seen the implementation.

This doesn't require keeping the PR open.

daverodgman · 2023-05-12T13:45:04Z

@paul-elliott-arm can this be closed?

paul-elliott-arm · 2023-05-12T13:47:03Z

Closing this for now as I think its reached its useful limit. Conversations about renaming functions etc can be done in new issues / PRs

paul-elliott-arm self-assigned this Sep 13, 2022

paul-elliott-arm force-pushed the psa_sign_hash_interruptable branch 2 times, most recently from e3295ff to c5ac7f5 Compare September 13, 2022 17:07

daverodgman reviewed Sep 14, 2022

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

daverodgman reviewed Sep 14, 2022

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

daverodgman reviewed Sep 14, 2022

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

daverodgman reviewed Sep 14, 2022

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

paul-elliott-arm force-pushed the psa_sign_hash_interruptable branch from c5ac7f5 to dae46e1 Compare September 14, 2022 17:16

paul-elliott-arm requested review from mpg and gilles-peskine-arm September 14, 2022 17:35

paul-elliott-arm force-pushed the psa_sign_hash_interruptable branch from dae46e1 to 6b934da Compare September 14, 2022 21:59

tom-cosgrove-arm reviewed Sep 15, 2022

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

tom-cosgrove-arm reviewed Sep 15, 2022

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

paul-elliott-arm changed the title ~~PSA Sign Hash (Interruptable) Design~~ PSA Sign Hash (Interruptible) Design Sep 15, 2022

paul-elliott-arm force-pushed the psa_sign_hash_interruptable branch from 6b934da to 33de902 Compare September 15, 2022 15:51

daverodgman requested changes Sep 15, 2022

View reviewed changes

paul-elliott-arm force-pushed the psa_sign_hash_interruptable branch from 33de902 to 2a5e792 Compare September 15, 2022 19:20

daverodgman removed the needs-reviewer This PR needs someone to pick it up for review label Sep 16, 2022

daverodgman reviewed Sep 16, 2022

View reviewed changes

include/psa/crypto_extra.h Outdated Show resolved Hide resolved

paul-elliott-arm force-pushed the psa_sign_hash_interruptable branch from 2a5e792 to e467aca Compare September 16, 2022 17:01

First draft of PSA interruptible ECC signing design

0cb0972

Signed-off-by: Paul Elliott <[email protected]>

paul-elliott-arm force-pushed the psa_sign_hash_interruptable branch from 84acf6c to 0cb0972 Compare October 26, 2022 16:31

paul-elliott-arm requested a review from gilles-peskine-arm November 7, 2022 15:34

gilles-peskine-arm added the needs-review Every commit must be reviewed by at least two team members, label Nov 7, 2022

athoelke mentioned this pull request Dec 5, 2022

Interruptible/bounded-latency asymmetric operations ARM-software/psa-api#23

Open

gilles-peskine-arm requested changes Dec 6, 2022

View reviewed changes

gilles-peskine-arm added needs-work and removed needs-design-approval labels Dec 6, 2022

gilles-peskine-arm mentioned this pull request Dec 7, 2022

Implement PSA interruptible sign/verify hash #6737

Closed

3 tasks

mpg reviewed Dec 12, 2022

View reviewed changes

gilles-peskine-arm reviewed Jan 25, 2023

View reviewed changes

daverodgman added the historical-reviewing Currently reviewing (for legacy PR/issues) label May 12, 2023

paul-elliott-arm closed this May 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSA Sign Hash (Interruptible) Design #6279

PSA Sign Hash (Interruptible) Design #6279

paul-elliott-arm commented Sep 13, 2022 •

edited by gilles-peskine-arm

Loading

paul-elliott-arm commented Sep 15, 2022

gilles-peskine-arm commented Sep 15, 2022

gilles-peskine-arm left a comment •

edited

Loading

gilles-peskine-arm Dec 4, 2022

gilles-peskine-arm Dec 4, 2022

gilles-peskine-arm Dec 4, 2022

gilles-peskine-arm Dec 4, 2022

gilles-peskine-arm Dec 4, 2022

gilles-peskine-arm Dec 4, 2022

gilles-peskine-arm Dec 4, 2022

gilles-peskine-arm Dec 6, 2022

gilles-peskine-arm Dec 6, 2022

gilles-peskine-arm Dec 6, 2022

gilles-peskine-arm commented Dec 6, 2022

mpg commented Dec 12, 2022

mpg commented Dec 12, 2022 •

edited

Loading

mpg left a comment

mpg Dec 12, 2022

gilles-peskine-arm commented Dec 13, 2022

paul-elliott-arm commented Dec 18, 2022

gilles-peskine-arm Jan 25, 2023

athoelke commented Jan 25, 2023

gilles-peskine-arm Jan 25, 2023

mpg commented Feb 16, 2023

gilles-peskine-arm commented Feb 16, 2023 •

edited

Loading

daverodgman commented May 12, 2023

paul-elliott-arm commented May 12, 2023

	/** The type of the state data structure for interruptible hash signing
	/** The type of the state data structure for interruptible hash verification

		* interface stability promises, and will only
		* leave beta after implementation.

	#define PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED INT32_MAX
	#define PSA_INTERRUPTIBLE_MAX_OPS_UNLIMITED UINT32_MAX

PSA Sign Hash (Interruptible) Design #6279

PSA Sign Hash (Interruptible) Design #6279

Conversation

paul-elliott-arm commented Sep 13, 2022 • edited by gilles-peskine-arm Loading

Description

Status

Requires Backporting

Migrations

Todos

paul-elliott-arm commented Sep 15, 2022

gilles-peskine-arm commented Sep 15, 2022

gilles-peskine-arm left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Dec 6, 2022

mpg commented Dec 12, 2022

mpg commented Dec 12, 2022 • edited Loading

mpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilles-peskine-arm commented Dec 13, 2022

paul-elliott-arm commented Dec 18, 2022

Choose a reason for hiding this comment

athoelke commented Jan 25, 2023

Choose a reason for hiding this comment

mpg commented Feb 16, 2023

gilles-peskine-arm commented Feb 16, 2023 • edited Loading

daverodgman commented May 12, 2023

paul-elliott-arm commented May 12, 2023

paul-elliott-arm commented Sep 13, 2022 •

edited by gilles-peskine-arm

Loading

gilles-peskine-arm left a comment •

edited

Loading

mpg commented Dec 12, 2022 •

edited

Loading

gilles-peskine-arm commented Feb 16, 2023 •

edited

Loading