If rsm != NULL then the curve type has to be Short Weierstrass, as we don't
implement restartable Montgomery now. If and when we do, then it's better to
check for the subcontext only, and not for the curve type.
Exactly one of three ways will be used, so make that clear by using an
if 1 else if 2 else 3 structure.
While at it, don't initialize variables at declaration, just to make extra
sure they're properly initialized afterwards in all code paths.
As done by previous commits for ECC and ECDSA:
- use explicit state assignments rather than increment
- always place the state update right before the operation label
This will make it easier to add restart support for other operations later if
desired.
SSL-specific changes:
- remove useless states: when the last restartable operation on a message is
complete, ssl->state is incremented already, so we don't need any additional
state update: ecrs_state is only meant to complement ssl->state
- rename remaining states consistently as <message>_<operation>
- move some labels closer to the actual operation when possible (no assignment
to variables used after the label between its previous and current position)
Systematically assign state just before the next operation that may return,
rather that just after the previous one. This makes things more local. (For
example, previously precompute_comb() has to handle a state reset for
mul_comb_core(), a kind of coupling that's best avoided.)
Note that this change doesn't move the location of state updates relative
to any potential return point, which is all that matters.
Incrementing the state is error-prone as we can end up doing it too many times
(loops) or not enough (skipped branches), or just make programming mistakes
(eg. the state was incremented twice at the end, so it ended up with a value
not in the enum...)
This is the first step of the rework, the next one will rationalize where the
state assignments are done.
The call would anyway check for pointer equality and return early, but it
doesn't hurt to save a function call, and also this follows more uniformly the
pattern that those two lines go together:
#if defined(MBEDTLS_ECP_RESTARTBLE)
if( rs_ctx != NULL && ...
Child was almost redundant as it's already saved in ver_chain, except it was
multiplexed to also indicate whether an operation is in progress. This commit
removes it and introduces an explicit state variable instead.
This state can be useful later if we start returning IN_PROGRESS at other
points than find_parent() (for example when checking CRL).
Note that the state goes none -> find_parent and stays there until the context
is free(), as it's only on the first call that nothing was in progress.
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.
