Labeled IPsec Traffic Selector Support for the Internet Key Exchange Protocol Version 2 (IKEv2)
draft-ietf-ipsecme-labeled-ipsec-12

# John Scudder, RTG AD, comments for draft-ietf-ipsecme-labeled-ipsec-11
CC @jgscudder

Thanks for this document. I have just one comment.

## COMMENTS

I agree with Rob Wilton's point (4), about specifying things only once. The place I bumped into this was that Section 2.2 says,

   The TS_SECLABEL Traffic Selector Type MUST NOT be the only TS_TYPE
   present in the TS payload.  If a TS payload is received with only
   TS_SECLABEL Traffic Selector types, an Error Notify message
   containing TS_UNACCEPTABLE MUST be returned.

whereas Section 3 says,

   TS_SECLABEL MUST NOT be used without another TS_TYPE in a Traffic
   Selector Payload, as it does not specify a complete set of traffic
   selectors on its own.  TS_SECLABEL is complimentary to another type
   of Traffic Selector.  There MUST be an IP address Traffic Selector
   type in addtion to the TS_SECLABEL Traffic Selector type in the
   Traffic Selector Payload.

The re-specification in Section 3 seems potentially problematic in that it introduces a restriction not in the Section 2.2 text. That is, 2.2 says only that TS_SECLABEL can't appear naked, whereas 3 says that it MUST be accompanied by an IP address Traffic Selector type. Possibly this could be viewed as an irrelevant difference, but (a) might there be other selector types introduced in the future, that would sufficiently contextualize TS_SECLABEL without the presence of an IP address selector, and (b) TS_FC_ADDR_RANGE is a thing?

Please consider specifying this only once. My impulse would be to remove the quoted paragraph from Section 3 but what do I know? 

### NITS

In the second quote above, s/addtion/addition/

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

Thanks for working on this specification.

Thank you for the work put into this document. It is easy to read and can be useful is specific use cases.

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education).

Special thanks to Tero Kivinen for the shepherd's detailed write-up including the WG consensus and the justification of the intended status. 

I hope that this review helps to improve the document,

Regards,

-éric


## SPD interaction

While this I-D is about IKEv2, there is little mention of interaction with IPsec SPD beside "pass the TS to the SPD". I wonder whether there must be more than a simple "pass to the SPD" operation on the responder side as the TS is opaque to IKE, so it is SPD/IPSEC that must decide whether to accept this TS, i.e., it is more than a unilateral "pass" operation.

## Section 2.2

`an Error Notify message containing TS_UNACCEPTABLE MUST be returned.` should the obvious be stated, i.e., the process stop and the proposal is not accepted ?

## No IPv6 examples in 2023 ?

Examples are always interesting and useful, but why not using IPv6 ?

# GEN AD review of draft-ietf-ipsecme-labeled-ipsec-11

CC @larseggert

Thanks to Maria Ines Robles for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/gTc6yk7Q4jKh4sNNEQREidgJa70).

## Comments

### Inclusive language

Found terminology that should be reviewed for inclusivity; see
https://www.rfc-editor.org/part2/#inclusive_language for background and more
guidance:

 * Term `traditionally`; alternatives might be `classic`, `classical`,
   `common`, `conventional`, `customary`, `fixed`, `habitual`, `historic`,
   `long-established`, `popular`, `prescribed`, `regular`, `rooted`,
   `time-honored`, `universal`, `widely used`, `widespread`

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Typos

#### Section 3, paragraph 1
```
-    type in addtion to the TS_SECLABEL Traffic Selector type in the
+    type in addition to the TS_SECLABEL Traffic Selector type in the
+               +
```

#### Section 3, paragraph 2
```
-    specifed for each of those TS_TYPE, such as narrowing them following
+    specified for each of those TS_TYPE, such as narrowing them following
+          +
```

### Grammar/style

#### "Table of Contents", paragraph 1
```
curity label. A security label is comprised of a set of security attributes. 
                               ^^^^^^^^^^^^^^^
```
Did you mean "comprises" or "consists of" or "is composed of"?

#### Section 3, paragraph 2
```
loads that include the TS_SECLABEL, than the Child SA MUST be created includ
                                    ^^^^
```
Did you mean "then"?

#### Section 3.1, paragraph 4
```
different specific Security Labels, than these should be negotiated in two d
                                    ^^^^
```
Did you mean "then"?

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Hi,

Thanks for this document.  I found it pretty easy to read/follow and only have a few minor comments/suggestions.

Minor level comments:

(1) p 2, sec 1.  Introduction

   Traffic Selector (TS) payloads specify the selection criteria for
   packets that will be forwarded over the newly set up IPsec SA as
   enforced by the Security Policy Database (SPD, see [RFC4301]).

Should "SA" be defined, or is this wellknown?


(2) p 3, sec 1.2.  Traffic Selector clarification

   A Traffic Selector (no acronym) is one selector for traffic of a
   specific Traffic Selector Type (TS_TYPE).  For example a Traffic
   Selector of TS_TYPE TS_IPV4_ADDR_RANGE for UDP traffic in the IP
   network 198.51.100.0/24 covering all ports, is denoted as (17, 0,
   198.51.100.0-198.51.100.255)
   A Traffic Selector payload (TS) is a set of one or more Traffic
   Selectors of the same or different TS_TYPEs.  It typically contains
   one or more of the TS_TYPE of TS_IPV4_ADDR_RANGE and/or
   TS_IPV6_ADDR_RANGE.  For example, the above Traffic Selector by
   itself in a TS payload is denoted as TS((17, 0,
   198.51.100.0-198.51.100.255))

Would be better to give IPv6 examples rather than IPv4 examples (both here and below)?


(3) p 4, sec 2.2.  TS_SECLABEL properties

   A zero length Security Label MUST NOT be used.  If a received TS
   payload contains a TS_TYPE of TS_SECLABEL with a zero length Security
   Label, that specific Traffic Selector MUST be ignored.

Why not just error (i.e., return TS_UNACCEPTABLE) , wouldn't this be the simpler thing to do?


(4) p 4, sec 3.  Traffic Selector negotiation

   TS_SECLABEL MUST NOT be used without another TS_TYPE in a Traffic
   Selector Payload, as it does not specify a complete set of traffic
   selectors on its own.  TS_SECLABEL is complimentary to another type
   of Traffic Selector.  There MUST be an IP address Traffic Selector
   type in addtion to the TS_SECLABEL Traffic Selector type in the
   Traffic Selector Payload.

I note that this text, usign RFC 2119 language, seems to be somewhat repeating the text in 1.3 and 2.2 second paragraph.  Please consider if it would be better to just state this requirement using RFC 2119 language only once.


(5) p 5, sec 3.1.  Example TS negotiation

         TSr = (((0,0,203.0.113.0-203.0.113.255),
                TS_SECLABEL1)

Looks like you might have an extra openning brace.



Nit level comments:

(6) p 6, sec 5.  IANA Considerations

   [Note to RFC Editor (please remove before publication): This value
   has already bee added via Early Allocation.

Missing closing bracket (not that it matters ...)

Thanks,
Rob