Subject: [Captive-portals] AD review of draft-ietf-capport-architecture-07

From: Barry Leiba <[email-masked]>
Date: Fri, 24 Apr 2020 19:35:49 -0400

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.

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

Please be consistent about using “URI”, and not “URL”.

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

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

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

— Section 1.2 —

   hosts (typically the internet).

Nit: capitalize “Internet”.

   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.

   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”

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

— Section 2.4 —

   The Captive Portal Enforcement component restrict the network access

Nit: “restricts”

   *  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”?

— Section 2.6 —
In Figure 1, shouldn’t “Query captivity status” and “Portal user
interface” both have bidirectional arrows?

— 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

— 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

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

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

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

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

— Section 4 —
Please be consistent about the use of “workflow” vs “work-flow”.

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

-- 
Barry

---

From: David Dolson <[email-masked]>
Date: Sun, 26 Apr 2020 23:39:29 -0400

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

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

> 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

> from current captive portal mechanisms, guided by these principles:”
Right; we removed an earlier reference and forgot to update this.
See commit 
https://github.com/capport-wg/architecture/commit/1d91dff5a55b90b22dece05cde13caac97a71225

> 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

> 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

> Nit: capitalize “Internet”.
done

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

> Nit: “validate the API server’s TLS certificate”
fixed

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

>    The Captive Portal Enforcement component restrict the network access
fixed

> 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

> 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

>    User Equipment from their interactions with it and to agree
Accepted. Commit 
https://github.com/capport-wg/architecture/commit/3a0396e9aca13cedef4b621aaea38b487944c80a

>    interacts with the other components of the Captive Portal.
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

> The API server doesn’t query whether the UE is captive, it responds to
Addressed in commit 
https://github.com/capport-wg/architecture/commit/8357b07dc309ddcfef8f5ad2dec3c29c89cffaf9

> “It” should be referring to the packet, not to the ED, so “whether the
Fixed

> Or does this really mean to discuss *methods* of generating or

I believe this is about choice of identifier in a solution/standard.
Opened issue https://github.com/capport-wg/architecture/issues/62

> 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

> Please be consistent about the use of “workflow” vs “work-flow”.
Fixed.

> 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

---

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

Thanks for the reply, Dave, and I think we're OK to start last call on
the document after you post a revised I-D with the changes so far --
most unresolved things are not at a blocking level.  Just one thing

> https://github.com/capport-wg/architecture/commit/a9c87ba48aa64564bd9d0e1f21bd82906a2714f4

But that's backward: the IETF formally defines "URI" [RFC3986], not
"URL", and that document says [Section 1.1.3]:

   Future specifications and related documentation should
   use the general term "URI" rather than the more restrictive terms
   "URL" and "URN"

(Additionally, we should probably include an informative reference to
3986 on first use of the term.)

If the working group really wants "URL" here I won't block it further,
but I would strongly prefer that we use "URI", consistent with IETF
usage.

---

Collecting the other points that aren't resolved, but that need not
block last call:

> intended.  We've allowed the user equipment to participate or not in various ways.

OK, then no more to do here.  This was mostly just a note that I
expect to see comments, and not an expectation of any changes to the
document.  Thanks for checking.

> I've opened https://github.com/capport-wg/architecture/issues/61

OK.  A small wording tweak will be good at some point, but let's see
if anyone else raises this point in reviews.

> Opened issue https://github.com/capport-wg/architecture/issues/62

Great; again, a small wording tweak will do it.

> > — Section 3.4.3 —
> > Is this section talking about using a context-free URI as a UEe

To be clear here, I'm not concerned about the content of the section
as such, only about how it fits into the topic of identifier
selection.  So I think it's just a matter of clarifying that, and it
should be easy to deal with by the time last call ends.

Barry

---

From: Tommy Pauly <[email-masked]>
Date: Mon, 27 Apr 2020 16:21:52 -0700

> On Apr 27, 2020, at 7:21 AM, Barry Leiba <[email-masked]> wrote:
> 

FWIW, the API document uses “URI” everywhere other than in its JSON keys. I’d agree that the architecture document should use URI as well.

Thanks,
Tommy
> _______________________________________________
> Captive-portals mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/captive-portals