Internet Key Exchange Protocol Version 2 (IKEv2) Configuration for Encrypted DNS
draft-ietf-ipsecme-add-ike-14

Thanks for the discussion and the resulting changes. It addresses my concerns.

# GEN AD review of draft-ietf-ipsecme-add-ike-11

CC @larseggert

Thanks to Stewart Bryant for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/4W1GbLLH-dUSHMRjawTAsl7Zp38).

## 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

Clearing discuss based on proposed resolution text.


Minor level comments:

(2) p 0, sec 

   This document specifies new Internet Key Exchange Protocol Version 2
   (IKEv2) Configuration Payload Attribute Types to assign DNS resolvers
   that support encrypted DNS protocols, such as DNS-over-HTTPS (DoH),
   DNS-over-TLS (DoT), and DNS-over-QUIC (DoQ).

Are there any updates needed to RFC 9061 needed to cover manageability aspects/updates of the attributes defined in this draft?  Note, I'm not requesting that they be added to this draft, but instead, I want to check if there is any need or plan to address them.


(3) p 2, sec 2.  Terminology

   Do53:  refers to unencrypted DNS.

This term only turns up a few (3 times) in this doc, and its not clear to me that it improves its readability.  I didn't know what it meant, possibly just referencing to "Unencrypted DNS" would be better for the wider audience?


(4) p 3, sec 3.1.  ENCDNS_IP* Configuration Payload Attributes

      -  0 if the Configuration payload has types CFG_REQUEST (if no
         specific DNS resolver is requested) or CFG_ACK.  If the
         'Length' field is set to 0, then later fields shown in Figure 1
         are not present.

I found this text unclear & confusing when combined with the following two paragraphs.  I would suggest rewording the first sentence to something like:

   0, if the Configuration payload has (i) type CFG_REQUEST and no
   specific DNS resolver is requested or (ii) type CFG_ACK.


(5) p 13, sec Appendix A.  Sample Deployment Scenarios

Readability may be slightly improved by adding a sentence here to explain what the purpose of this section is.


(6) p 14, sec Appendix A.  Sample Deployment Scenarios

   Enterprise networks are susceptible to internal and external attacks.
   To minimize that risk all enterprise traffic is encrypted
   (Section 2.1 of [I-D.arkko-farrell-arch-model-t]).

Would "SHOULD be encrypted" be better than "is encrypted"? Or, alternatively, "Encrypting all internal enterprise traffic minimizes the risks of attacks (Section 2.1 of [I-D.arkko-farrell-arch-model-t]).


(7) p 14, sec Appendix B.  Examples

I would suggest putting each of the two examples into its own subsection.



Nit level comments:

(8) p 4, sec 3.1.  ENCDNS_IP* Configuration Payload Attributes

      -  0 if the Configuration payload has types CFG_REQUEST (if no
         specific DNS resolver is requested) or CFG_ACK.  If the
         'Length' field is set to 0, then later fields shown in Figure 1
         are not present.
      -  (4 + Length of the ADN + N * 4 + Length of SvcParams) for
         ENCDNS_IP4 attributes if the Configuration payload has types
         CFG_REQUEST or CFG_REPLY or CFG_SET; N being the number of
         included IPv4 addresses ('Num addresses').

Possibly "(4 + 'Length of the ADN' + (N * 4) + Length of SvcParams)", and similarly for IPv6, would be more explicit.


(9) p 5, sec 3.2.  ENCDNS_DIGEST_INFO Configuration Payload Attribute

   *  Length (2 octets, unsigned integer) - Length of the enclosed data
      in octets.  This field MUST be set to "2 + 2 * number of included
      hash algorithm identifiers".

For clarity, I suggest: "2 + (2 * 'number of included
      hash algorithm identifiers')"


(10) p 6, sec 3.2.  ENCDNS_DIGEST_INFO Configuration Payload Attribute

   *  Length (2 octets, unsigned integer) - Length of the enclosed data
      in octets.  This field MUST be set to "2 + 2 * number of included
      hash algorithm identifiers".
   *  Num Hash Algs (1 octet) - Indicates the number of included hash
      algorithm identifiers.  This field MUST be set to "(Length -
      2)/2".

I suggest, included 'hash algorithm identifiers'.

Regards,
Rob