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

[Ted Lemon <[e-mail]>]

A slight nit: RFC9157 updates RFC8624, but not in a way that is relevant to
this reference, so I'm going to leave the reference to 8624.

On Fri, Jul 7, 2023 at 2:41 PM Ted Lemon <[e-mail]> wrote:

> Thanks for the review, Patrik!
>
> On Sun, Jun 11, 2023 at 10:29 AM Patrik Fältström via Datatracker <
> [e-mail]> wrote:
>
>> Reviewer: Patrik Fältström
>> Review result: Ready with Issues
>>
>> Most of these comments are nits and related to how the document is
>> written. It
>> might be more clear to someone that already implements mDNS than to
>> someone
>> "only" deals with DNS. I think the document is (as you see below) a bit
>> unclear
>> on issues. Whether they should actually be made more clear or not can be
>> discussed.
>>
>> SRP is an acronym that has been in use many times. For example for Spatial
>> Reuse Protocol / Dynamic Packet Transport (SRP/DPT) that can be found for
>> example at Cisco:
>> <
>> https://www.cisco.com/c/en/us/tech/optical/spatial-reuse-protocol-dynamic-packet-transport-srp-dpt/index.html
>> >.
>> I presume the choice will be SRP although there are many acronym
>> collisions...?
>>
>
> This isn't even an RFC. I don't think we have a problem here. It's really
> DNSSD SRP, but we don't need to say that every time. :)
>
>
>> In section 3.2.1 there is some text that talks about what RR types that
>> are
>> supported. At least for me the text ends up being a bit unclear on
>> whether it
>> describes what is implemented, or what is required by entities being in
>> accordance with this specification. As specified in many DNS related RFCs,
>> protocols should allow any RR Type be used, and limiting protocols to
>> some is
>> by experience a bad thing. For example all crazy use of TXT records
>> because
>> implementations do not handle RR Types properly.
>>
>
> This is specifically for DNSSD, so we tried to keep it simple. DNSSD uses
> PTR, TXT, SRV and A/AAAA in very specific ways. The working group
> explicitly chose not to make this more general. That's what RFC2136 is for.
>
>
>> I think 3.2.1 must be more clear, specifically second bullet in second
>> paragraph.
>
>
>
>> I also think the specifically is too wordy. Example is section 3.2.3 that
>> starts by explaining how this specification is NOT working by explaining
>> DNS
>> updates. Then it is later explaining how this spec works.
>>
>> This spec should be about how things according to this spec should work.
>> Maybe
>> it is also explaining differences from other protocols, but it should not
>> explain on what and how this is *not* doing things. Makes it harder to
>> implement.
>>
>
> You're misunderstanding the purpose of this section. The goal of SRP is to
> provide a very constrained update mechanism for DNS specifically to support
> DNSSD, and the reason for all this "it is NOT an SRP update" is so the
> implementor knows what updates it can treat as SRP updates (and do FCFS
> with) and what updates it can't. So this text is exactly as intended, and
> your proposed changes would essentially break the specification.
>
>
>>
>> In section 3.2.4 there is a claim "This model does not work for automatic
>> service registration.". I do not understand why it is not. I can guess it
>> is
>> "impractical" as there must be a shared secret deployed and various other
>> things, which implies the zero-conf piece of service registration is
>> messy, but
>> "...does not work..."?
>>
>
> This is what we intended to say. It does not work for this use case. I
> don't think there's any controversy about that.
>
>
>> I also think wording in section 1 (related to 3.2.4) is unfortunate. It
>> says in
>> sixth paragraph "to authenticate both the initial claim and subsequent
>> updates". I do not think it is authenticating the initial claim. It is
>> using
>> the FCFS security model together with (section 6.1), if TCP is used,
>> inability
>> to inject registrations from outside network locations.
>>
>
> So the objection is the use of the term "authentication?" The sense in
> which it authenticates the update is precisely that we know that the update
> came from the same host that sent the initial registration. It's true that
> the initial update is not authenticated, and I don't think the document
> claims otherwise, so I don't see what change I could make here other than
> to add more text to clarify something I think is already pretty clear.
>
>>
>> In section 3.2.4 there is a statement "The goal is not to provide the
>> level of
>> security of a network managed by a skilled operator." which honestly I do
>> not
>> understand what it means and why it is in this text.
>>
>
> You're the third person to mention this sentence, which I agree is pretty
> confusing and also useless, so it's been removed. :)
>
>
>> In section 3.2.5.3 there is a time constraint of lease time of 14 days
>> that is
>> "typical" but it can be chosen to something else. As this is a choice that
>> might impact the security (and reuse of the same name) my question is
>> whether
>> it is not for security reasons necessary to say it MUST be higher than a
>> certain lowest value.
>>
>
> No, I don't think so. Although we do suggest in the update-lease document
> that values lower than 30s are a bad idea for performance/DoS reasons.
>
>
>> In section 6.3 a reference is made to RFC8624. Should be to RFC9157.
>>
>
> Thanks for noticing that. I added this reference before 9157 was published
> and didn't notice the update.
>
>
>> In 3.2.5.4 there is a claim that compression saves "substantial space".
>> Although it might do, I think this specification should stay at saying
>> "compression MUST be supported".
>>
>
> Yeah, that wording is pretty optimistic, isn't it?  I changed it as
> follows:
>
> Compression of the
>      target can save space in the SRP Update, so we want clients to be
> able to assume that the SRP server will handle
>      this. Therefore SRP registrars MUST support compression of SRV RR
> targets.
>
>
>> In 3.3.1.3 it is stated that some A or AAAA records can be ignored by the
>> server. I think it would be more proper to say that the server can ignore
>> ANY
>> request due to contents of the update request be "weird".
>>
>
> I don't know what specific advice we'd give here, and I'd rather not say
> something this vague.
>
> In section 4 about TTLs it is said the request TTL should only be
>> advisory. I
>> understand why the SRP registrar might believe the TTL is too short, but
>> that
>> should be flagged in the response. A client must according to my view be
>> able
>> to request something short so that the client can honor the use of the
>> service
>> for the protected time period.
>>
>
> This is the wrong model for what we're doing here. If this were a general
> DNS protocol, that might make sense, but the goal here isn't to provide a
> general mechanism for updating DNS zones. It's to provide a replacement for
> mDNS using DNS. In that context, the client's preference can only ever be
> advisory. If the client wants a service to only be valid for a short time,
> it can use a short lease, and in fact that is the current practice. A short
> TTL would not actually accomplish this, if the zone weren't updated to
> withdraw the service.
>
> Anyway, I've updated the document as best I can to address your
> suggestions. Sorry about my lack of cooperation on the ones that I don't
> think should be changed!
>
>