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

From: Barry Leiba <[email-masked]>
Date: Fri, 24 Apr 2020 00:17:48 -0400

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.

— Section 2 —

   negotiation ([RFC7231] Section 3.4), captive portals implementors

Make it “captive portal implementors” (or “implementors of captive
portals”).

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.

   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?

   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.

— 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.

— 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.

— 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.

—
Barry

---

From: Erik Kline <[email-masked]>
Date: Sat, 25 Apr 2020 21:37:29 -0700

Barry,

Thanks for the prompt review.

> Please use the new BCP 14 boilerplate from RFC 8174.

Fixed in the repo (commit
<https://github.com/capport-wg/7710bis/commit/488e6b5df49b8a308d425a4911fd49613f7dd590>
).

— Section 2 —
> Make it “captive portal implementors” (or “implementors of captive

Fixed in the repo (commit
<https://github.com/capport-wg/7710bis/commit/7f1d8a27e3a33e62aa7f161d0cbf02be084c53a6>
).

> information can be merged or removed.

Filed issue #17 <https://github.com/capport-wg/7710bis/issues/17>.

> NOT, rather than MUST NOT?

Filed issue #18 <https://github.com/capport-wg/7710bis/issues/18>.

> would seem more in line with how “ietf:param” URNs are generally done.

Filed issue: #19 <https://github.com/capport-wg/7710bis/issues/19>.

— Sections 2.1 and 2.2 —
> specifying in the text the size of the “len” field.

Filed issue: #20 <https://github.com/capport-wg/7710bis/issues/20>.

— Section 3 —
> be removed, to avoid any confusion.

Filed issue: #21 <https://github.com/capport-wg/7710bis/issues/21>.

— Section 5 —
> But, again, as you judge best; this is just a comment.

Filed issue: #22 <https://github.com/capport-wg/7710bis/issues/22>.

—

---

From: Barry Leiba <[email-masked]>
Date: Mon, 27 Apr 2020 10:28:02 -0400

> Thanks for the prompt review.

You're welcome, and thanks for the prompt reply!

I've eliminated the resolved items below, and leave the ones for which
issues are open, for my own quick reference.  I think they should be
easy to resolve, and then we can get a revised I-D and start last
call.

-- 
Barry

> Filed issue #17.

>> Why not, and how does it maintain interoperability to have this as
>> SHOULD NOT, rather than MUST NOT?
>
> Filed issue #18.

> Filed issue: #19.

>> 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.
>
> Filed issue: #20.

>> 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.
>>
> Filed issue: #22.

---

From: Warren Kumari <[email-masked]>
Date: Tue, 28 Apr 2020 17:51:23 -0400

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 <[email-masked]> 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 —
> Fixed in the repo (commit).
>
>> — Section 2 —
>>
>> 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.
>
>> Why not, and how does it maintain interoperability to have this as SHOULD NOT, rather than MUST NOT?
>
>
> Filed issue #18.
>
>> 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.

>> 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.
>>

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.

-- 
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

---

From: Barry Leiba <[email-masked]>
Date: Wed, 29 Apr 2020 01:46:15 -0400

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

Thanks!

Barry

On Tue, Apr 28, 2020 at 5:52 PM Warren Kumari <[email-masked]> 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
> (
> https://github.com/capport-wg/7710bis/commit/d229279dfba7f2a7d3e1f50be2ecf9244b4bc9ba
> )
>
> me that the Security Considerations in 7710 rather buried the lede.  The