Re: [Hipsec] rfc5201-bis-04 review

[Tobias Heer <[e-mail]>]

Hello Ari,

ok, this took a while. Finally I am through. Most of your comments did not require second thoughts. On some I made more detailed comments. Please check if I always got your point or if I misunderstood something.

Also. A BIG thank you for providing alternative text when appropriate. That did speed up addressing the comments quite much. Excellent work!

Am 03.02.2011 um 18:47 schrieb Ari Keranen:

> Hi,
> 
> Here's comments, suggestions, and questions of the rest of the rfc5201-bis-04 document. 
> 
> BTW, don't be scared of the amount of comments; I just tried to do a thorough review and weed out all the remaining errors, ambiguities, and style issues -- even the smaller ones.
> 
Thanks, I appreciate the tough review (even if it keeps me busy for a while).

> 
> 5.1.  Payload Forma
> 
>  | Next Header   | Header Length |0| Packet Type |  VER. | RES.|1|i
> 
> s/  VER. /Version/
> (if fits there, so no need to abbreviate?)
> 
Done.


> 
>  document does not describe processing for Next Header values other
>  than decimal 59, IPPROTO_NONE, the IPv6 'no next header' value.
>  Future documents MAY do so. 
> 
> s/do so/define behavior for also other values/
> 
Agreed.

> 
>  The Header Length field contains the length of the HIP Header and HIP
>  parameters in 8-byte units, excluding the first 8 bytes. 
> 
> s/the length/the combined length/
> 
Ok.
> 
> The HIP Version is four bits.  The current version is 2. 
> 
> s/Version is/Version field is/
> s/current version/version defined in this document/
> 
Ok.

> 
>  For example, in the case
>  that the forthcoming SHIM6 protocol happens to be compatible with
>  this specification
> 
> Since SHIM6 is already an RFC, this text could be updated.
> 
Right. This can be removed/adjusted. 

> 
> 5.1.2.  HIP Controls
> 
>  The HIP Controls section conveys information about the structure of
> 
> s/section/field/
> 
> 
>  The following fields have been defined:
> 
> s/fields/sub-fields/

Ok.

> 
> 
>     This control is set in packets R1 and/or
>     I2.  The peer receiving an anonymous HI may choose to refuse it.
> 
> How about using anonymous HIs with other packets? Say, NOTIFYs or UPDATEs?

It doesn't make much sense because the anonymous HI is only important for end-hosts when they bootstrap a new HIP association.
UPDATEs and NOTIFYs are not sent before BEX is complete. 

For middleboxes, things may be different because after an UPDATE a middlebox may suddenly be on the primary communication path between two HIP hosts that previously established a connection.

However, middleboxes are only addressed to a minimal degree in BEX. The semantics and function of the HIT for middleboxes is undefined so far. If we start adding things for middleboxes here we might have to delve much deeper.

Any opinion?

> 
> 
> 5.1.3.  HIP Fragmentation Support
> 
>  In IPv4 networks, HIP packets may encounter low MTUs along their
>  routed path.  Since HIP does not provide a mechanism to use multiple
>  IP datagrams for a single HIP packet
> 
> HIP over HIP does provide mechanism for this, so I'd change:
> s/HIP does not/basic HIP, as defined in this document, does not/ (or something)

Agreed.
> 
> 
>  HIP-aware NAT
>  devices MUST perform any IPv4 reassembly/fragmentation.
> 
> Despite the warning in the next paragraph, this would potentially make all HIP-aware IPv4 NAT devices vulnerable for fragment-attacks. Could change the MUST into SHOULD? Also, could clarify that reassembly/fragmentation is a MUST/SHOULD for HIP packets, not for any random packet.

It should definitely be a SHOULD. The box may or may not be useful without fragmentation support. BEX is not the document to decide that.

I also agree that only HIP packets are to be considered. However, the distinction may be difficult if the fragments arrive out of order. How will the box figure out if the fragment is a HIP control packet or not if the HIP header is in another fragment?

