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

Warren Kumari <warren@kumari.net> Tue, 28 April 2020 21:52 UTC

Return-Path: <warren@kumari.net>
X-Original-To: captive-portals@ietfa.amsl.com
Delivered-To: captive-portals@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 8795C3A09A8 for <captive-portals@ietfa.amsl.com>; Tue, 28 Apr 2020 14:52:05 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.899
X-Spam-Level:
X-Spam-Status: No, score=-1.899 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=kumari-net.20150623.gappssmtp.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id XqS7avJz0SKo for <captive-portals@ietfa.amsl.com>; Tue, 28 Apr 2020 14:52:03 -0700 (PDT)
Received: from mail-lj1-x235.google.com (mail-lj1-x235.google.com [IPv6:2a00:1450:4864:20::235]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id EAAC13A09AF for <captive-portals@ietf.org>; Tue, 28 Apr 2020 14:52:02 -0700 (PDT)
Received: by mail-lj1-x235.google.com with SMTP id g4so488640ljl.2 for <captive-portals@ietf.org>; Tue, 28 Apr 2020 14:52:02 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kumari-net.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=cwKV1xE3zwpm4nxxZj/WcCYivc8rCNz/3OBDwSPiTmI=; b=AbKJD71Dl4zPxCDaxcjr6rB/e0nIkWFCD6yOWE+Bq5ispO6KIfOaZzQGYL5iKhFedO 1ZWKfFVvAO0CNeruaSAhUUudpiv9/wNyElVCbVLGKEdOnhr19VOfbiEWOCD0wTTfI/YY J9UiX9BmI2x4Noszou8x8LeMVztOihCxdkm5BO/t80tSWFFc2zzlPiU9RXT11lw2VtAU QLt9MeiRijrgVSbC3p/ix4Cfthdihume1sB+RiwtUZxkEd7NKMHG0v8J7ZjMlKWIPO/k tYECwA0O2shbkuI8xipA7a4wn/abj36zSPcAKFaHsQKB5sPHmboA7PXfYCvqwoPZqBsU xt8g==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=cwKV1xE3zwpm4nxxZj/WcCYivc8rCNz/3OBDwSPiTmI=; b=a8EMcatpgYizAozyaKde3+TZw8SpsE0qr18aD4RnD9yOvTsxG8Wpm7hWJ2Zx5E2RLR FJbB4522IuuM4ysHQv40Q8IQ4f41o98cEOg1CafU7yx/RYsdrNQSwH80gah5RmZqE6UR CEFd46ZQ+nTpHL4/1GvPHn/9lyzYgmxCqPddQLvtr/bc0vl1uKSkpo9A94PDX70x++iF 8HukXs9NVmXvV4666Zjwg0MPagvKJ+qR89WRauhM2cHLyjIFSyDuUcuKzlHvy34BR15i YNyGVH0FksxycMice5MB17hqViHyprilsUoow2byoBmaKfLgXixJ6e0rwmDJ7+7ttCZQ oVzA==
X-Gm-Message-State: AGi0PuaigeoSPbfFhPXQgfVXe4mJvOU5YDr2zvg09Z5atuNYCbWYJoBV wkP8dX86sbiZldLRebTgRxTXLy3FJUX4NmOOAqDkAg==
X-Google-Smtp-Source: APiQypKJumev5GSeovvk6K/2qQdZKmsMeWS2dKVoTv0yKCwzk7FYpQH8szhJdPLIhaMCazzHyI3+GYluf7U6QNs6iHM=
X-Received: by 2002:a2e:9785:: with SMTP id y5mr17460523lji.66.1588110720489; Tue, 28 Apr 2020 14:52:00 -0700 (PDT)
MIME-Version: 1.0
References: <CALaySJ+tmuU_DZU6ZvC4RsnuRJAVfLgYyw_=RYbA6XBcgvgKJQ@mail.gmail.com> <CAMGpriU0LbpZC+57hO-MAgjXq-nMTJgWSTWh1ycPjU1hCOsrpg@mail.gmail.com>
In-Reply-To: <CAMGpriU0LbpZC+57hO-MAgjXq-nMTJgWSTWh1ycPjU1hCOsrpg@mail.gmail.com>
From: Warren Kumari <warren@kumari.net>
Date: Tue, 28 Apr 2020 17:51:23 -0400
Message-ID: <CAHw9_i+XXSV9SVzCxw7FDeKv-RAjf3iEc=DjZAEgBqJ5JzgJZQ@mail.gmail.com>
To: Erik Kline <ek.ietf@gmail.com>
Cc: Barry Leiba <barryleiba@computer.org>, "draft-ietf-capport-rfc7710bis.all@ietf.org" <draft-ietf-capport-rfc7710bis.all@ietf.org>, "captive-portals@ietf.org" <captive-portals@ietf.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://mailarchive.ietf.org/arch/msg/captive-portals/3h4JtHcCQkJ7sW6ArEripycCd0o>
Subject: Re: [Captive-portals] AD review of draft-ietf-capport-rfc7710bis-03
X-BeenThere: captive-portals@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Discussion of issues related to captive portals <captive-portals.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/captive-portals>, <mailto:captive-portals-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/captive-portals/>
List-Post: <mailto:captive-portals@ietf.org>
List-Help: <mailto:captive-portals-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/captive-portals>, <mailto:captive-portals-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 28 Apr 2020 21:52:06 -0000

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 <ek.ietf@gmail.com> wrote:
>
> Barry,
>
> Thanks for the prompt review.
>
> On Thu, Apr 23, 2020 at 9:18 PM Barry Leiba <barryleiba@computer.org> 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