Last Call Review of draft-ietf-dnsop-svcb-https-07
review-ietf-dnsop-svcb-https-07-genart-lc-worley-2021-08-14-00

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at
https://trac.ietf.org/trac/gen/wiki/GenArtfaq

Document:  draft-ietf-dnsop-svcb-https-07
Reviewer:  Dale R. Worley
Review Date:  2021-08-14
IETF LC End Date:  2021-08-19
IESG Telechat date:  unknown

Summary:

    The document is generally in quite good shape and is almost ready
    for publication, but it has a technical point that should be
    reviewed and some editorial issues.

Technical issues:

Whereas SRV records have both a priority and weight field, SVCB
records have only a priority field.  (This point is not addressed in
section C.1.)  This point should be reviewed to ensure that is what we
want.  It may be that the weight field has not proven useful in
practice, but my concern is that people may mistakenly think that
implementing weight is complex and left it out of SVCB because of
that.  In fact, SRV records processed in the following way produce
results with the specified statistical properties:

    - Assign each record
        effective_weight = - weight * log(uniform_random(0, 1))

    - Sort the records first by priority in ascending order then by
      effective_weight in descending order.

You may want to add a requirement that all SVCB-compatible RR types
have presentation formats that are identical to SVCB (except for the
RR type name).  (see comments on section 8)

Major editorial issue:

While I am sure the details of the relationship between SVCB and HTTPS
are given correctly, the text isn't as clear as it should be regarding
the meaning and requirements of "SVCB-compatible RR types".  It would
be an improvement to:

    - Add a section that lays out the rules for SVCB-compatible RR
      types.

    - Move/copy the 4th and 5th paragraphs of the Introduction to the
      new section.

    - Clarify that the term "SVCB" is used to mean (1) the SVCB RR
      type, (2) collectively, SVCB and all SVCB-compatible RR types
      (via the sentence "All behaviors described as applying to the
      SVCB RR also apply to the HTTPS RR unless explicitly stated
      otherwise."), and (3) the SVCB conceptual structure (e.g. App. B
      "a non-normative summary of the HTTP mapping for SVCB").  This
      should probably be done in the Terminology section, where oddly
      "SVCB" is mentioned but not specifically listed as a defined
      term.
      
    - "SVCB-compatible" should be listed as a defined term.

    - Instead of defining "SVCB-compatible" by the example of HTTPS,
      identify the statements about HTTPS that are applicable to all
      SVCB-compatible records and state them as such.  In particular,
      revise "All behaviors described as applying to the SVCB RR also
      apply to the HTTPS RR unless explicitly stated otherwise."
     
    - Add a note that when the mapping is first established for a
      protocol, a choice must be made whether it uses SVCB RRs or a
      new SVCB-compatible RRs.  But once that choice is made, it is
      impossible (or very difficult) to change it in an
      upward-compatible way.

The current document reads as if HTTPS was devised well into the
process based on its ability to reduce the number of DNS queries, and
then late in the process it was realized that HTTPS can be generalized
to SVCB-compatible, but the document wasn't edited to fully describe
"SVCB-compatible".

Minor editorial issues:

1.  Introduction

Some of the text here regarding SVCB-compatible records as a whole
seems to be normative (in that it is not stated anywhere else).  But
that has been covered in the editorial point above.

It's may worth mentioning here that while the document defines SVCB
and then derives HTTPS from it, the document defines only the mapping
for the http: and https: schemes, and defines them as using the HTTPS
record.  So there are at this point no schemes with mappings for SVCB
proper.

1.1.  Goals of the SVCB RR

      This is important, as DNS does not provide any way to tie
      together multiple RRs for the same name.

This is ambiguous, as of course DNS ties together all of the RRs for
the same name.  Perhaps:

      This is important, as DNS does not provide any way to specify
      multiple groups of RRs for the same name.

1.4.  Terminology

This is usually section 2, which clarifies that it is normative, in
that the definitions are needed to understand the normative
statements in the rest of the document.

2.2.  RDATA wire format

   When the list of SvcParams is non-empty (ServiceMode)

