Hi Tero, thank you for the review. Please see inline.
> Here are some my review comments while reading > draft-ietf-ipsecme-add-ike: > > ---------------------------------------------------------------------- > The text in section 3.1 should say that if length is 0, then no > Service Priority, Num Addresses etc fields are present. > > I.e., add text in first bullet under Length saying that if length is > zero, then later fields are not present. Makes sense. > -- > > Also the text in Num Addresses indicate that it would be valid to send > CFG_REQUEST with proposed Service Priority, but having Num Addresses > set to zero? > > Is this intended? I.e., is the client allowed to request data, but not > propose IP address? If so, perhaps add sentence to Num Addresses > explaining that it can be 0 for CFG_REQUEST when no specific address > is request, but other parameters are requested. Hm... I think my co-authors can comment on this. > -- > > In IP Address(es) explictly mention that it is field contain 4 octet > IPv4 addresses, or 16 octet IPv6 address without any delimeters etc. > This can be derived from the calculation of the length field, but I > think it should be mentioned here, even when it is obvious. OK. > -- > > In section 3.2 it is not clear what the length of the Hash Algorithm > Identifiers fields is. It contains list of hash algorithms or one hash > algorithm if this is response, but it is not clear what is response. What was meant is that a list of hashes is sent by a client (in CFG_REQUEST) and a single hash is sent by a server (in CFG_REPLY). > We have CFG_REQUEST, CFG_REPLY, CFG_SET, and CFG_ACK. Most likely > CFG_REPLY and CFG_ACK are sent in IKE exchange response. On the other > hand CFG_SET is usually used to set the parameters, thus the > Certificate Digest would be required there. True, but IKEv2 doesn't currently use CFG_SET/CFG_ACK and it explicitly allows implementations to ignore them. > I would assume that there is only one Hash Algorithm Identifier for > CFG_REPLY and CFG_SET, and then the Certificate Digest field is > present. For CFG_REQUEST the Hash Algorithm Identifier is a list of > two octet hash algorithm identifiers and the Certificate field is > omitted. For the CFG_ACK only first 4 octets are included and Length > is set to zero. > > I think it would be better to split the Figure 2 to three different > figures: > > > 1 2 3 > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > +-+-----------------------------+-------------------------------+ > |R| Attribute Type | Length | > +-+-----------------------------+---------------+---------------+ > | RESERVED | ADN Length | > +-----------------------------------------------+---------------+ > ~ Authentication Domain Name ~ > +---------------------------------------------------------------+ > ~ Hash Algorithm Identifier (2 octets) ~ > +---------------------------------------------------------------+ > ~ Certificate Digest ~ > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > Figure 2: ENCDNS_DIGEST_INFO Attribute Format for CFG_REPLY and CFG_SET > > > > 1 2 3 > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > +-+-----------------------------+-------------------------------+ > |R| Attribute Type | Length | > +-+-----------------------------+---------------+---------------+ > | RESERVED | ADN Length | > +-----------------------------------------------+---------------+ > ~ Authentication Domain Name ~ > +---------------------------------------------------------------+ > ~ List of Hash Algorithm Identifiers ~ > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > Figure 3: ENCDNS_DIGEST_INFO Attribute Format for CFG_REQUEST > > 1 2 3 > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > +-+-----------------------------+-------------------------------+ > |R| Attribute Type | Length | > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > Figure 4: ENCDNS_DIGEST_INFO Attribute Format for CFG_ACK. > > and then explain the Hash Algorithm Identifier and List of Hash > Algorithm Identifiers separately. We may do this for completeness, but as I've already mentioned CFG_SET/CFG_ACK are not currently used in IKEv2. So I'm in not sure if this is really needed and won't further confuse implementers... > Actually is there any point of having ADN Length and Authenticated > Domain Name in CFG_REQUESTS ever? Why would someone calculate hashes > with certain domain names with different hash algorithms? Perhaps we > should define the format for CFG_REQUEST as follows: > > > 1 2 3 > 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 > +-+-----------------------------+-------------------------------+ > |R| Attribute Type | Length | > +-------------------------------+-------------------------------+ > ~ List of Hash Algorithm Identifiers ~ > +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ > > Figure 3: ENCDNS_DIGEST_INFO Attribute Format for CFG_REQUEST I'm confused, since CFG_REQUEST doesn't include Digest. Am I missing your point? > -- > > In section 4 there is text: > > If the CFG_REPLY includes an ENCDNS_DIGEST_INFO attribute, the DNS > client has to create a digest of the DNS resolver certificate > received in the TLS handshake using the negotiated hash algorithm in > the ENCDNS_DIGEST_INFO attribute. > > But this does not specify how the digest of the DNS resolver > certificate is calculated. There are multiple ways of doing this (only > Subject Public Key Info element like RFC7296 or SPKI(1) selector field > of RFC7671, or full certificate like Cert(0) selector field of > RFC7671). > > I would prefer the SPKI (Subject Public Key Info) selector field way > of RFC7671, as then it does not matter if the certificate is renewed > etc. Does certificate renewal matter in this case? In my reading "digest of the certificate" means the digest over whole certificate (Cert(0)), but I'd rather hear from my co-authors. > -- > > I do not think the [Hash] is normative reference. I did not need to > read and understand that to somewhat understand this document :-) Well, it's only 58 words including title, you may read them in few seconds :-) Kidding aside, how this can be informative? The document uses these codepoints. Or did you mean [I-D.ietf-dnsop-svcb-https]? > -- > > IANA-IKE is bit misleading reference name when you are actually > refering to the IKEv2 Configuration Payload Attribute Types. Perhaps > using IANA-IKE-HASH for the [Hash] above, and IANA-IKE-CFG for this. OK. > -- > > It would actually be useful to have example configuration for some of > the deployment scenarios in Appendix A, similar than in 3.15.2 of > RFC7296. > > i.e., > > CP(CFG_REQUEST) = > INTERNAL_IP6_ADDRESS() > INTERNAL_IP6_DNS() > ENCDNS_IP6() > ENCDNS_DIGEST_INFO(0, "", (SHA2-256, SHA2-384, SHA2-512)) > > and getting reply > > CP(CFG_REPLY) = > INTERNAL_IP6_ADDRESS(2001:DB8:0:1:2:3:4:5/64) > ENCDNS_IP6(23, 1, 16, > (2001:DB8:99:88:77:66:55:44), > "doh1.example.com", > "???") > ENCDNS_DIGEST_INFO(0, "", SHA2-256, > 00112233445566778899aabbccddeeff) > > where the data in ECNDNS_IP6 is in the same order they are in the > actual packet, i.e., 23 is the Service Priority, 1 is num address, 16 > is ADN Length and then is list of IPv6 addresses and so on. > > I for example have no idea what could be in the Service Parameters > field for some specific service (for example DoH), and how it would be > different for DoT... Adding an example makes sense, IMHO. Regards, Valery. > -- > kivi...@iki.fi > > _______________________________________________ > IPsec mailing list > IPsec@ietf.org > https://www.ietf.org/mailman/listinfo/ipsec _______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec
