Re: [Captive-portals] AD review of draft-ietf-capport-architecture-07
David Dolson <ddolson@acm.org> Mon, 27 April 2020 03:39 UTC
Return-Path: <ddolson@acm.org>
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 DD43B3A0901;
Sun, 26 Apr 2020 20:39:32 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.235
X-Spam-Level:
X-Spam-Status: No, score=-1.235 tagged_above=-999 required=5
tests=[BAYES_00=-1.9, SPF_SOFTFAIL=0.665]
autolearn=no autolearn_force=no
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 T-cXZ5UknI-y; Sun, 26 Apr 2020 20:39:31 -0700 (PDT)
Received: from smtp2.execulink.net (smtp2.execulink.net [69.63.44.83])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(No client certificate requested)
by ietfa.amsl.com (Postfix) with ESMTPS id 221F43A08FE;
Sun, 26 Apr 2020 20:39:30 -0700 (PDT)
Received: from webmail.execulink.ca ([199.166.6.210])
by smtp2.execulink.net with esmtpsa (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256)
(Exim 4.92.3) (envelope-from <ddolson@acm.org>)
id 1jSucD-0003hN-D5; Sun, 26 Apr 2020 23:39:30 -0400
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
format=flowed
Content-Transfer-Encoding: 8bit
Date: Sun, 26 Apr 2020 23:39:29 -0400
From: David Dolson <ddolson@acm.org>
To: Barry Leiba <barryleiba@computer.org>
Cc: draft-ietf-capport-architecture.all@ietf.org, captive-portals@ietf.org
Reply-To: ddolson@acm.org
In-Reply-To: <CALaySJ+K1FZu5POa=TLz2-ON8=jvyE03gqdi+5+cNj8X4E9RoA@mail.gmail.com>
References: <CALaySJ+K1FZu5POa=TLz2-ON8=jvyE03gqdi+5+cNj8X4E9RoA@mail.gmail.com>
User-Agent: Roundcube Webmail/1.4-git
Message-ID: <a4a3747ef330d8b2ac94a178ab691ca8@golden.net>
X-Sender: ddolson@acm.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/captive-portals/TXdICwR5KTFIcZpk24wqgdAiSDs>
Subject: Re: [Captive-portals] AD review of
draft-ietf-capport-architecture-07
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: Mon, 27 Apr 2020 03:41:59 -0000
Barry, Thanks for reviewing. I've committed some changes to the repo and opened some issues too. Nits fixed in commit https://github.com/capport-wg/architecture/commit/f7de70f8dc71dd3f22e2db4208641d1975278ec0 On 2020-04-24 19:35, Barry Leiba wrote: > Thanks for the work in this document. Here’s my AD review; I think a > revised I-D is needed before we go to last call. > > General: > Expect comments during IESG Evaluation about the extensive use of BCP > 14 key words in an Informational document. I don’t object to it here, > though I do find all the “MAY”s odd (example: “A device MAY query this > API at any time”, rather than “A device may query this API at any > time”), but I do expect some ADs to comment on it. I've reviewed all upper-case "MAY", and I believe they are used as intended. We've allowed the user equipment to participate or not in various ways. > Please check consistency in capitalization of defined terms. The one > that stands out to me as inconsistent is “User Equipment” / “user > equipment”, but I’ve also noticed others, such as “enforcement > device”./User User Equipment fixed with commit https://github.com/capport-wg/architecture/commit/274b60a851ca4ef5e2b6f8ccb718ab9ac03d2119 The enforcement device language is inconsistently used, and is not such a quick fix. Opened issue #60 https://github.com/capport-wg/architecture/issues/60 > > Please be consistent about using “URI”, and not “URL”. Changed all URI to URL in commit https://github.com/capport-wg/architecture/commit/a9c87ba48aa64564bd9d0e1f21bd82906a2714f4 > > — Section 1 — > > This document standardizes an architecture for implementing captive > portals that provides tools for addressing most of those problems. > We are guided by these principles: > > The text that comes before this doesn’t identify anything as problems, > so I don’t see, from what’s written, what “those problems” refers to. > Maybe it would be better to say, “most of the problems that result > from current captive portal mechanisms, guided by these principles:” > or some such? Right; we removed an earlier reference and forgot to update this. See commit https://github.com/capport-wg/architecture/commit/1d91dff5a55b90b22dece05cde13caac97a71225 > > * Solutions MUST operate at the layer of Internet Protocol (IP) or > above, not being specific to any particular access technology > such > as Cable, WiFi or 3GPP. > > 3GPP is not an access technology; it’s an SDO (or a consortium of > SDOs). Maybe “mobile telecom”, or “LTE”, or “5G”? Or if you want to > retain 3GPP, maybe “protocols developed in 3GPP”? I think my > preference is “5G” (and it’s only an example anyway). I think "mobile telecom" suits the intent. See commit https://github.com/capport-wg/architecture/commit/e95238ccef15d874882ce0f4254b7529ebbd3f39 > > * End-user devices can be notified of captivity with Captive Portal > Signals in response to traffic. This notification should work > with any Internet protocol, not just clear-text HTTP. > > I’m not quite sure what this means. It sounds like it says that the > notification will be in-band with any Internet protocol, which clearly > isn’t possible. I think it’s talking about a separate protocol to > deliver the notification, which can work alongside any Internet > protocol, rather than having it be in-band to clear-text HTTP. Maybe > re-wording that sentence will help. I believe the intent was, "we are no longer relying on modifying clear-text HTTP." Because this is discussed elsewhere in the doc, I propose: * End-user devices can be notified of captivity with Captive Portal Signals in response to traffic. This notification works in response to any Internet protocol, and is not done by modifying protocols in-band. See commit https://github.com/capport-wg/architecture/commit/b42f9b5242f2d33d2dd163fd04aa5c372cd8442c > > — Section 1.2 — > > hosts (typically the internet). > > Nit: capitalize “Internet”. done > > Captive Portal API: Also known as API. An HTTP API allowing User > Equipment to query its state of captivity within the Captive Portal. > > This (and the Abstract) says “an HTTP API”, but there’s nothing at the > architecture level that assumes HTTP, is there? Instantiation may > well be via HTTP, but why does that need to be in the architecture? I > don’t see anything else in this document, apart from these two > definitions, binding the API to HTTP at the architecture level. Yes, the architecture intentionally says it is HTTP. No one argued for any alternatives, as far as I know. Details of the API are left unspecified, so it's still pretty open. > > If User Equipment supports the Captive Portal API, it MUST validate > API server TLS certificate (see [RFC2818]). > > Nit: “validate the API server’s TLS certificate” fixed > > — Section 2.3 — > > The API SHOULD provide evidence > to the caller that it supports the present architecture. > > I don’t understand what this means; can you explain? To me, this means that User Equipment can determine from the interaction that the API is implementing this architecture vs. being some random API. I imagine that a version number or content- type could achieve this. I've opened https://github.com/capport-wg/architecture/issues/61 to track the issue. > > — Section 2.4 — > > The Captive Portal Enforcement component restrict the network access > > Nit: “restricts” fixed > > * Allows traffic through for allowed User Equipment that has > > “Allows traffic through” reads oddly to me. Maybe “Allows traffic to > pass through”? Or “Allows general Internet traffic”? Changed to * Allows traffic to pass for User Equipment that is permitted to use the network and has satisfied the Captive Portal Conditions. https://github.com/capport-wg/architecture/commit/9da69e46eb77b348ee1cb1f45fe84b49a564268f > > — Section 2.6 — > In Figure 1, shouldn’t “Query captivity status” and “Portal user > interface” both have bidirectional arrows? I think the idea was to show which agent initiated. I'll add a status response. and bidirectional arrows to the UI. See commit https://github.com/capport-wg/architecture/commit/9f817d148353425c43678402350df2267db5db05 > > — Section 3 — > > these interactions, the components must be able to both identify the > user equipment from their interactions with it, and be able to agree > > Common problem with “both” and “either”: “be able” is already outside > the scope of “both”, so having it inside for the second case is wrong > (and the comma is unnecessary): > > NEW > these interactions, the components must be able to both identify the > User Equipment from their interactions with it and to agree > END Accepted. Commit https://github.com/capport-wg/architecture/commit/3a0396e9aca13cedef4b621aaea38b487944c80a > > — Section 3.2.1 — > > In order to uniquely identify the User Equipment, at most one user > equipment interacting with the other components of the Captive > Portal > MUST have a given value of the identifier. > > The way “MUST” is used here feels very odd. I think you mean the MUST > to be related to the uniqueness, not to what components MUST have. > Maybe something like this works better?: > > NEW > In order that User Equipment be uniquely identified, a given > identifier MUST NOT represent more than one User Equiment that > interacts with the other components of the Captive Portal. > END You're right; it's poorly worded. It's just trying to say each UE must have a unique ID at a point in time. The next point says they can be recycled. E.g., dynamic IP addresses could be used. I changed both bullets. NEW * Each instance of User Equipment interacting with the Captive Network MUST be given an identifier that is unique among User Equipment interacting at that time. * Over time, the User Equipment assigned to an identifier value MAY change. Allowing the identified device to change over time ensures that the space of possible identifying values need not be overly large. END Commit https://github.com/capport-wg/architecture/commit/290d24a648a6ed8b99300c0c2544f9e310d4f574 > > — Section 3.2.3 — > > Since the API Server will need to perform operations which rely on > the identity of the user equipment, such as query whether it is > captive, the API Server needs to be able to relate requests to the > User Equipment making the request. > > The API server doesn’t query whether the UE is captive, it responds to > such queries. Addressed in commit https://github.com/capport-wg/architecture/commit/8357b07dc309ddcfef8f5ad2dec3c29c89cffaf9 > > — Section 3.2.4 — > > The Enforcement Device will decide on a per packet basis whether it > should be permitted to communicate with the external network. > > Nit: hyphenate “per-packet”. > “It” should be referring to the packet, not to the ED, so “whether the > packet should”. Fixed > > — Section 3.3 — > Are we really talking about evaluating individual identifiers here? > Or does this really mean to discuss *methods* of generating or > choosing identifiers? I believe this is about choice of identifier in a solution/standard. Opened issue https://github.com/capport-wg/architecture/issues/62 > > — Section 3.4.3 — > Is this section talking about using a context-free URI as a UE > identifier? It should be clearer about that, if so (and if that’s not > what it’s about, the section is misplaced). There’s nothing in here > that discusses how such identifiers would meet the specified criteria. I think this is trying to say the server should not be looking at Ethernet addresses, for example, because the server is probably not on the same subnet as the User Equipment. So the info needs to be in the URL. I hope others can weigh in on this. Created issue https://github.com/capport-wg/architecture/issues/63 > > — Section 4 — > Please be consistent about the use of “workflow” vs “work-flow”. Fixed. > > — Appendix A — > > method is to attempt to make a HTTP request to a known, vendor > hosted > > Nit: hyphenate “vendor-hosted” > > DNS queries not using UDP > may potentially fail this test if operating over TCP or DNS over > HTTP. > > Nit: it’s “DNS over HTTPS”. But I would say, “DNS queries not using > UDP may potentially fail this test if operating over TCP or HTTPS.” Thanks. I made some other improvements to that paragraph too. As committed in https://github.com/capport-wg/architecture/commit/0a949c6ae536f4db390bfdfd5d795c64ab69783f, the two bullets are now: NEW * Another test that can be performed is a DNS lookup to a known address with an expected answer. If the answer differs from the expected answer, the equipment detects that a captive portal is present. DNS queries over TCP or HTTPS are less likely to be modified than DNS queries over UDP due to the complexity of implementation. * The different tests may produce different conclusions, varying by whether or not the implementation treats both TCP and UDP traffic, and by which types of DNS are intercepted. END Thanks again Barry. Great points! -Dave
- [Captive-portals] AD review of draft-ietf-capport… Barry Leiba
- Re: [Captive-portals] AD review of draft-ietf-cap… David Dolson
- Re: [Captive-portals] AD review of draft-ietf-cap… Barry Leiba
- Re: [Captive-portals] AD review of draft-ietf-cap… Tommy Pauly
- Re: [Captive-portals] AD review of draft-ietf-cap… Barry Leiba
- Re: [Captive-portals] AD review of draft-ietf-cap… Kyle Larose
- Re: [Captive-portals] AD review of draft-ietf-cap… Michael Richardson
- Re: [Captive-portals] AD review of draft-ietf-cap… Barry Leiba
- Re: [Captive-portals] AD review of draft-ietf-cap… Barry Leiba
- Re: [Captive-portals] AD review of draft-ietf-cap… Barry Leiba
- Re: [Captive-portals] AD review of draft-ietf-cap… Martin Thomson