Re: [Ace] Genart last call review of draft-ietf-ace-oauth-params-06

[Ludwig Seitz <[e-mail]>]

Hello Elwyn,

thank you for your review. Comments inline.

/Ludwig


On 2019-12-14 23:46, Elwyn Davies via Datatracker wrote:
> Reviewer: Elwyn Davies
> Review result: Not Ready
>
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
>
> For more information, please see the FAQ at
>
> <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.
>
> Document: draft-ietf-ace-oauth-params-06
> Reviewer: Elwyn Davies
> Review Date: 2019-12-14
> IETF LC End Date: 2019-12-13
> IESG Telechat date: Not scheduled for a telechat
>
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
>
> For more information, please see the FAQ at
>
> <">https://trac.ietf.org/trac/gen/wiki/GenArtfaq>;.
>
> Document: draft-ietf-ace-oauth-params-06
> Reviewer: Elwyn Davies
> Review Date: 2019/12/14
> IETF LC End Date: 2019/2/13
> IESG Telechat date: (if known) N/A
>
> Summary: Not ready.  In particular there seems to be some doubt as to whether
> the definitions in this document are actually stable - or alternatively that it
> lacks a versioning mechanism to cope with changes that the might be.
>
> Major issues:
> Dealing with possible updates to items defined here:
> In s1 the following appears:
>
>      These parameters and claims can also be used in other contexts, and may
>      need to be updated to align them with ongoing OAuth work. Therefore, these
>      parameters and claims have been put into a dedicated document, to
>      facilitate their use and any potential updates in a manner independent of
>      [I-D.ietf-ace-oauth-authz].
>
> I am unclear whether this implies that it is intended that these potential
> updates would alter the definitions here after they have been standardized or
> alternatively that the standardization of this document should be delayed until
> the alternative usage is defined.  If the first case applies, I do not see any
> versioning mechanism that would allow early implementations to cope with later
> updates of the items defined here.  In the second case, I have to ask myself
> why this document has been submitted for standardization before the usages have
> stabilized.
>

The intention with this document was to place certain parameters
(req_cnf, cnf, rs_cnf) needed for draft-ietf-ace-oauth-authz into a
separate document, that could be updated or obsoleted by other drafts
without obsoleting or updating draft-ietf-ace-oauth-authz.

This is mainly due to some work in the OAuth WG which has been running
in parallel, especially
https://tools.ietf.org/html/draft-ietf-oauth-pop-key-distribution.

Since this is not a high priority item for OAuth (the draft named above
has expired), the feeling in ACE was (@ACE please correct me if I
misrepresent) that going forward with this document was a good
compromise to get these parameters standardized, while causing minimal
breakage if OAuth at some later point finalized the pop key distribution
work.


> Minor issues:
> ss3.1, 3.2 and 4.1:  The COSE_Key type 'EC' used in several kty fields is not
> defined.  I assume it should be 'EC2'.
>
Correct, will fix

> ss3.1, 3.2 and 4.1:  Does it matter that the definitions of the x and y
> parameters in an EC2 key are given as 'h' encodings in RFC8152 but are given as
> 'b64' in this document?  I am very much not an expert here.

This is just a matter of the diagnostic representation used here for
human readability. On the wire this will be the same binary string.

Note that the hex encoding h'....' renders a much longer string than the
Base64 encoding b64'...' so I prefer b64 for readability.
>
> s6: This section starts with 'If CBOR is used...': The main ACE draft
> draft-ietf-ace-oauth-authz is apparently intended to cover both JSON and CBOR
> encodings of payloads, although CBOR is recommended.  This is not made explicit
> in this addition to that specification and the use of CBOR diagnostic
> representation and the prominence of COSE_Key items could make it appear up
> until s6 that this specification is intended just for CBOR encoding.  A few
> words at the beginning would clarify the dual alternatives.
>
I will add a clarification. The intention is to cover both JSON and CBOR.


> Nits/editorial comments:

Will fix.

> General: s/e.g./e.g.,/ (3 places)
>
> Abstract, 2nd sentence: s/whishes/wishes/
>
> Abstract: Need to expand AS and RS.
>
> s2:  RS, AS and (probably) various other terms are defined in RFC 6749 and need
> to be expanded on  first use.  Adding something like the para from
> draft-ietf-ace-oauth-authz would solve this (except for the abstract).
>
>      Terminology for entities in the architecture is defined in OAuth 2.0
>      [RFC6749] such as client (C), resource server (RS), and authorization
>      server (AS).s2, para 3: Need to expand CoAP on first use.
>
> s3:  This section needs a reference to RFC 8152 for the specification of the
> CWT COSE_Key item that is used extensively.
>
> s3/s4: Some introductory text for each section is desirable.
>
> s3.1, para 2 (definition of req_cnf):: Possibly add a note that the
> recommendation against symmetric keys implies currently kty being 'Symmetric'.
> Will it ever be anything else?
>
> s3.1:  The text in s3.2 of draft-ietf-ace-cwt-proof-of-possession-03 contans
> the following
>
>     The COSE_Key MUST contain the required key members for a COSE_Key of that
>     key type and MAY contain other COSE_Key members, including the "kid" (Key
>     ID) member.
>
>     The "COSE_Key" member MAY also be used for a COSE_Key representing a
>     symmetric key, provided that the CWT is encrypted so that the key is not
>     revealed to unintended parties. The means of encrypting a CWT is explained
>     in [RFC8392]. If the CWT is not encrypted, the symmetric key MUST be
>     encrypted as described in Section 3.3.
>
> These riders probably apply to all the subsectons of s3 and to s4.1 and could
> be included in the currently empty main section text.
>
> s4.1, rs_cnf - DTLS-RPK: This term needs a reference (RFC 7250). Also all other
> uses do not hyphenate and RPK needs to be expanded.
>     s/DTLS-RPK handshake/DTLS Raw Public Key (RPK) handshake [RFC7250]/
>
>
> _______________________________________________
> Ace mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/ace
>