Hi Rob,
Thanks for the review.
Please see inline.
Cheers,
Med
Orange Restricted
> -----Message d'origine-----
> De : Robert Wilton via Datatracker <nore...@ietf.org>
> Envoyé : jeudi 27 avril 2023 11:19
> À : The IESG <i...@ietf.org>
> Cc : draft-ietf-ipsecme-add-...@ietf.org; ipsecme-cha...@ietf.org;
> ipsec@ietf.org; kivi...@iki.fi; kivi...@iki.fi
> Objet : Robert Wilton's Discuss on draft-ietf-ipsecme-add-ike-11:
> (with DISCUSS and COMMENT)
>
> Robert Wilton has entered the following ballot position for
> draft-ietf-ipsecme-add-ike-11: Discuss
>
> When responding, please keep the subject line intact and reply to
> all email addresses included in the To and CC lines. (Feel free to
> cut this introductory paragraph, however.)
>
> ------------------------------------------------------------------
> ----
> DISCUSS:
> ------------------------------------------------------------------
> ----
>
> Hi,
>
> Thanks for this document.
>
> This should be a trivial discuss to resolve, and only flagging it
> as a discuss
> because I think that it makes the spec unclear (or wrong):
>
> (1) p 4, sec 3.1. ENCDNS_IP* Configuration Payload Attributes
>
> * IP Address(es) (variable) - Includes one or more IP
> addresses that
> can be used to reach the encrypted DNS resolver identified
> by the
> Authentication Domain Name. For ENCDNS_IP4 this field
> contains
> one or more 4-octet IPv4 addresses, and for ENCDNS_IP6 this
> field
> contains one or more 16-octet IPv6 addresses.
>
> Shouldn't this be zero or more IP addresses? Otherwise, the
> example that only
> contains a domain and no IP address appears to be invalid.
>
[Med] That text is correct. The field is present only when there is an IP
address to convey; otherwise the field is skipped. The presence is indicated by
this field:
Num Addresses (1 octet) - Indicates the number of enclosed IPv4 (for
ENCDNS_IP4) or IPv6 (for ENCDNS_IP6) addresses.
>
> ------------------------------------------------------------------
> ----
> COMMENT:
> ------------------------------------------------------------------
> ----
>
> 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.
Med: Not AFAIK.
>
> (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?
>
[Med] Do53 is widely used but without a reference. I prefer to maintain in this
section. Thanks.
> (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.
>
[Med] OK.
> (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.
[Med] We have a sentence in the intro:
Sample use cases are described in Appendix A. The Configuration
Payload Attribute Types defined in this document are not specific to
these deployments, but can also be used in other deployment contexts.
It is out of the scope of this document to provide a comprehensive
list of deployment contexts.
Will see if we can make a change.
>
> (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]).
>
[Med] We removed that sentence as per a comment from Paul.
> (7) p 14, sec Appendix B. Examples
>
> I would suggest putting each of the two examples into its own
> subsection.
>
[Med] OK.
> 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.
>
[Med] OK
> (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')"
>
[Med] OK
> (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'.
>
[Med] OK
> Regards,
> Rob
>
>
_________________________________________________________________________________________________________________________
Ce message et ses pieces jointes peuvent contenir des informations
confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce
message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages
electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou
falsifie. Merci.
This message and its attachments may contain confidential or privileged
information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete
this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been
modified, changed or falsified.
Thank you.
_______________________________________________
IPsec mailing list
IPsec@ietf.org
https://www.ietf.org/mailman/listinfo/ipsec