Actually, an AliasMode record can have a non-empty SvcParams, it
simply SHOULD NOT.  The subtle case is whether an AliasMode record
with non-empty SvcParams is governed by this section (which requires
the SvcParams to be properly formatted and "If any RRs are malformed,
the client MUST reject the entire RRSet") or by section 2.4.2 ("In
AliasMode, ...  recipients MUST ignore any SvcParams that are
present.")  I recommend stiffening the requirement so that AliasMode
records MUST NOT have SvcParams (and having the zone file processor
enforce it).

   *  a 2 octet field containing the length of the SvcParamValue as an
      integer between 0 and 65535 in network byte order (but constrained
      by the RDATA and DNS message sizes).

It seems redundant to include the parenthesized text, as the same
requirement is applied in the 3rd bullet point of this section.

   *  an octet string of this length whose contents are in a format
      determined by the SvcParamKey.

More exactly, "... whose contents are the SvcParamValue in a format
determined by the SvcParamKey."

2.3.  SVCB query names

   When a prior CNAME or SVCB record has aliased to a SVCB record, each
   RR shall be returned under its own owner name.

Oddly, I've not heard "owner" applied to a DNS record before, but
"owner" is defined in [DNSTerm].

   (Parentheses are used to ignore a line break ([RFC1035],
   Section 5.1).)

I would add "... in DNS RR presentation format".

3.  Client behavior

This could be made clearer by rearranging the information in these
paragraphs to:

   This procedure does not rely on any recursive or authoritative DNS
   server to comply with this specification or have any awareness of
   SVCB.  [this paragraph is unchanged]

   A client is called "SVCB-optional" if it can connect without the use
   of ServiceMode records, and "SVCB-reliant" otherwise.  Clients for
   pre-existing protocols (e.g.  HTTPS) SHALL implement SVCB-optional
   behavior (except as noted in Section 3.1 and Section 9.1).

   SVCB-optional clients SHOULD issue in parallel any other DNS
   queries that might be needed for connection establishment using
   non-SVCB connection modes.

   Once SVCB resolution has concluded, whether successful or not,
   SVCB-optional clients SHALL append to the list of known
   endpoints an endpoint consisting of the final value of
   $QNAME, the authority endpoint's port number, and no SvcParams.
   (This endpoint will be attempted before falling back to non-SVCB
   connection modes.  This ensures that SVCB-optional clients will
   make use of an AliasMode record whose TargetName has A and/or AAAA
   records but no SVCB records.)
                                       
   The client proceeds with connection establishment via SVCB's list
   of known endpoints, if any.  Clients SHOULD try higher-priority
   alternatives first, with fallback to lower-priority alternatives.
   Clients issue AAAA and/or A queries for the selected TargetName,
   and MAY choose between them using an approach such as Happy
   Eyeballs [HappyEyeballsV2].

   If the client is SVCB-optional and connecting using the list of
   known endpoints failed, it then attempts non-SVCB connection modes.

   [continue with "Some important optimizations..."]

3.1.  Handling resolution failures

   If SVCB resolution fails due to [...]

This condition isn't actually defined in section 3.  A clearer
formulation is:

   If any DNS query specified in the SVCB resolution process fails due
   to a SERVFAIL error, transport error, or timeout, and DNS exchanges
   between the client and the recursive resolver are cryptographically
   protected (e.g. using TLS [DoT] or HTTPS [DoH]), a SVCB client
   SHOULD abandon the connection even if it is SVCB-optional.

3.2.  Clients using a Proxy

   Providing the proxy with the final TargetName has several benefits:

Somewhat clearer as "with the final TargetName rather than its IP
address".

4.2.  Recursive resolvers

   Recursive resolvers that are aware of SVCB SHOULD help the client to
   execute the procedure in Section 3 with minimum overall latency, by
   incorporating additional useful information into the response.  For
   the initial SVCB record query, this is just the normal response
   construction process (i.e. unknown RR type resolution under
   [RFC3597]).  For followup resolutions performed during this
   procedure, we define incorporation as adding all useful RRs from the
   response to the Additional section without altering the response
   code.

   Upon receiving a SVCB query, recursive resolvers SHOULD start with
   the standard resolution procedure, and then follow this procedure to
   construct the full response to the stub resolver:

The wording could be clarified:

   Recursive resolvers that are aware of SVCB SHOULD help the client
   to execute the procedure in Section 3 with minimum overall latency
   by incorporating additional useful information into the response.
   The normal response construction process (i.e. unknown RR type
   resolution under [RFC3597]) generates the Answer section of the
   query.

   Upon receiving a SVCB query, recursive resolvers SHOULD start with
   the standard resolution procedure, and then follow this procedure to
   obtain additional records to incorporate into the Additional section:

This provides the definition of "incorporate" which is used in the
following clauses.

--

   1.  Incorporate the results of SVCB resolution.  If the chain length
       limit has been reached, terminate successfully (i.e. a NOERROR
       response).

Shorten this to:

   1.  Incorporate the results of SVCB resolution.  If the chain length
       limit has been reached, terminate.

--

After this:

   In this procedure, "resolve" means the resolver's ordinary recursive
   resolution procedure, as if processing a query for that RRSet.  This
   includes following any aliases that the resolver would ordinarily
   follow (e.g.  CNAME, DNAME [DNAME]).

add:

   In all cases, errors or anomalies in obtaining additional records
   MAY cause this process to terminate, but MUST NOT themselves cause
   the resolver to send a failure response.

4.3.  General requirements

   No part of this specification requires
   recursive resolvers to alter their behavior based on its contents,
   even if the contents are invalid.

It seems like this sentence is redundant and could be omitted.  But
better would be to start it "This specification does not require ...".

   Recursive resolvers MAY validate
   the values of recognized SvcParamKeys and reject records containing
   values which are invalid according to the SvcParam specification.

It might be better to s/reject/ignore/, because "reject" seems to
imply a specific error response toward some source of the records, and
resolvers don't do that.

8.  Using SVCB with HTTPS and HTTP

Some of this information is actually part of the general structure of
SVCB-compatible records and should be moved to the general discussion
of SVCB-compatibility.

   Clients MUST NOT perform SVCB queries or
   accept SVCB responses for "https" or "http" schemes.

This is true of a client resolving any scheme whose mapping provides a
separate RR type.

   The HTTPS RR wire format and presentation format are identical to
   SVCB, and both share the SvcParamKey registry.  SVCB semantics apply
   equally to HTTPS RRs unless specified otherwise.  The presentation
   format of the record is:

   Name TTL IN HTTPS SvcPriority TargetName SvcParams

All of the above applies to any SVCB-compatible RR, and so should be
documented as such -- except the presentation format requirement.

But for sanity's sake, SVCB-compatibility should require that the
presentation format be the same as for SVCB (except for the RR type
name).

8.1.  Query names for HTTPS RRs

   Reusing the service name also allows ...

I assume this means "Using one record for both HTTP and HTTPS allows ..."

10.1.  Structuring zones for flexibility

   Each ServiceForm RRSet can only serve a single scheme.

Is this "ServiceMode"?

Appendix B.  HTTP Mapping Summary

   This table does not indicate any SvcParamKeys that servers are
   required to publish, or that clients are required to implement,
   because there are none in this mapping.

It would be better to show these two facts as entries, as other
mappings may need to give values, and putting entries in the one
existing example will help them remember that:

            ...
            +--------------------------+----------------------+
            | *Special behaviors*      | HTTP to HTTPS        |
            |                          | upgrade              |
            +--------------------------+----------------------+
            | *SvcParamKeys servers    | none                 |
            | must publish*            |                      |
            +--------------------------+----------------------+
            | *SvcParamKeys clients    | none                 |
            | must implement*          |                      |
            +--------------------------+----------------------+

I'm not sure if the asterisks around the items in the first column add
any value.

12.  Security Considerations

   A hostile DNS intermediary might forge AliasForm "." records

This paragraph uses "AliasForm" twice, which should probably be
"AliasMode".

D.1.  AliasForm

Is this "AliasMode"?

D.2.  ServiceForm

Is this "ServiceMode"?

This section uses "vector" in three places where "form" or "example"
would likely be better.

[END]