> 
> 
>  it is strongly recommended that the separate document
>  specifying the certificate usage in the HIP Base Exchange defines the
>  usage of "Hash and URL" formats rather than including certificates in
>  exchanges. 
> 
> This already exists in the CERT draft, so this text could be removed/re-worded. The reference to fragmentation DoS attack is still useful though.
> 
Alternative text:

   Certificate chains can cause the packet to be fragmented and
   fragmentation can open implementations to denial-of-service attacks
   [KAU03].  "Hash and URL" schemes as defined in [I-D.ietf-hip-cert]
   for HIP version 1 may be used to avoid fragmentation and mitigate
   resulting DoS attacks.

The implications and security considerations for certficate chains and hash and url are discussed there. Hence, I see no reason to put a MUST or SHOULD here.

> 
> 5.2.  HIP Parameters
> 
>  The HIP Parameters are used to carry the public key associated with
>  the sender's HIT, together with related security and other
>  information.
> 
> Considering the amount of different things carried in HIP Parameters today, this section could start with a more generic description of the use of the parameters ("and other information" sounds a bit understatement). Can't come up with the right words for now, but would be good to have something that would explain that Parameters are used to carry the security stuff, but are also one of the main ways for extending HIP.

Let me try ;-)

   The HIP parameters carry information that is necessary for
   establishing and maintaining a HIP association.  For example, the
   peer's public keys as well as the signaling for negotiating ciphers
   and payload handling are encapsulated in HIP parameters.  Additional
   information, meaningful for end-hosts or middleboxes, may also be
   included in HIP parameters.  The specification of the HIP parameters
   and their mapping to HIP packets and packet types is flexible to
   allow HIP extensions to define new parameters and new protocol
   behavior.

   In HIP packets, HIP parameters are ordered according to their numeric
   type number and encoded in TLV format.

> 
> 
>  | R1_COUNTER             | 128   | 12       | System Boot Counter   |
> 
> The data description does not seem to match other descriptions of R1_COUNTER.
> 
--> Puzzle generation counter
> 
>  | HIP_CIPHER             | 579   | variable | HIP encryption        |
>  |                        |       |          | algorithm             |
> 
> s/HIP encryption algorithm/List of HIP encryption algorithms/
> 
Ok.

> 
>  | ENCRYPTED              | 641   | variable | Encrypted part of I2  |
>  |                        |       |          | packet                |
> 
> ENCRYPTED could be also in other packets than just I2, right?
> 
Now, yes. Agreed. It used to be there for the HI of the Initiator.
> 
>  | CERT                   | 768   | variable | HI Certificate; used  |
> 
> Also this text regarding certificates should be updated.
> 
Yep. The cert stuff has evaded me so far. It seems I have been focusing on crypto agility too much.

> 
>  | ECHO_RESPONSE_SIGNED   | 961   | variable | Opaque data echoed    |
>  |                        |       |          | back; under signature |
> 
> s/echoed back/echoed back by request/
> 
Do you think this is necessary? To me it is obvious and pronounced in the rest of the text.


> Also "covered by signature", or "signed" could be better than "under signature" (also in ECHO_REQ_SIGNED).
> 
> 
>  |  2048 -  4095 | Parameters related to HIP transport types         |
> 
> s/transport types/transport formats/
> (to be consistent with the later text)
> 
> 
> 5.2.1.  TLV Format
> 
>  The type-field value also describes the order of these
>  fields in the packet, except for type values from 2048 to 4095 which
>  are reserved for new transport forms.
> 
> This is a bit strange exception. I wonder if it would make sense to make this consistent with the other ordered lists, i.e., have one parameter that lists the supported transport formats and then, depending on which format was chosen, the corresponding parameter(s) from this range are used (and others ignored).

I recently sent this issue to the list and unless there are any objections, I'll add the following.

I won't be able to get this done before the next version but it will be the first thing for the next revision.

Option: Give all transforms different type numbers, keep the ordering and express preference in a list.

