Some parts were already implicitly using this as the two ifdefs were nested,
and some others didn't, which resulted in compile errors in some configs. This
fixes those errors and saves a bit of code+RAM that was previously wasted when
ECP_RESTARTABLE was defined but ECDSA_C wasn't
Previously we kept the ecdsa context created by the PK layer for ECDSA
operations on ECKEY in the ecdsa_restart_ctx structure, which was wrong, and
caused by the fact that we didn't have a proper handling of restart
sub-contexts in the PK layer.
The fact that you needed to pass a pointer to mbedtls_ecdsa_restart_ctx (or
that you needed to know the key type of the PK context) was a breach of
abstraction.
Change the API (and callers) now, and the implementation will be changed in
the next commit.
Goals include:
- reducing the number of local variables in the main function (so that we
don't have to worry about saving/restoring them)
- reducing the number exit points in the main function, making it easier to
update ssl->state only right before we return
- more consistent naming with ecrs prefix for everything
- always check it enabled before touching the rest
- rm duplicated code in parse_server_hello()
For selection of test cases, see comments added in the commit.
It makes the most sense to test with chains using ECC only, so for the chain
of length 2 we use server10 -> int-ca3 -> int-ca2 and trust int-ca2 directly.
Note: server10.crt was created by copying server10_int3_int-ca2.crt and
manually truncating it to remove the intermediates. That base can now be used
to create derived certs (without or with a chain) in a programmatic way.
This is mainly for the benefit of SSL modules, which only supports restart in
a limited number of cases. In the other cases (ECDHE_PSK) it would currently
return ERR_ECP_IN_PROGRESS and the user would thus call ssl_handshake() again,
but the SSL code wouldn't handle state properly and things would go wrong in
possibly unexpected ways. This is undesirable, so it should be possible for
the SSL module to choose if ECDHE should behave the old or the new way.
Not that it also brings ECDHE more in line with the other modules which
already have that choice available (by passing a NULL or valid restart
context).
For RSA, we could either have the function return an error code like
NOT_IMPLEMENTED or just run while disregarding ecp_max_ops. IMO the second
option makes more sense, as otherwise the caller would need to check whether
the key is EC or RSA before deciding to call either sign() or
sign_restartable(), and having to do this kind of check feels contrary to the
goal of the PK layer.
Two different changes:
- the first one will allow us to store k in the restart context while
restarting the following ecp_mul() operation
- the second one is an simplification, unrelated to restartability, made
possible by the fact that ecp_gen_privkey() is now public
(Unrelated to restartable work, just noticed while staring at the code.)
Checking at the end is inefficient as we might give up when we just generated
a valid signature or key.
Otherwise code that uses these functions in other modules will have to do:
#if defined(MBEDTLS_ECP_RESTARTABLE)
ret = do_stuff( there, may, be, many, args );
#else
ret = do_stuff( their, may, be, namy, args, rs_ctx );
#fi
and there is a risk that the arg list will differ when code is updated, and
this might not be caught immediately by tests because this depends on a
config.h compile-time option which are harder to test.
Always declaring the restartable variants of the API functions avoids this
problem; the cost in ROM size should be negligible.
This will be useful for restartable ECDH and ECDSA. Currently they call
mbedtls_ecp_gen_keypair(); one could make that one restartable, but that means
adding its own sub-context, while ECDH and ECDSA (will) have their own
contexts already, so switching to this saves one extra context.
This should only be done in the top-level function.
Also, we need to know if we indeed are the top-level function or not: for
example, when mbedtls_ecp_muladd() calls mbedtls_ecp_mul(), the later should
not reset ops_done. This is handled by the "depth" parameter in the restart
context.
When a restartable function calls another restartable function, the current
ops_count needs to be shared to avoid either doing too many operations or
returning IN_PROGRESS uselessly. So it needs to be in the top-level context
rather than a specific sub-context.
