Re: [dnssd] Opsdir last call review of draft-ietf-dnssd-srp-20

[Dhruv Dhody <[e-mail]>]

Thanks Ted for taking my comments into consideration!

On Sat, Jul 8, 2023 at 12:12 AM Ted Lemon <[e-mail]> wrote:

> Dhruv, thanks for the review!
>
> On Tue, Jun 13, 2023 at 5:33 AM Dhruv Dhody via Datatracker <
> [e-mail]> wrote:
>
>> Reviewer: Dhruv Dhody
>> Review result: Has Nits
>>
>> # OPSDIR review of draft-ietf-dnssd-srp
>>
>> I have reviewed this document as part of the Operational directorate's
>> ongoing
>> effort to review all IETF documents being processed by the IESG.  These
>> comments were written with the intent of improving the operational
>> aspects of
>> the IETF drafts. Comments that are not addressed in the last-call may be
>> included in AD reviews during the IESG review.  Document editors and WG
>> chairs
>> should treat these comments just like any other last-call comments.
>>
>> The document is very clear and well-written. The motivation is described
>> well.
>> A separate section dealing with Operational Considerations would be an
>> excellent addition that could explicitly deal with Backward compatibility,
>> Logging requirements, Default values settings, Monitoring requirements
>> etc. See
>> RFC 5706 for inspiration. This is just a suggestion...
>>
>
> Thanks. I think this isn't a bad idea in theory, but in practice the use
> cases we know about for SRP are all situations where there is no operator
> in the sense you mean: e.g. a Thread border router has no operator other
> than the end-user, who isn't going to rely on this document. A home CE
> router with an SRP server is in the same boat.
>
> I think there may be some value in thinking about the operations model for
> SRP when deployed in an enterprise or educational setting, but we don't
> actually have any experience to share, so it would be purely speculative. I
> think this probably needs to wait for a later document, unfortunately.
>
>
>>
>> The document is ready. I have a few minor comments and nits -
>>
>> ## Minor
>>
>> * I suggest the I-D explicitly state the default values which can be
>> overridden
>> via configurations. The use of the word "typically" in section 3.2.5.3 is
>> a bit
>> unusual.
>>
>
> We're reporting how this is actually used in practice. I don't know how to
> write a specification that would be appropriate. We actuallly at one time
> specified a value of one hour (or maybe it was two, I can't remember) but
> then we saw that in practice, people were using very different values,
> sometimes a different value for paired devices than unpaired, etc. So our
> goal became to simply talk about the tradeoffs and make suggestions.
>
>
>> * Section 8. My preference would be to disregard brevity and list all
>> considerations for "service.arpa" instead of relying on "home.arpa" in
>> RFC8375.
>> In my reading, the text refers to homenet at places and seems incorrect to
>> blindly rely on it. Again just a suggestion and something to think about.
>>
>
> The IANA review agreed with you, and this will be in the new version of
> the document.
>
>
>> ## Nits
>>
>> * Expand IoT in Abstract. Also, put the abbreviation next to "DNS-Based
>> Service
>> Discovery" as you use the abbreviation later on.
>>
>
> I just took IoT out—it's not really accurate anyway. We give the
> abbreviation for DNSSD in the Introduction--it doesn't belong in the
> abstract.
>
>
>> * Section 3.1.1.
>>
>>     * Remove the "," at "..a registration domain, or discover the
>> default.."
>>
>>     * Remove the "," at "..mechanisms are possible, but are.."
>>
>>     * s/out of scope for this document/out of the scope of this document/
>>
>>     * Add a "," at "For these names they then discover" i.e. "For these
>> names,
>>     they then discover"
>>
>
> These are issues for the RFC editor.
>
>
>> * Section 3.2.4
>>
>>     * Expand TSIG
>>
>
> Good point, changed to Secret Key Transaction Signatures.
>
>
>>     * I suggest rewording this "The goal is not to provide the level of
>>     security of a network managed by a skilled operator."!
>>
>
> The secdir review also complained about this. I deleted it—I think it's
> just confusing.
>
>
>> * Add a suitable reference for "a YXDomain RCODE" (Section 3.2.5.2)
>>
>
> Done.
>
>
>> * Weird capitalization in "..both the Delete An RR From An RRset update
>> and the
>> Add To An RRSet update,.." (Section 3.2.5.5.2)
>>
>
> That's how we did it. I guess we could have used single quotes instead?
>
>
>> * Section 3.3.1
>>
>>     * s/RFC2136/[RFC2136]/g -- If you don't want to make this update,
>> consider
>>     using a hyphen as in RFC2136-implementations etc.
>>
>
> Should be an xref, fixed.
>
>>
>>     * Should you also state what happens when the MUST in this section
>> are not
>>     met?
>>
>
> See "Valid SRP Update Requirements". I don't think it will help to add
> more text about this here.
>
>
>>
>> * s/are rejected with Refused./are rejected with Refused RCODE./ (section
>> 3.3.6)
>>
>> OK
>
>
>> * Section 6.1
>>
>>     * Add reference to TCP Fast Open
>>
>
> OK
>
>
>>
>>     * s/credentials to to update/credentials to update/
>>
>
> Yup.
>
>
>>
>> * Table 1, please remove the last "." in "default.service.arpa."; See
>>
>> https://www.iana.org/assignments/locally-served-dns-zones/locally-served-dns-zones.xhtml
>
>
> It's an FQDN, so it's correct to have a '.' on the end.
>
>
>> * The IDNITS has some warnings. I guess that no change is needed, but just
>> making sure -
>>
>> https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-dnssd-srp-20.txt
>
>
> It doesn't like i18n. These are peoples' names, so too bad for it. :)
>
> Thanks again for the review. I have applied your suggestions except as
> noted above.
>