Example HIP packet excerpt:
+------+ +------+---------+ +------+-----+ +------+-----+ 
|Header| | 2050 | XYZ, ESP| | 2095 | ESP | | 2096 | XYZ | ... 
+------+ +----------------+ +------+-----+ +------+-----+
              ^
              |
List----------+



> 
> Also:
> s/order of these fields/order of these parameters/
> s/transport forms/transport formats/
> 
Done.

> 
>  If the parameter can exist multiple times in the packet, the
>  type value may be the same in consecutive parameters. 
> 
> This could rather be formatted as a normative requirement (and then it would be also implicit for the responder). Something like:
> 
>  If the parameter can exist multiple times in the packet, the
>  parameters with the same type MUST be consecutive in the packet.
> 
Agreed and done.
> 
> 5.2.2.  Defining New Parameters
> 
>      Implementations operating in a mode
>      adhering to this specification MUST disable the sending of new
>      critical parameters.
> 
> This sounds a bit strange. Should that be "MUST allow disabling"?
> 
I think the original meaning was that you have to enable it by hand and it is disabled by default. This makes sense because otherwise standard compliant implementations will be incompatible by default. To make it clearer I changed the sentence to:

               Implementations operating in a mode adhering to this
               specification MUST disable the sending of new critical
               parameters by default. 

> 
> 5.2.3.  R1_COUNTER
> 
>  The sender is supposed to increment this counter
>  periodically.
> 
> s/is supposed to increment/increments/ or "SHOULD increment"
> 
SHOULD is fine here. Done.

> 
> 5.2.6.  DIFFIE_HELLMAN
> 
>  The 160-bit ECP
>  group can be used when lower security is enough (e.g., web surfing)
>  and when the equipment is not powerful enough (e.g., some PDAs).
> 
> Should mention which Group ID the 160-bit ECP is (I'd assume 10, but it's not explicit).
> 
I rephrased the sentence to mention the group name explicitly. This should clarify it.

           A HIP implementation MUST implement Group ID 3. The 160-bit
           SECP160R1 group can be used when lower security is enough ...

> Also, some general recommendation on the order of proposed group IDs would be helpful so that implementations don't end up (unnecessarily) proposing the weakest groups by default.
> 
Hmmm.. I think the words of caution above should avoid that. Otherwise, I could write something like:

In general stronger groups SHOULD be preferred. 

Should we explicit state the order here? Stronger groups are of course more costly and need more space in the packets. So I wouldn't want to propose group 9 as default.

> 
> 5.2.7.  HIP_CIPHER
> 
>    Cipher ID      defines the cipher algorithm to be used for
>                   encrypting parts of the HIP packet
> 
> Is HIP_CIPHER used anywhere else except in the ENCRYPTED parameter? If not, the explanation above could say that explicitly. That is, for example,
> s/parts of the HIP packet/the contents of the ENCRYPTED parameter/
> 
Nope it is nowhere else (at least in this document). However other documents might use it for something else.
But well, these documents will define it more explicitly anyway.

Agreed, changed.


> 
>  NULL-ENCRYPTION is included
>  for testing purposes.  NULL-ENCRYPTION SHOULD NOT be configurable via
>  the UI.
> 
> There may be no UI for the implementation, so could say "by the user" instead of "via the UI".
> 
There is (almost) always a UI, right? Maybe its no GUI, maybe its not even a command line tool... but any interface that allows the user to interact is a UI, right. Changing it to "by the user" implies the existence of some UI as well.

> 
> 5.2.8.  HOST_ID
> 
>   DI-type           type of the following Domain Identifier field
>   Domain Identifier the identifier of the sender
> 
> The actual content of these fields is not clear from the text. Could at least move the DI-types table closer to the figure. Also, "DI Length" could simply say "length (in octets) of the Domain Identifier field"; the FQDN and NAI part just confuses here.
> 
I moved it closer and adjusted the order of the field explanations to match the order of the fields in the parameter.
I hope things are clearer now.

I also changed the DI field length descriptions according to your suggestion.

