Subject: Re: [dnssd] WGLC for draft-ietf-dnssd-srp / review

From: Ted Lemon <[email-masked]>
Date: Sun, 24 Oct 2021 22:53:07 -0400

Thanks for the review!

On Thu, Aug 19, 2021 at 7:04 AM Esko Dijk <[email-masked]>
wrote:

> *** Abstract
>
> 802.11 (Wi-Fi) and 802.15.4 (IoT) networks
>
> -> add 'IEEE' so "IEEE 802.11 (Wi-Fi) and 802.15.4 (IoT) networks"
>
> As an editorial comment, in general the abstract should not contain more
> details than the main text.
>
> So typically one would see back these terms again in an introduction.
>

 I'm generally against bulking up the abstract, so I'd prefer not to add
IEEE twice here. I think the typical IETF document reader knows what the
IEEE 802 working groups are... However, if there is consensus to make this
change, I will.

>
>
> *** 2.2.1
>
> "RFC 6763 describes the details of what each of these types of updates
> contains"
>
> -> not everything contained in these updates is explained in RFC 6763.
> Specifically, the "Key RR" is not in that RFC. Another RFC ref can be added
> here that defines Key RR as definitive source of information.
>

I've added a reference to RFC2539—thanks for noticing this!

> *** 2.2.3.1
>
> "SRP clients are allowed to send updates to the generic domain
> "default.service.arpa""
>
> -> In section 2.1.* it looks as if only constrained hosts can do this. Can
> full-featured hosts also do this? If not we can say "Constrained-Node SRP
> clients are ..."   If yes, then I would expect section 2.1.1 to mention
> this use for the full-featured hosts.
>

I've added a caveat here.

> *** 2.2.5.1
>
> duplicated sentence "This key pair MUST be unique to the device."
>

Oops, thanks!

> *** 2.2.5.4
>
> Does the statement here imply that this draft formally updates RFC 2782?
>
> (Question to the WG. Maybe not: since the update only applies in SRP
> context not for SRV records in general. But maybe yes: RFC 2782 says
> "Unless and until permitted by future standards action, name compression is
> not to be used for this field." which would make it proper to refer to the
> SRP draft as such future action.)
>

No, this is just for SRP servers and clients.

