Re: [Ipsec-tools-devel] [PATCH] MIPv6, PF_KEY: support for MIGRATE and KMADDRESS
Brought to you by:
mit_warlord,
netbsd
From: <ar...@na...> - 2008-10-29 13:26:20
|
Hi, Timo Teräs <tim...@ik...> writes: >>> I prefer the current logic. We use IKE addresses most of time so >>> it's better to have that with short name. >> >> That's what I thought. > > I've been trying to figure out what I would prefer or find to most > understandable, but I haven't had too much success. > > Now I'm also thinking that since the SA addresses never change, we > might want to keep them always in some member (as those are what we > use for DB lookups when talking with kernel, right?). And that's > what we also print to debug logs, so we'd avoid possible ?: > constructs. thas was my previous proposal to reverse things in struct iph2handle, i.e. have sa_src/sa_dst always defined and src/dst there only if IKE addresses do not match SA ones. But there will still be some ?: constructs ... just elsewhere, IMHO. > As far as I understand the SA is always negotiated against HoA. This is true for transport mode, the negotiation being done using the CoA (for IKE packets) for the HoA (SA-endpoint wise). But not for tunnel mode ;-) In that case, SA endpoints are the CoA and the address of the HA. The IKE negotiation is done using the CoA for the transport. > And we need to store the CoA from MIGRATE so we can use that to > negotiate SA:s when HoA is not yet registered. > > So I'm considering again the reversed logic. As packet sending / > receiving is the only place to need CoA, it might make sense to put > the conditional stuff there. And have them src/dst (for sa) and > coa_src/dst for care of addresses. Do you think that would make sense? This is a matter of taste. Both should work equally. I had chosen current version because it looks logical at that time but I have no specific argument. At some point, I also considered (and then dropped) the following ideas: - have some inline helper to access SA or IKE addresses. Having explicit things in the code make it more readable, AFAICT. - always have all 4 attributes set, which would remove the tests and simplify some parts but with additional memory requirements. This is IMHO a bad idea. > Also what comes to the original proxy address stuff, I think it can > be set using remote block "my_identifier address" and sainfo block > local_id field. I guess those mostly come from ACQUIRE message, but > you can also force connection initiation using racoonctl. Maybe that's > a way to set them (didn't check code for that)? I think we should just not bother with support_proxy for now, leave things as is and keep it on a todo list. After spending some time this morning (see below), I would say that: - removing the last support_proxy would remove a feature that may be used by someone. - always sending IDci/IDcr will break things (NAT-T wise) > I guess the support proxy directive is important after all. If we > have NAT enabled we actually want to use the IKE addresses instead > of the identities received when injecting the SA:s to kernel. So > this probably needs further thought too. It does, indeed. Take a quick look (follow natoa, idci/idcr and support_proxy values) at the commented patch at the end of the message. If I completely remove support_proxy and have IDci and IDcr *always* sent, I bet this has a bad impact on NAT-T. IMHO, the code at that location would deserve some comments (and possibly rewriting). Anyway, I have done the following things, which results in updated patches: - removed all exit(1) calls in patch 9, as requested - changed '@' by 'address(es)' in comments - removed funny indentation. Just tell me if some remain. - implemented a callback-based match and apply mechanism for acting on Phase 1. Tell me if this is what you expected. - tested previous changes. Patches numbers have not been modified: 1 smarter_addresses_and_socket_update.patch 2 enhanced_migrate_libipsec.patch 3 enhanced_migrate_struct_transition.patch 4 enhanced_migrate_addnewsp_update.patch 5 enhanced_migrate_pk_recvgetspi_improvements.patch 6 enhanced_migrate_pk_recvupdate_changes.patch 7 enhanced_migrate_pk_recvadd_changes.patch 8 enhanced_migrate_pk_recvdelete_changes.patch 9 enhanced_migrate_helpers.patch 10 enhanced_migrate_core.patch 11 remove_useless_getph2bysaidx_params.patch 12 remove_unused_flushlcconf.patch 13 remove_unused_clear_myaddr.patch 14 remove_MAXNESTEDSA_weirdness.patch 15 add_scopeid_test_in_cmpsaddr.patch Comments welcome. Cheers a+ Below is the patch I initially (it is no more in the repo) wrote to have IDci and IDcr sent all the time and remove last support_proxy use. Index: ipsec-tools/src/racoon/isakmp_quick.c =================================================================== --- ipsec-tools.orig/src/racoon/isakmp_quick.c 2008-10-28 18:22:22.520725782 +0100 +++ ipsec-tools/src/racoon/isakmp_quick.c 2008-10-28 18:27:40.632722067 +0100 @@ -190,7 +190,7 @@ int tlen; int error = ISAKMP_INTERNAL_ERROR; int natoa = ISAKMP_NPTYPE_NONE; - int pfsgroup, idci, idcr; + int pfsgroup; int np; struct ipsecdoi_id_b *id, *id_p; #ifdef ENABLE_NATT @@ -250,26 +250,6 @@ plog(LLV_DEBUG, LOCATION, NULL, "IDcr:\n"); plogdump(LLV_DEBUG, iph2->id_p->v, iph2->id_p->l); - /* - * we do not attach IDci nor IDcr, under the following condition: - * - all proposals are transport mode - * - no MIP6 or proxy - * - id payload suggests to encrypt all the traffic (no specific - * protocol type) - * - SA endpoints and IKE addresses for the nego are the same - * (iph2->src/dst) - */ - id = (struct ipsecdoi_id_b *)iph2->id->v; - id_p = (struct ipsecdoi_id_b *)iph2->id_p->v; - if (id->proto_id == 0 && - id_p->proto_id == 0 && - iph2->ph1->rmconf->support_proxy == 0 && - iph2->sa_src == NULL && iph2->sa_dst == NULL && - ipsecdoi_transportmode(iph2->proposal)) { - idci = idcr = 0; - } else - idci = idcr = 1; - #ifdef ENABLE_NATT /* * RFC3947 5.2. if we propose UDP-Encapsulated-Transport @@ -304,10 +284,9 @@ + sizeof(*gen) + iph2->nonce->l; if (pfsgroup) tlen += (sizeof(*gen) + iph2->dhpub->l); - if (idci) - tlen += sizeof(*gen) + iph2->id->l; - if (idcr) - tlen += sizeof(*gen) + iph2->id_p->l; + tlen += sizeof(*gen) + iph2->id->l; + tlen += sizeof(*gen) + iph2->id_p->l; + #ifdef ENABLE_NATT if (natoa != ISAKMP_NPTYPE_NONE) tlen += 2 * sizeof(*gen) + nat_oai->l + nat_oar->l; @@ -326,27 +305,18 @@ p = set_isakmp_payload(p, iph2->sa, ISAKMP_NPTYPE_NONCE); /* add NONCE payload */ - if (pfsgroup) - np = ISAKMP_NPTYPE_KE; - else if (idci || idcr) - np = ISAKMP_NPTYPE_ID; - else - np = natoa; + np = pfsgroup ? ISAKMP_NPTYPE_KE : ISAKMP_NPTYPE_ID; p = set_isakmp_payload(p, iph2->nonce, np); /* add KE payload if need. */ - np = (idci || idcr) ? ISAKMP_NPTYPE_ID : natoa; if (pfsgroup) - p = set_isakmp_payload(p, iph2->dhpub, np); + p = set_isakmp_payload(p, iph2->dhpub, ISAKMP_NPTYPE_ID); /* IDci */ - np = (idcr) ? ISAKMP_NPTYPE_ID : natoa; - if (idci) - p = set_isakmp_payload(p, iph2->id, np); + p = set_isakmp_payload(p, iph2->id, ISAKMP_NPTYPE_ID); /* IDcr */ - if (idcr) - p = set_isakmp_payload(p, iph2->id_p, natoa); + p = set_isakmp_payload(p, iph2->id_p, natoa); #ifdef ENABLE_NATT /* NAT-OA */ |