> 
>  If there is no Domain Identifier, i.e., the DI-type field is zero,
>  the DI Length field is set to zero as well.
> 
> Should there be some normative text on when to have the domain identifier?
It is zero if the HIT does not belong to any domain, I guess. I am not fully aware of the reasons that led to the inclusion of the NAI and FQDN here. From my perspective it is some nice-to-have information that adds some weak hierarchy to the HOST_ID (weak because it is not a input for the actual HI or HIT.

Maybe someone can shed some light? (I will post this as separate mail on the list so that it is not lost in the bulk of the comments and replies here).

> 
> 
> 5.2.10.  DH_GROUP_LIST
> 
>  The information in the
>  DH_GROUP_LIST allows the Responder to select the DH group preferred
>  by itself and the Initiator.
> 
> s/and the Initiator/and supported by the Initiator/
> 
Good catch! I am impressed with the scrutiny you applied. Thanks!

> 
> 5.2.13.  HIP_SIGNATURE
> 
> The signature algorithms are defined in Section 5.2.8.
> 
> The algorithm identifier field side for HOST_ID (Section 5.2.8) is 16 bits whereas here it is 8 bits. Something wrong here? 

Woops. That's still a remnant of 5201 (no bis). It should be changed, of course. It won't bloat the signature to use a 16 bit field. On the other hand, we're limited in the numbers of algorithms anyway so 256 seems kind of okay. Well, I'll go for the 16 bit field in HIP_SIGNATURE if no one produces valid reasons not to do this.

This has to be carefully documented in the changelog - otherwise implementers will struggle with this hidden change quite a lot.

> 
> Could also reference to exact table with the algorithms since section 5.2.8 has 4 different tables.
I added a introductory sentence to the table in 5.2.8. I hope this does the trick.

> 
> 
> Sections 5.2.15 and 5.2.16 (SEQ and ACK) 
> 
> Figures, and their Length descriptions are not consistent although, AFAIK, the format of the parameters is identical.
> 
> 
Woops. There is something fishy here. The figure for SEQ is wrong. SEQ should ONLY contain ONE update ID. Not more. ACK can acknowledge several update IDs, though. I'll change it. This seems like a editing error when the acknowledgment of several updates was added. The editor changed the wrong figure. I am surprised that no one noticed this so far. I'll change it.

The figures are now correct. The description was updated. 


> 5.2.16.  ACK
> 
>  The Length field identifies the number of
>  peer Update IDs that are present in the parameter.
> 
> This is a bit misleading. Could say something like "number of peer Update IDs can be inferred from the length by dividing it by 4"
> 
> 
One could and should. Thanks.


> 5.2.17.  ENCRYPTED
> 
>    Encrypted      The data is encrypted using an encryption algorithm
>      data         as defined in the HIP_CIPHER parameter.
> 
> s/an encryption algorithm as defined in/the encryption algorithm defined by/
> 
> 
Done.

> 5.2.18.  NOTIFICATION
> 
>    Padding        any Padding, if necessary, to make the parameter a
>                   multiple of 8 bytes.
> 
> This is the first parameter to explicitly define padding after the figure (not consistent). Also, base text says that padding MUST be zero, but here it says "anything will do". Would suggest removing this altogether.
> 
> 
Good catch. Thanks.


>  Notification information can be error messages specifying why an SA
> 
> First occurrence of SA. Expand, and maybe add a reference too.
> 
I think this refers to the HIP security association, not IPsec. In that regard, the text is blurry (I'll change that) but does not require a reference.

The new text is:

           Notification information can be error messages specifying
           why an HIP Security Association could not be established.
           It can also be status data that a HIP implementation
           wishes to communicate with a peer process.  The table
           below lists the Notification messages and their
           corresponding values.

> 
> Overall, this section is fairly inconsistent with the use of different ways to refer to "Notify Message Type", some suggestions for change:
> 
>  The table below lists the Notification messages and their
>  corresponding values.
> 
> s/Notification messages/Notify Message Types/
> 
suggestion: The table
           below lists the notification messages and their 
           Notification Message Types.


>  Types in the range 0-16383 are intended for reporting errors and in
> 
> s/Types/Notify Message Types/
> 

Ack.

>  An
>  implementation that receives a NOTIFY packet with a NOTIFICATION
>  error parameter in response to a request packet
> 
> s/NOTIFICATION error parameter/(something)/
> 

An implementation that receives a
           NOTIFY packet with a Notify Message Type that indicates an
           error in response to a request packet ...

is this appropriate for (something)?


>  Notify payloads with status types MUST be ignored if not recognized.
> 
> s/Notify payloads with status types/Notification Data in NOTIFICATION parameters with status Notify Message Types/
> 
Ack. Thanks for providing alternative text. Saves me some cycles :-)


>    NOTIFICATION PARAMETER - ERROR TYPES     Value
> 
> s/NOTIFICATION PARAMETER - ERROR TYPES/Notify Message Types - Errors/
> (or why not in all-caps, if that looks better)
> 
>    NOTIFY MESSAGES - STATUS TYPES           Value
> 
> s/NOTIFY MESSAGES - STATUS TYPES/Notify Message Types - Status/
> 
I guess that's a matter of taste - but I like your suggestion - changed.

> 
>    UNSUPPORTED_CRITICAL_PARAMETER_TYPE        1
> 
>      Sent if the parameter type has the "critical" bit set and the
>      parameter type is not recognized.  Notification Data contains the
>      two-octet parameter type.
> 
> What if there are multiple unsupported critical parameter types? Should send multiple NOTIFICATIONs of this Message Type or would it be better to concatenate the types?

There may be several notify parameters in one HIP packet in that case. I spelled it out explicitly in the introduction: 

           HIP packets MAY contain
           multiple notify parameters if several problems exist or
           several independent pieces of information must be transmitted.



> 
> 
>    INVALID_SYNTAX                             7
> 
>      Indicates that the HIP message received was invalid because some
>      type, length, or value was out of range or because the request
>      was rejected for policy reasons. 
> 
> Rejecting message with "invalid syntax" code due to policy reasons sounds strange. Isn't the type "BLOCKED_BY_POLICY" for that purpose? Could change that policy text to: "[...] or the message was otherwise malformed".
> 
Agreed. Makes sense.


> 5.2.19.  ECHO_REQUEST_SIGNED
> 
>  A HIP
>  packet can contain only one ECHO_REQUEST_SIGNED or
>  ECHO_REQUEST_UNSIGNED parameter.  
> 
> 
> s/can/MUST/ ?
> 
That is complete bull. There MAY be one SIGNED but there MAY be several UNSIGNED.
I'll change it.


> 
> 5.2.20.  ECHO_REQUEST_UNSIGNED
> 
>  HIP packet can contain one or more ECHO_REQUEST_UNSIGNED parameters.
>  It is possible that middleboxes add ECHO_REQUEST_UNSIGNED parameters
>  in HIP packets passing by. 
> 
> This seems to contradict with the previous section (see above).

Yepp. Changed it.

> 
>  The sender has to create the Opaque field
>  so that it can later identify and remove the corresponding
>  ECHO_RESPONSE_UNSIGNED parameter.
> 
> Does this apply also to middleboxes, i.e., is "sender" the sender of the packet or sender of the parameter?
> 
The creator of the parameter. I'll state this more clearly.

           The creator of the
           ECHO_REQUEST_UNSIGNED (end-host or middlebox) has to
           create the Opaque field so that it can later identify and
           remove the corresponding ECHO_RESPONSE_UNSIGNED parameter.

> 
> 5.3.1.  I1 - the HIP Initiator Packet
> 
>    IP ( HIP ( DH_GROUP_LIST ) )
> 
> This is not strictly consistent with the notation definition "X(y)   indicates that y is a parameter of X" (HIP stuff is not really a parameter of IP packet). I'm not entirely sure how to fix this though (or if it even needs to be fixed).
> 
> 

I think it ti rather clear here. if it were IP(HIP) I could imagine that someone might be confused. All other forms of braces are used up anyway ([{}])



>  The Initiator receives the Responder's HIT either from a DNS lookup
>  of the Responder's FQDN, 
> 
> Add here a reference to 5205-bis.
> 
Ack.


> 
> 5.3.2.  R1 - the HIP Responder Packet
> 
>  The Initiator's HIT MUST match the one received in the I1 packet. 
> 
> ...if the R1 is sent due to I1. 
> 
 The Initiator's HIT MUST match the one received in the I1
           packet if the R1 is a response to an I1. 

> 
>  Re-using Diffie-Hellman public keys opens up the potential security
>  risk of more than one Initiator ending up with the same keying
> 
> s/public keys/public values/ ?
> (same in next paragraph)
> 

kk

> 
>  If a future version of this protocol is considered, we strongly
>  recommend that these issues be studied again.
> 
> Has anyone looked into this?
> 
I thought about it for a while but did not come up with any good conclusion.


> 
>  The signature is calculated over the whole HIP envelope
> 
> "HIP envelope" is not defined anywhere (but used many times after this occurrence too). Perhaps add it to the definitions part.
> 
> 
We can substitute it with HIP packet. That is more technical and more precise.


> 5.3.3.  I2 - the Second HIP Initiator Packet
> 
>  The HITs used MUST match the ones used previously.
> 
> This is ambiguous. Should that be "used in R1" or are there also other possibilities?
> 
> 
Nope. Changed.

(Damn... Man, this review is sooo much work. I am not even close to the end. Thanks for doing this in such great detail!)

> 
>  The HIP_CIPHER contains the single encryption transform selected by
>  the Initiator, that it uses to encrypt the ENCRYPTED parameter.
> 
> s/the ENCRYPTED parameter/ENCRYPTED parameters/
> (The ENCRYPTED parameter is optional here, but may be sent later, right?)
> 
Ack.

> 
> 5.3.5.  UPDATE - the HIP Update Packet
> 
>  An UPDATE that does not contain a SEQ parameter is
>  simply an acknowledgment of a previous UPDATE and itself MUST NOT be
>  acknowledged by a separate ACK.

The text above seems shaky in itself. The update should nonetheless contain an ACK because otherwise it is not clear which SEQ it acknowledges. I'll change that.

> 
> What about UPDATE without ACK nor SEQ? Unreliable UPDATE or a protocol error?
> 
I changed the text to:

           The UPDATE packet contains zero or one SEQ parameter.  The
           presence of a SEQ parameter indicates that the receiver
           MUST acknowledge the the UPDATE.  An UPDATE that does not
           contain a SEQ but only an ACK parameter is simply an acknowledgment of a
           previous UPDATE and itself MUST NOT be acknowledged by a
           separate ACK. UPDATE packets without SEQ parameters do not
           require an acknowledgment and do not require processing in
           relative order to other UPDATE packets.

Satisfied?

> 
> 5.3.7.  CLOSE - the HIP Association Closing Packet
> 
>  The receiver peer MUST validate the ECHO_RESPONSE_SIGNED and validate
>  both the HIP_MAC and the signature if it has state for a HIP
>  association,
> 
> The ECHO_RESPONSE_SIGNED is only in the CLOSE_ACK. I assume this is not just a typo, since there's not much to validate in the request, right? Also, could be more explicit about who the state is with.
> 
You are right about this. This paragraph must be moved to close ACK.
Done.


> 5.3.8.  CLOSE_ACK - the HIP Closing Acknowledgment Packet
> 
>  The receiver peer MUST validate both the HMAC and the signature.
> 
> ...and the contents of the ECHO_RESPONSE?
> 
> 
This is related to the comment above. There is text on the ECHO_RESPONSE now.


> 5.4.1.  Invalid Version
> 
>  pointing to the VER./RES. byte in the HIP header.
> 
> If you change the "VER." field (as suggested in the beginning of this mail), remember to change it also here.
> 
Done. Thanks for pointing this out.

> 
> 6.1.  Processing Outgoing Application Data
> 
>  The exact format and method for transferring the data from the source
> 
> s/the data/the user data/
> (this does not apply to HIP packets)
> 
Eagle-eye Ari.

> 
> 6.4.1.  HMAC Calculation
> 
>  6.  Set Checksum and Header Length field in the HIP header to
>      original values.
> 
> This could be just an implementation issue, but unless the signatures and MACs are also restored, the length and checksum are invalid after this step. Is this step even needed?
> 
Well.. this is a hint for the implementors so that they don't deal with a corrupted packet. I guess its fine.

> 
> 6.5.  HIP KEYMAT Generation
> 
> 
>     HIP-gl encryption key for HOST_g's outgoing HIP packets
>     HIP-gl integrity (HMAC) key for HOST_g's outgoing HIP packets
> 
> It's a bit confusing to use the same "key name" twice (same for -lg). I assume "outgoing HIP packets" means the ENCRYPTED parameter? If so, could say it explicitly.

Changed it to make ENCRYPTED more obvious but I am not sure what you mean by "same key name twice"... can you explain?


> 
>     HIP-lg encryption key (currently unused) for HOST_l's outgoing HIP
>     packets
> 
> Why HOST_l's key is not used if either Initiator or Responder may end up being the HOST_l?
> 
I fully agree.

> 
> 6.7.  Processing Incoming I1 Packets
> 
>  2.  If the Responder is in ESTABLISHED state, the Responder MAY
>      respond to this with an R1 packet, prepare to drop existing SAs,
> 
> Should prepare to drop only the SAs one has with the I1's sender?
> 
New text:
             If the Responder is in ESTABLISHED state, the Responder
             MAY respond to this with an R1 packet, prepare to drop
             an existing HIP security association with the peer, and
             stay at ESTABLISHED state.


> 
>      Other than that, selecting the HIT is a
>      local policy matter.
> 
> Shouldn't the Responder use the same as HIT the Initiator used for it? This would otherwise break section 6.8 step 3.
Actually that is stated two sentences before the clause you cited:

             If the received Responder's HIT in the I1 is NULL, the
             Responder selects a HIT with a the same HIT Suite as the
             Initiator's HIT.  If this HIT Suite is not supported by
             the Responder, it SHOULD select a REQUIRED HIT Suite
             from <xref target="hit_suite_list"/>, which is currently
             RSA/DSA/SHA-1. Other than that, selecting the HIT is a
             local policy matter.

The "Other than that, selecting the HIT is a local policy matter" is for the case if several HITs (same suite) are possible.

> 
> 
> 6.8.  Processing Incoming R1 Packets
> 
> 
>  7.   The system MUST check that the DH Group ID in the DH parameter
> 
> s/DH parameter/DIFFIE_HELLMAN parameter/ 
> (twice in this step)
kk.


> 
> 
> 6.9.  Processing Incoming I2 Packets
> 
> 
>  Hellman key, decrypting the Initiator's Host Identity, verifying the
> 
> s/decrypting/possibly decrypting/
> (if it's not within ENCRYPTED parameter; also in step 11)
> 
okay.


> 
>  7.   If the system's state machine is in any other state than R2-
>       SENT, the system SHOULD check that the echoed R1 generation
>       counter in the I2 packet is within the acceptable range.
> 
> ...if the counter is included
> 
Done.

> 
> 6.10.  Processing of Incoming R2 Packets
> 
>  If an R2 packet is received in state I2-SENT, it SHOULD be processed.
> 
> When would it be appropriate to *not* process the R2 in I2-SENT state? This looks like a MUST to me.
> 
To me, too.

> 
>  2.  The system MUST verify that the HITs in use correspond to the
>      HITs that were received in the R1 packet.
> 
> "..that caused the transition to the I1-SENT state" 
> i.e., not just any R1
> 
> 
Details, details, details. But these matter :-) I'll gladly change it.



> 6.14.  Processing CLOSE Packets
> 
>  The HIP association is not discarded before the host moves from the
>  UNASSOCIATED state.
> 
> s/from/to/?

Woops. Okay.
> 
> 
> 6.15.  Processing CLOSE_ACK Packets
> 
>  A host can map CLOSE messages to CLOSE_ACK messages by using
>  the included ECHO_RESPONSE_SIGNED in response to the sent
>  ECHO_REQUEST_SIGNED in the CLOSE_ACK.
> 
> s/using/matching/
> And the end of the sentence seems to be incorrect anyway (REQ is not in ACK). Could rephrase the whole sentence.
> 

New text:

         A host can map CLOSE messages to
         CLOSE_ACK messages by using the included
         ECHO_RESPONSE_SIGNED that was sent in the CLOSE_ACK packet
         in response to the ECHO_REQUEST_SIGNED in the CLOSE packet.

> 
>  The CLOSE_ACK contains the HIP_MAC and the SIGNATURE for
> 
> s/and the SIGNATURE/and SIGNATURE parameters/
> 
Done.

> 
> 7.  HIP Policies
> 
>  Initiators may use a different HI for different Responders to provide
>  basic privacy.  Such implementations SHOULD keep state for mapping
>  Initiator's HIT to Responder's HIT.  This access control list (ACL)
>  SHOULD also include preferred transform and local lifetimes.
> 
> This sounds a bit ambiguous. Why is the mapping needed? What are the lifetimes? Why store the preferred transform?
> 
Okay... The text was blurry and weird. I have some equally blurry but clearer alternative here. Blurry because this is really local policy, not HIP.

       Initiators may use a different HI for different Responders to
       provide basic privacy. Whether such private HITs are used
       repeatedly with the same Responder and how long these HITs are
       used is decided by local policy and depends on the privacy
       requirements of the Initiator.




> 
>  The value of K used in the HIP R1 packet can also vary by policy.  K
>  should never be greater than 20, but for trusted partners it could be
>  as low as 0.
> 
> s/should never/SHOULD NOT/
> Also, why 20? Some rough number on "how much is 20" could be helpful. How about, say, 10 years from now, is 20 that big still then or should we comment something on adjusting the maximum value in the future? Or will it be HIP v3 by then? :)

