Re: [CDNi] AD review of draft-ietf-cdni-uri-signing-19

[Phil Sorber <[e-mail]>]

Barry,

Thanks again for the review and sorry about the amount of time taken to
address your issues. I have opened a PR on the git repo for this draft
here: https://github.com/PSUdaemon/URISigningSpec/pull/65

Let's iterate there and I will submit an updated draft once you are
satisfied with the changes.

A couple specific comments below.

Thanks.

On Thu, Mar 12, 2020 at 11:05 PM Barry Leiba <[e-mail]>
wrote:

> Here is my AD review of draft-ietf-cdni-uri-signing-19.  My apologies for
> taking a couple of weeks to get to this: the IESG has, as you can imagine,
> been swamped with both heavy telechat schedules because of the impending AD
> changeover and unexpected craziness with IETF 107 planning and decisions
> related to the meeting cancellation.
>
> Kevin, thanks so much for a very thorough and excellent shepherd writeup!
>
> A couple of these comments are substantive, so I would like to discuss and
> resolve them before last call.  I’m going to set the draft’s substate to
> “revised I-D needed”.
>
> Please expand “CDNI” in the abstract... and possibly in the title as well,
> though you could wait for the RPC to do that if you prefer.  Please also
> fully expand “CDN” on its first use in the introduction.
>
> Please replace “UA” in the abstract with “user agent”.
>
>    This document uses the terminology defined in the CDNI Problem
>    Statement [RFC6707].
>
> I believe that terminology definitions are normative references, so I
> believe this makes 6707 normative.  Happily, normative downrefs such as
> this are accepted now, so please make the reference normative and I will
> make sure it’s called out in the last call notice.
>
> — Section 1.3 —
>
>    With symmetric keys, the same key is
>    used by both the signing entity for signing the URI as well as by the
>    verifying entity for verifying the Signed URI.
>
> Nit: Think of “both” as an open parenthesis (and the end of he sentence as
> the closing one here).  The first “by” is outside the parens (before
> “both”), but the second is inside, and that’s not parallel.  Make it either
> “by both the signing entity [...] and the verifying entity” or “both by the
> signing entity [...] and by the verifying entity”.
>
>    distributed openly (because they cannot be used to sign URIs) and
>    private keys are kept secret by the URI signer.
>
> I would say “and the corresponding private keys”.
>
> — Section 1.4 —
>
>    While the URI signing method defined in this document was primarily
>    created for the purpose of allowing URI Signing in CDNI scenarios,
>    i.e., between a uCDN and a dCDN, there is nothing in the defined URI
>    Signing method that precludes it from being used in a non-CDNI
>
> As you see here, you capitalize “URI Signing” inconsistently,even in this
> one paragraph.  Please check the document for consistency in the
> capitalization: pick one and use it throughout.
>
> — Section 2 —
>
>    HTTP or HTTPS URI [RFC7230] (Section 2.7).
>
> This is a quirk of the tools and not a fault in the draft, but this
> results in the html version having a clickable link to RFC 7230, and then
> another clickable link to Section 2.7 of this document (which, of course,
> isn’t there).  I think if you change this to “HTTP or HTTPS URI (see
> [RFC7230] Section 2.7)” the generated link will work better.
>

I made this change but not sure if it had the intended effect. Please
confirm.


>
>    o  a reserved character (as defined in [RFC3986] Section 2.2),
>
> Any reserved character?  And the three reserved characters in bullets 1,
> 3, and 5 can be any three different ones?  I think I know what you mean,
> but it seems that an implementer who doesn’t could go quite wrong.  Maybe
> some more words along with these bullets would be better?
>
> [For example, in the URI <
> http://example.com/content?sign=jwtdatahere&other >, I think that if I
> strictly implement what you have here I would interpret “example.com/” as
> the attribute name (because “/“ is a reserved character), “content” as the
> JWT package, and “?” as the terminating special character.  But you’re
> looking for something more like “sign” as the attribute name and
> “jwtdatahere” as the JWT package, yes?]
>
> Also, I don’t see any examples in the draft of a Signed URI, and I think
> an example or two here of a couple of variations would also help.
>
> — Section 2.1 —
> Several of the subsections that deal with time values repeat this text
> exactly:
>
>    Note: The time
>    on the entities that generate and verify the signed URI SHOULD be in
>    sync.  In the CDNI case, this means that CSP, uCDN, and dCDN servers
>    need to be time-synchronized.  It is RECOMMENDED to use NTP [RFC5905]
>    for time synchronization.
>
> I suggest cleaning this up by moving that note up here to the end of
> Section 2.1, saying, “Note: When claims related to time are used, the time
> on the entities...”
>
> — Section 2.1.7 —
>
>    previously seen the nonce used in a request for the same content
>
> This is the only spot where you don’t capitalize “Nonce”.
>
> — Section 2.1.9 —
>
>    This claim MUST be understood and processed by
>    all implementations.
>
> Section 2.1 already said this about all claims, so why say it here about
> this one in particular?
>

The part in Section 2.1 should have been removed. I made that change
instead.


>
> — Section 2.1.10 —
>
>    Client IP (cdniip) [optional] - The Client IP (cdniip) claim hold an
>
> Nit: “holds”
>
> — Section 2.1.15 —
>
>    The URI Container (cdniuc) claim takes one of the following forms
>
> This is the only claim definition that doesn’t say “[optional]”.  Should
> it?
>

I see it as optional in the latest published version of the draft already.
Was there another claim you were meaning instead?


>
> — Section 3.2 —
>
>    the following section defines a mechanism using HTTP Cookies
>
> Please use a specific section xref instead.
>
> — Section 6.7 —
> Thanks for including advice to the experts.
>
>    Expert Reviewers should be empowered to pass judgements
>    as they see fit
>
> This is very vague and, to me, implies more capriciousness than I think
> you really intend.  Can you come up with another way to word this that
> gives a bit more guidance?  What sorts of judgments are they likely to make?
>

This one I agree with you on, but I struggled in adding it. I made some
changes, but more guidance is welcome.


>
> — Section 7 —
>
>       seconds to prevent a sudden network issues from denying legitimate
>
> Nit: either remove “a” or make it “issue”.
>
> —
> Barry
>
> _______________________________________________
> CDNi mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/cdni
>