Re: [Captive-portals] AD review of draft-ietf-capport-rfc7710bis-03

[Barry Leiba <[e-mail]>]

You have, indeed, and I’ve just requested last call.

Thanks!

Barry

On Tue, Apr 28, 2020 at 5:52 PM Warren Kumari <[e-mail]> wrote:

> Thank you -- I think that we have addressed all of the comments (thank
> you!).
>
> See below.
>
> On Sun, Apr 26, 2020 at 12:37 AM Erik Kline <[e-mail]> wrote:
> >
> > Barry,
> >
> > Thanks for the prompt review.
> >
> > On Thu, Apr 23, 2020 at 9:18 PM Barry Leiba <[e-mail]>
> wrote:
> >>
> >> Good work on this, folks: a nice improvement on 7710, and let’s hope
> for wider deployment.  Some comments below; I think we should have a quick
> revised I-D before we go to last call, mostly because of the comment about
> the diagrams in Sections 2.1 and 2.2, though most of the comments are
> editorial.
> >>
> >> — Section 1.1 —
> >> Please use the new BCP 14 boilerplate from RFC 8174.
> >
> >
> > Fixed in the repo (commit).
> >
> >> — Section 2 —
> >>
> >>    negotiation ([RFC7231] Section 3.4), captive portals implementors
> >>
> >> Make it “captive portal implementors” (or “implementors of captive
> portals”).
> >
> >
> > Fixed in the repo (commit).
> >
> >> It looks like the two consecutive paragraphs that both begin with “A
> captive portal” have some duplication with regard to the absence of
> “Accept” headers; please check that and adjust if you agree that some
> information can be merged or removed.
> >
> >
> > Filed issue #17.
> >
> >>    The URI SHOULD NOT contain an IP address literal.
> >>
> >> Why not, and how does it maintain interoperability to have this as
> SHOULD NOT, rather than MUST NOT?
> >
> >
> > Filed issue #18.
> >
> >>    Networks with no captive portals MAY explicitly indicate this
> >>    condition by using this option with the IANA-assigned URI for this
> >>    purpose (see Section 4.1.1).  Clients observing the URI value
> >>    "urn:ietf:params:capport-unrestricted" MAY forego time-consuming
> >>    forms of captive portal detection.
> >>
> >> Two things here:  I don’t see that the forward reference to 4.1.1 adds
> anything, as the only thing that section does that’s useful here is to
> repeat the URI that you’re already including.  And why
> “capport-unrestricted” rather than “capport:unrestricted”?  The latter
> would seem more in line with how “ietf:param” URNs are generally done.
>
> ... so, we had incorrectly thought that the colon character (:) was
> similar in operation to the . in a DNS name, and implied a hierarchy
> -- looking at RFC8141 it turns out that we were wrong, and that colons
> can be part of the NSS:
>
>    Because the colon character (":") is used to separate "urn" from the
>    NID and the NID from the NSS, it's tempting to think of the entire
>    URN as being structured by colon characters and to assume that colons
>    create a structure or hierarchy within the NSS portion of the URN.
>    Such structure could be specified by a particular NID specification,
>    but there is no implicit structure.  In a URN such as
>
>       urn:example:apple:pear:plum:cherry
>
>    the NSS string is "apple:pear:plum:cherry" as a whole, and there is
>    no specific meaning to the colon characters within that NSS string
>    unless such meaning is described in the specification of the
>    "example" namespace.
>
> So, we are changing to capport:unrestricted  (Commit:
>
> https://github.com/capport-wg/7710bis/commit/aae1c3cbd08d3822fa22056f7c2a7ec4a12e0a9a
> ) -- Thank you!
>
> >
> >
> > Filed issue: #19.
> >
> >> — Sections 2.1 and 2.2 —
> >> Why aren’t the diagrams for the two in the same format?  And neither
> explicitly says what the length of the “len” field is.  In the v4 version,
> one has to guess that it’s the same size as the “code” field, 1 byte.  In
> the v6 version, one can look at the diagram.  I suggest making the diagrams
> the same format (calibrated with bit/byte markings) and explicitly
> specifying in the text the size of the “len” field.
>
> These were represented differently because of hysterical raisins /
> tradition -- the format we had used for IPv4 was how many of the other
> IPv4 documents represented their DHCP option (and the same for
> IPv6)... however, seeing them on the same page does look silly, and so
> we have changed the IPv4 one to look like the IPv6 one (
> https://github.com/capport-wg/7710bis/pull/28 )
>
> We also fixed the missing length for length...
> (
> https://github.com/capport-wg/7710bis/commit/d229279dfba7f2a7d3e1f50be2ecf9244b4bc9ba
> )
>
> In addition, we clarify that the URI should not exceed 255 bytes if
> you are using IPv4 and IPv6 (because they have to match) - this closes
> Issues #20 and PR #25 (https://github.com/capport-wg/7710bis/issues/20
> , https://github.com/capport-wg/7710bis/pull/25 ). Thanks to Subash
> Tirupachur Comerica for pointing this out.
>
>
> >
> >
> > Filed issue: #20.
> >
> >> — Section 3 —
> >>
> >>    Specifically,
> >>    URIs learned via any of the options in Section 2 should take
> >>    precedence over any URI learned via some other mechanism
> >>
> >> Given the MUST that comes before this, I think the word “should” ought
> to be removed, to avoid any confusion.
> >
> >
> > Filed issue: #21.
> >
> >> — Section 5 —
> >> Purely a judgment thing here, so do what you hink is best: it seems to
> me that the Security Considerations in 7710 rather buried the lede.  The
> bottom line is that by removing the MitM interception, this mechanism
> improves security by making the portal and its actions visible, rather than
> hidden, and that vulnerabilities that exist here also existed before and
> are now lessened.  I would say something like that up front, in a new first
> paragraph, and then continue with the existing text as is.
> >>
> >> But, again, as you judge best; this is just a comment.
>
> Thank you! We (well, mainly Erik) are bad at tootin' our own horn, but
> with your prompting we have reordered and strengthered this --
> https://github.com/capport-wg/7710bis/pull/29
>
> We also fixed the TBD to say Option 114 -
>
> https://github.com/capport-wg/7710bis/commit/edbaca20dab8a5b83353b9637c7cde0a8ac5486e
>
>
> Thank you again for your comments, I think we've addressed them all
> and are uploading a new version (-04)
>   Warren and Erik.
>
> >
> >
> > Filed issue: #22.
> >
> >> —
> >> Barry
> >>
>
>
> --
> I don't think the execution is relevant when it was obviously a bad
> idea in the first place.
> This is like putting rabid weasels in your pants, and later expressing
> regret at having chosen those particular rabid weasels and that pair
> of pants.
>    ---maf
>