I would remove the text on 20 altogether. The 20 makes assumptions on the capabilities of the clients and the attacker, which we should not do. The 20 is just an unjustified magic number. The new text says:

       The value of K used in the HIP R1 must be chosen with care.
       Too high numbers of K will exclude clients with weak CPUs
       beause these devices cannot solve the puzzle within reasonable
       time.  K should only be raised if a Responder is under high
       load attack, i.e., it cannot process all incoming HIP
       handshakes any more. If a responder is not under high load, K
       SHOULD be 0.

> 
> 
> 10.  IANA Considerations
> 
>  This document also creates a set of new namespaces.  These are
>  described below.
> 
> Do we create new namespaces for v2 or should we re-use the existing ones?
> 
We create new ones because the parameter ranges and the parameter definitions change. So I guess no textual change is needed here.


> 
>  HIP Version
> 
> At least this namespace we shouldn't redefine (but e.g., HIP Suite we should).
> 

Agreed. Changed the text.

> 
>  Group ID
> 
>     New values either from the reserved or unassigned
>     space are assigned through IETF Consensus.

> What are these spaces? Is "0" the "reserved space"? Same with CIPHER_ID.
> 
Removed the reserved space. Also changed IETF Consensus to IETF Review.

> 
>     Notify Message Type values 1-10 are used for informing about
>     errors in packet structures, values 11-20 for informing about
>     problems in parameters [...]
> 
> This text could be more helpful in the NOTIFICATION section.
> 

Agreed.

> 
> That's all for now. More nits coming off-lists.
> 
Thank you so much for this detailed review. I am really impressed.

Tobias


> 
> Cheers,
> Ari
> _______________________________________________
> Hipsec mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/hipsec