Subject: [Lwip] minimal esp

From: Daniel Migault <[email-masked]>
Date: Mon, 26 Jul 2021 12:23:48 -0400

Hi Erik,

Thanks for the review which I believe the draft version 06 addresses them.
Please find the details inline below.

Yours,
Daniel

[abstract] [nit]

* "Some constrains include" -> "Some constraints include"

> done

[S1] [comment]

* Someone will inevitably point out that requirements terminology
  don't belong in an Informational document.

  Suggest removing this and lowercase all the terms throughout the
  document.  I know it doesn't seem ideal, but I don't think it
  really lowers the value of any of the recommendations in the doc.

> I am a bit surprised by such a rule. While I might be missing the
rationale, but it seems to me that informational documents are also
concerned with interoperability and the 2119 language  has been introduced
to make implementation interoperable. At least that was my understanding.
That said, I proceeded as follows:
SHOULD NOT: 2
SHOULD: 1
MUST NOT: 3
MUST: 10
RECOMMENDED: 10
MAY: 3

and committed in case we need to reverse it back ;-)

[S2] [nit]

* "and limited traffic flow confidentiality" seems to be redundant
  with "provide confidentiality" earlier in the sentence.

  (It also raises the question of "in what way is it limited?",
  which just seems like an unnecessary digression.)

> limited traffic flow confidentiality in fact designates some padding and
the feature is often designated as TFC. It is defined in RFC4303 section
2.7 and the section title mentions padding, so I "padding" was missing, and
I also added the acronyme TFC. so the final text is:

 "and limited traffic flow confidentiality (TFC) padding."

I think that addresses the confusion, but we can also point to the specific
section of RFC4303.

[S3] [nit]

* "used internal" -> "used internally"
> done
* ", checks does not fall" -> "and checks it does not fall"
> done

[S3.1] [question]

* "proceed to clean key update"

  What does this mean?  "clean"?

> "clean key" update means that there is a mechanism that enables a peer to
say which key is being used. In other words, there is not a set a keys to
test.  I propose the following change.

OLD:
SPI can typically be used to proceed to clean key update and the SPI value
may be used to indicate which key is being used.
This can typically be implemented by a SPI being encoded with the Security
Association Database (SAD) entry on a subset of bytes (for example 3
bytes), while the remaining byte is left to indicate the rekey index.

NEW:
SPI can typically be used to implement a key update with the SPI indicating
the key is being used.
For example, a SPI might be encoded with the Security Association Database
(SAD) entry on a subset of bytes (for example 3 bytes), while the remaining
byte indicates the rekey index.

* "use of randomly generated SPI" ->
  "use of randomly generated SPIs"
> done

* "leakage or privacy or security related information" ->
  "leakage of privacy or security related information"
> done

[S4] [question]

* When too many packets are dropped (due to "out of window" conditions),
  does this cause the session to be renegotiated?

> SN is checked as the first operation- prior integrity check. If taken
into account, these could easily be used to trigger the rekey. In fact the
consecutive failed authentication is used to trigger a rekey. RFC 4303,
does not mandate any other parameters than consecutive authentication
failure to be considered for the trigger, but does not prevent others to be
considered and it is worth noticing that out of windows SN are auditable
events.  So I think that suggesting to consider out of windows event are a
bit out of scope of the minimal esp implementation. That said, we may add
that when removing the mechanism we should consider that additional
integrity checks might be performed.

I suggest the following text:

OLD:
As a result, some implementations MAY drop a non required anti replay
protection especially when the necessary resource involved overcomes the
benefit of the mechanism.

NEW:
 As a result, some implementations MAY drop a non required anti replay
protection especially when the necessary resource involved overcomes the
benefit of the mechanism. These resources need also to balance that absence
of anti-replay mechanism, may lead to unnecessary integrity check
operations that might be significantly more expensive as well.

  If so, is it important to weight the computational impact of
  reestablishing IPsec crypto state relative to trying to maintain
  better record of the previously seen SNs?

[S4] [nit]

* "drop an non required anti replay protection" ->
  "drop a non-required anti-replay protection"
> done
* "the support ESN is not" -> "the support of ESN is not"
>done

[S5] [nit]

* "that were rely on" -> "that were relying on"
> done
* "rather when" -> "rather than when"
> done

[S6] [nit]

* "generate and receive dummy packet" ->
  "generate and receive dummy packets"
> done
* "fix value" -> "fixed value"
> done ( 2 times)

[S8] [nit]

* "provided as indicative" -> "provided as information"?
> done: provided as informational
* "Constraint devices" -> "Constrained devices"
> done
* "energy associated to it" -> "energy associated with it"
> done

[S10] [nit]

* "associated to the management" -> "associated with the management"
> done

* "This usually include mechanisms to prevent a nonce to repeat
  for example."

  "This usually includes mechanisms to prevent a nonce from repeating,
  for example."
> done

* "in conjunction of" -> "in conjunction with"
> done

* "responsible to negotiate" -> "responsible for negotiating"
-- 
Daniel Migault
Ericsson