Hi Kyle, Thanks for the review! I’ve made some editorial fixes in this commit:
https://github.com/capport-wg/api/commit/fabc5994e3d7945140dd379a8b81a28b5b668bb4 > On Mar 14, 2020, at 9:21 AM, Kyle Larose <kyle=40agilicus....@dmarc.ietf.org> > wrote: > > Hello, > > I support publication of capport-api. Thanks for all the hard work! > > I have a bunch of minor comments: > > Regarding Section 4.1.1: > > >> If the certificates do >> require the use of AIA, the captive network will need to allow client >> access to the host specified in the URI. > > Should this be phrased as "... the captive network MUST allow client > access..." Without this the solution won't work in this situation, so > isn't a strong requirement necessary? Agreed, changed the wording as suggested. > > Regarding this section as a whole, we are placing some requirements on > the enforcement device here... Do you think > those should be repeated in the architecture document so those > implementing only enforcement devices can consider them? Filed https://github.com/capport-wg/architecture/issues/50 > > Regarding Section 4.2: > > >> "bytes-remaining" (optional, integer): indicates the number of >> bytes remaining, after which the client will be in placed into a >> captive state. The byte count represents the total number of IP >> packet (layer 3) bytes sent and received by the client. Captive >> portal systems might not count traffic to whitelisted servers, >> such as the API server, but clients cannot rely on such behavior. > > We say that the "seconds-remaining" field should be omitted if > `captive=false`. We do > not for "bytes-remaining". That feels inconsistent. I think we should > add the following > statement to the end of this field's definition: > > Added text: > " > The API server SHOULD include this value if the client is not captive > (i.e. captive=false) > and SHOULD omit this value for captive clients. > " Added clarification as suggested. > > That brings me to another point. What should the API reply with if > there are no time or byte constraints > on the session? Should the fields be omitted in that case? The > document does not discuss that. If we do > decide to add that, do we need discussion about what the client > can/cannot infer from omission of these fields (i.e. > if bytes-remaining is not set, then there is no byte quota for the > connection)? I added some clarification that these fields only SHOULD be included when the client session is indeed limited in the fashion described. > > Regarding the byte-count: we say it represents the total number of > tx/rx packets. I think it's obvious that this means > the sum of the total of each, but maybe it'd be better to make that > explicit. New text: > > "The byte count represents the sum of the total number of IP packet > (layer 3) bytes sent and received by the client." Fair clarification! Added as suggested. > > On that note, do most captive portal vendors count this way, or is > there often a distinction between tx/rx that we may > want to account for? Or is that complicating things too much, since > this is more a hint than anything else? This is a strategy that some portals use at least, based on previous discussion. More complex strategies (which are harder to describe in the API, and harder for the client to be able to estimate) could always be added later. > > Finally, do we need to version the API (i.e. add a version field)? I'm > not sure what the best practice for IETF-defined HTTP APIs is. My impression is that we don’t need any version field at the moment. We always could add a new field that would indicate a re-interpretation of fields, etc. > > Regarding section 6.2: > >> Type: The type of the JSON value to be stored, as one of the value >> types defined in [RFC8259]. > > I don't think the types given in section 4.2 (the existing key > registry) exactly follow RFC8259. > In particular, whether or not a field is required/optional is not part > of its type in JSON. > Should we mention what the type field in the registry should include that? > > E.g.: > >> Type: The type of the JSON value to be stored, as one of the value >> types defined in [RFC8259], along with whether it is optional or required. > > Or, add a new field: > >> Required: Whether or not the key is required in the document. > > I guess it's unlikely that we'd add a new required field since the API > isn't versioned right now, but we should probably be explicit and > consistent > about it. To note, we already did have the following comment: “All keys other than the ones defined in this document as "required" will be considered optional.” I think the main issue was that the list was one list with the (requirement, type) tuple being listed together. I rearranged the text into two lists: a list of one entry for the only required field, and a list following of the optional fields. I think this should address your comment. Best, Tommy > > Thanks, > Kyle > > On Thu, 5 Mar 2020 at 01:56, Martin Thomson <m...@lowentropy.net> wrote: >> >> Hi folks, >> >> Our fine editor teams have contributed updates to these drafts. >> >> https://tools.ietf.org/html/draft-ietf-capport-architecture-06 >> https://tools.ietf.org/html/draft-ietf-capport-api-05 >> >> This starts a joint working group last call on these documents. Please >> respond this mail with your views regarding the suitability of these >> documents for publication (as Informational RFC and Proposed Standard RFC >> respectively) before 2020-03-23. >> >> There are a few minor issues, but I consider those to be minor enough to >> require only trivial fixes. Some appear to be already addressed. If you >> think that major changes are needed, or have proposed resolutions to issues, >> adding those to your email would be helpful. >> >> Issues are tracked here: >> https://github.com/capport-wg/architecture/issues >> https://github.com/capport-wg/api/issues >> >> I encourage people to add issues to the tracker as they review these >> documents. Directly raising minor editorial issues on GitHub will help us >> focus attention on substantive issues here. >> >> Cheers, >> Martin (and Erik) >> >> _______________________________________________ >> Captive-portals mailing list >> Captive-portals@ietf.org >> https://www.ietf.org/mailman/listinfo/captive-portals > > _______________________________________________ > Captive-portals mailing list > Captive-portals@ietf.org > https://www.ietf.org/mailman/listinfo/captive-portals _______________________________________________ Captive-portals mailing list Captive-portals@ietf.org https://www.ietf.org/mailman/listinfo/captive-portals
