[Swan-dev] state m/c 2of3: State machine cleanups
Andrew Cagney
andrew.cagney at gmail.com
Wed Mar 4 03:56:54 EET 2015
On 3 March 2015 at 16:42, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
> | From: Andrew Cagney <andrew.cagney at gmail.com>
>
> (I didn't write the centralized V2 code driven by the microcode but it
> was kind of copied from what I did in V1.)
>
> | - I earlier posted questions related to ikev2_process_payloads() - it
> | is, to me, doing more than it should.
> | By moving its search logic into the main search-for-state-transition
> | loop things get more transparent, and SMF2_CONTINUE_MATCH (which
> | scares me) can also be deleted.
> | In addition to checking the clear payload, SMF2_UNPACK_SK indicates
> | that the SK (encrypted) payload can be checked - less stuff for my
> | rekey states to deal with
>
> The following is based on my imperfect memory. With no checking.
> Beware.
>
> I hacked in the SMF2_CONTINUE_MATCH code. Why?
>
> - decryption was not done by the central routine: the per-transition
> routines do it.
>
> - in some cases, decryption exposes information that affects microcode
> selection. So a second go at matching was required
> (SMF2_CONTINUE_MATCH).
It sounds like the code in this area has evolved then. Thanks for the
background.
Currently it is only used to differentiate between two I1 transitions -
expected INIT reply and some other INIT reply such as INVALID_KE. Both of
these cases are deal with clear text.
> Some thoughts:
>
> - I think that it would be better to have the centralized routine do
> the decryption before calling the appropriate per-transition
> routine.
I did the change in stages:
- first I split ikev2_process_payload into several parts:
-> code to decode the payload and record the results
-> code to check the results match expected
-> code to log unexpected results
the code looping over SMF2_CONTINUE_MATCH could then be moved into
process_v2_packet so that contained separate loops
- then I merged the two loops
- finally I added stuff to continue to the SK payload if the applicable bit
is set; it just re-used the above (along with a tweaked decrypt payload
method)
> - I think that there are some cases where a bit of transition-specific
> code is needed before the keying material for decryption is
> available. (So it was easier to hack in what I did rather than
> making larger changes.)
Yes. In pluto for IKEv2, the original responder only completes the DH
calculation when the AUTH request arrives (it's a smart trick). In V1 it
is done in the background. This is why I added a flag to mark that things
could be decrypted (and allow smother migration).
> - ikev2_process_payloads gets called twice on an encrypted message:
> the first gobbles just the single E payload; the second gobbles the many
> payloads within the decrypted E payload.
> - I think that it would be clearer if the E payload were special-cased
> and that ikev2_process_payload were only called once
> and that the payload checking only be done once
> and only then would the per-transition function be called.
> But the proof would be if the code were simplified by this.
See above, it looks something like this:
for (svm = v2_state_microcode_table; svm->state != STATE_IKEv2_ROOF;
svm++) {
if (svm->state != from_state)
continue;
if (svm->recv_type != ix)
continue;
/*
* Does the original initiator flag match?
*/
if ((svm->flags & SMF2_IKE_I_SET) && !ike_i)
continue;
if ((svm->flags & SMF2_IKE_I_CLEAR) && ike_i)
continue;
/*
* Does the message reply flag match?
*/
if ((svm->flags & SMF2_MSG_R_SET) && !msg_r)
continue;
if ((svm->flags & SMF2_MSG_R_CLEAR) && msg_r)
continue;
/*
* Since there's a state that, at least, looks like it
* will accept the packet, unpack the clear payload
* and continue matching.
*/
if (clear_payload_summary.status != STF_OK) {
DBG(DBG_CONTROL, DBG_log("Unpacking clear payload
for svm: %s", svm->story));
clear_payload_summary = ikev2_decode_payloads(md,
&md->message_pbs,
md->hdr.isa_np);
if (clear_payload_summary.status != STF_OK) {
complete_v2_state_transition(mdp,
clear_payload_summary.status);
return;
}
}
struct ikev2_payload_errors clear_payload_errors
= ikev2_verify_payloads(clear_payload_summary, svm,
FALSE);
if (clear_payload_errors.status != STF_OK) {
/* Save this failure for later logging. */
clear_payload_status = clear_payload_errors;
continue;
}
/*
* Continue checking by unpacking the SK payload but
* only its ok and there's one to unpack. Otherwise
* assume its a correct match.
*/
if (!(svm->flags & SMF2_UNPACK_SK)) {
break;
}
if (!(svm->req_clear_payloads &
LELEM(PINDEX(ISAKMP_NEXT_v2SK)))) {
break;
}
if (enc_payload_summary.status != STF_OK) {
DBG(DBG_CONTROL,
DBG_log("Unpacking encrypted payload for svm:
%s",
svm->story));
stf_status status =
ikev2_verify_and_decrypt_sk_payload(md);
if (status != STF_OK) {
complete_v2_state_transition(mdp, status);
return;
}
unsigned np =
md->chain[ISAKMP_NEXT_v2SK]->payload.generic.isag_np;
enc_payload_summary = ikev2_decode_payloads(md,
&md->clr_pbs, np);
if (enc_payload_summary.status != STF_OK) {
complete_v2_state_transition(mdp,
enc_payload_summary.status);
return;
}
}
struct ikev2_payload_errors enc_payload_errors
= ikev2_verify_payloads(enc_payload_summary, svm,
TRUE);
if (enc_payload_errors.status != STF_OK) {
/* Save this failure for later logging. */
enc_payload_status = enc_payload_errors;
continue;
}
/* must be the right state machine entry */
break;
}
(inside verify_and_decrypt_payload there's a check that crypto is ready, I
should probably make that explicit here)
SMF2_UNPACK_SK provides a migration path and addresses the original
responder trick.
> My feeling is that a significant amount of code that is replicated in
> several per-transition functions could be moved into a central routine
> and gated by microcode. That was the motivation for the microcode
> construct in V1.
Events, such as timeouts, and even request initiation should get dispatched
by the state machine. Make this state machine behave like a state machine.
> An example of centralizing code is moving the timeout-setting logic out of
> the per-transition functions. I think that Antony has done (most of?
> all of?) this.
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20150303/7226d156/attachment-0001.html>
More information about the Swan-dev
mailing list