> *** 2.2.5.5.2
>
> "update that deletes the PTR record whose target is the service instance
> name."
>
> -> This looks like a single PTR record. Prior section 2.2.1 mentions that
> the service instance name may be referred to by multiple PTR records.  So
> in case of multiple, it is not required to have multiple instructions to
> delete these multiple PTR records?
>
> (I.e. there's no risk of lingering PTR records that would point to a
> service instance that's deleted?)
>

If you mean for subtypes, see the discussion I had with Abtin recently on
the mailing list. For practical reasons, we do not support deleting of
individual subtypes—it's all or nothing.

> *** 2.3 title
>
> [Nit] -> may give the impression that all SRP server behavior is specified
> in here.
>
> Somewhere in 2.3 / 2.3.* it could mention that more normative behavior is
> specified in other sections too, like Sections 3 and 4.
>

I've changed this to "Validation and Processing of SRP Updates".

> *** 2.3.1
>
> Grammar to be fixed: "Each instruction consists some combination"
>

Yup, thanks!

> *** 2.3.1.2
>
> [Nit] Bullet:
>
>  "*  Service Descriptions Instructions do not modify any other RRs."
>
> -> best rephrased to
>
>  "*  no RR updates other than the ones defined above."
>
>
>
> That would fit better in the style of the previous bullet points in the
> list.
>

I've changed 2.3.1.2 and 2.3.1.3 to say "resource records" for consistency
rather than "RRs" in the first case and "records" in the second case.

>
>
> *** 2.3.1.3
>
> "If a link-scope address or IPv4 autoconfiguration address is provided by
> the SRP client, the
>
>   SRP server MUST treat this as if no address records were received; that
> is, the Host Description is not valid."
>
> -> This seems rather strict. If the client provides 1 routable IP address
> and 1 link-local address, couldn't the SRP server just ignore the
> link-local address?
>
> This might have some benefits for future/backwards compatibility , in case
> of future cases where link-local addresses are to be used for some reason.
> (If only for debug/diagnostic purposes)
>
> I agree that if the client sends only link-local address(es) then the SRP
> server treats it as if no address records were received.
>

The issue is that the SRP server has no way of knowing what the scope of
the link-local address is. So it's by definition an error to send one.

> *** 2.3.2
>
> [Nit] "must have that Host Description instruction as the target of its
> SRV record"
>
> -> could rephrase to "MUST have the hostname of that Host Description
> Instruction as the target of its SRV record"  to be more precise.
>
> Similar comments can be given for the remainder of the section text that
> talks about instructions pointing to instructions.  (It's more like names
> in instructions being equal to names that are used in other instructions)
>
> (not sure if MUST or must is desired here by the way)
>

The target of an SRV record is a domain name, so this would be redundant
(and more wordy, which is why I'm resisting).

> "For example, a DNS update that contains an RRset
>
>    Add to a Service Name and an RRset Add to a Service Instance Name,
>
>    where the Service Name does not reference the Service Instance Name,
>
>    is not a valid SRP update message"
>
> -> 'RRset Add' refers probably to the "Add To An RRset" semantics of RFC
> 2136? If so it would be good to use that exact same naming "Add To An
> RRset" including the quotes here.
>
> With present text I'm wondering if I missed something in the DNS specs
> that's called "RRset Add".
>

Good point, fixed.

>
> *** 2.3.3
>
> "If any existing KEY record corresponding to a
>
>    KEY record in the SRP Update does not match the KEY same record in
>
>    the SRP Update "
>
> -> grammar issue. 'match the KEY same record'    And a bit confusing: can
> a KEY record in an update correspond to a stored KEY record but not match
> at the same time?
>
>   Perhaps the 'name' concept should be introduced here to clarify.
> (Presumably the server will check if an existing Service Instance Name or
> existing Hostname is stored and look up the associated KEY for that.)
>

Fixed, thanks.

> "KEY record updates omitted from Service Description update are
>
>    processed as if they had been explicitly present: every Service
>
>    Description that is updated MUST, after the update, have a KEY RR,
>
>    and it must be the same KEY RR that is present in the Host
>
>    Description to which the Service Description refers."
>
> -> Can we replace here 'Service Description update' by 'Service
> Description Instruction' ?
>
> E.g.
>
> "KEY record updates omitted from Service Description Instructions are
>
>    processed as if they had been explicitly present: every Service
>
>    Description that is updated through a Service Description Instruction
> MUST, after the update, have a KEY RR,
>
>    and it must be the same KEY RR that is present in the Host
>
>    Description Instruction to which the Service Description Instruction
> refers."
>

I've updated the text—see what you think.

> "Otherwise, the server validates the SRP Update using SIG(0) on the
>
>    public key in the KEY record of the Host Description update."
>
> -> what about:
>
> "Otherwise, the server validates the SRP Update using SIG(0) against the
>
>    public key in the KEY record of the Host Description Instruction."
>
>
>
> (Using 'Instruction' and 'validates X against Y' instead of 'validations X
> on Y' )
>

OK

> *** 3
>
> "SRP update servers MUST check"
>
> ->"SRP servers MUST check"
>
>
>
OK

> *** 4.1
>
> [Nit] " MUST be removed at the same time-it is never valid for a service"
>
> -> " MUST be removed at the same time -- it is never valid for a service "
> or
>
> -> " MUST be removed at the same time: it is never valid for a service "
>

This is a rendering bug—the text uses &mdash;. I think we should leave it
and let the tools team fix the bug.

> 3rd paragraph mentions that a SRP is by default configured with a 2 hour
> Lease limit and 14-day KEY retaining time limit; and then it says that
> constrained devices may negotiate longer leases. This looks incorrect, as
> leases beyond configured upper limits won't be given by the server.
>
> Probably something else is meant here with 'negotiate'? For example if one
> configures 'default values' for Lease and Key-Lease in the server, the
> serve may extend to these default values if the client asks for something
> shorter. If the client asks for something longer than the 'default values',
> the server grants the longer values up to a hard limit that is also
> configured in the server.
>

This text is somewhat divorced from reality, but I've changed it as follows:

Constrained devices with limited battery that wake infrequently are likely
to request longer leases; servers that support such devices may need to set
higher limits.

> *** 5 Sleep Proxy
>
> The introduction text should clarify whether Sleep Proxy support for an
> SRP server is OPTIONAL or MANDATORY.
>
> Also to make more clear that for clients the use of this function is
> optional/OPTIONAL. (The last paragraph says so for constrained hosts, but
> nothing for the full-featured hosts.)
>
>
>
> In relation to above, the same questions from my review of -04 of
> 2020-10-13 still apply:
>
> - How would a service’s host know that the server is capable to do this?
> If it doesn't know then it cannot trust the mechanism to work and go to
> sleep.
>
> - Or, is there a way for the SRP server to reject an update with EDNS(0)
> OWNER option such that the service’s host knows not to use it again?
>
>
>
> [Nit] "Another use of SRP is for devices that sleep to reduce power
> consumption"
>
> -> ideally it should mention somewhere that the device that sleeps is an
> SRP client. Otherwise the statement is a bit too generic. E.g.
>
> "Another use of SRP is for SRP client devices that sleep to reduce power
> consumption, while being able to offer registered service(s) without
> interruption."
>
>
>
> "the device includes an EDNS(0) OWNER Option
> [I-D.cheshire-edns0-owner-option]"
>
> -> the reference here is Informational; how can this be? To implement the
> sleep proxy function it is normative to include the option.
>
>
>
> "When the DNS server receives a TCP SYN or UDP packet addressed to"
>
> -> "When the Sleep Proxy receives a TCP SYN or UDP packet addressed to"
>
> (see also below comment)
>
>
>
> "the Sleep Proxy must be on the same link"
>
> -> the term "Sleep Proxy" is not introduced. In the prior text when it
> says "should set up a proxy" the name could be formally introduced e.g.
> "should set up a Sleep Proxy" or "should set up a proxy, called the Sleep
> Proxy, "
>
>
>
> Overall, I miss some details about what the Sleep Proxy does once it
> receives a TCP SYN or UDP packet and it has woken up the sleepy device.
> Does it then stop the ARP/ND proxying for the sleepy device right away? And
> when would it again resume proxying?
>
> What does the proxy do with the receive TCP SYN or UDP packet, discard or
> forward to the woken sleepy device? (After how much waiting time?)
>

I reread the Sleep Proxy section carefully, and I think it's just
incomplete. Since we're in last call, this creates a bit of an issue—should
we spend the time to fix it, or should we specify sleep proxy as a separate
document? Given that we actually have a separate document that describes
the owner name option, I'm leaning in that direction, and have removed this
section from the current document.

> *** 6.1
>
> "Service Discovery Protocol servers"
>
> -> is this term defined? Should we say SRP servers here?
>

Yes, don't know how that happened. Probably very old text.

> "a promise made by the registration protocol"
>
> -> "a promise made by the service registration protocol"
>

This text is a bit squidgy. Protocols don't make promises. I've changed it
to: a promise made by the SRP Server to an SRP client

> "replacing all or part of the service registration information with
> information provided by an SRP client"
>
> -> the "SRP client" mentioned here I think should be "DNS client". Because
> the case mentioned looked like an authenticated (non-SRP) DNS client making
> DNS updates to records that were added by an SRP client.
>

Yes, that text was wrong.

> *** 7 "Privacy Considerations"
>
> This text mentions the requirements for TLS. Perhaps the TLS bits are
> better placed in Section 6, and Section 7 if needed can refer back to that.
> To me TLS seems one of the key ingredients to secure the protocol so I'd
> expect to read something / most-things about this in Section 6.   (Maybe
> I'm wrong here in case TLS can't provide the authentication - since in some
> deployments the client may have no trust anchor of the domain installed a
> priori, so it can't authenticate the SRP server even when TLS is used.)
>

At present, TLS doesn't provide authentication—that's correct. We don't
have a way to establish trust.

> *** 9.2
>
> "Availability of DNS Service Discovery Service Registration Protocol
>
>    Service for a given domain is advertised using the
>
>    "_dnssd-srp._tcp.<domain>."  SRV record gives the target host and
>
>    port where DNSSD Service Registration Service is provided for the
>
>    named domain."
>

>
> -> seems like a grammar issue here. Was this meant as a single sentence,
> or as 2 sentences separated by a dot before the "SRV record" text?
>
> E.g. I could interpret as "using the X SRV record which gives the Y" or as
> "using the X name. The SRV record gives the Y".
>
> Similar point for 9.3.
>

That is confusing. The reason for the trailing dot there is that it's a
fully-qualified domain name, and such names end in a '.' to indicate the
root label. But I think it's probably clearer to leave out the dot here.