Re: [jose] [apps-discuss] Appsdir review for draft-ietf-jose-json-web-algorithms-33

[Carsten Bormann <[e-mail]>]

Here is a quick re-check of my review against -34.
I’m not sure any of the necessary fixes made it into -34.

Grüße, Carsten

On 02 Oct 2014, at 09:22, Carsten Bormann <[e-mail]> wrote:

> I have been selected as the Applications Area Directorate reviewer for
> this draft (for background on appsdir, please see
> ​http://trac.tools.ietf.org/area/app/trac/wiki/ApplicationsAreaDirectorate
> ).
> 
> Please resolve these comments along with any other Last Call comments
> you may receive. Please wait for direction from your document shepherd
> or AD before posting a new version of the draft.
> 
> Document:  draft-ietf-jose-json-web-algorithms-33
> Title: JSON Web Algorithms (JWA)
> Reviewer: Carsten Bormann
> Review Date: 2014-10-02
> IESG Telechat date: 2014-10-02
> 
> Summary: This draft is ready for publication as a standards track RFC,
> with a few nits corrected.
> 
> However, some additional editorial improvements might improve the
> security outcome when it is referenced by application developers.
> 
> Major issues: None.
> 
> Minor issues:
> 
> 5.2:
> Add a reference that defines PKCS #7 padding.

No change.
(Note that there is a reference behind “PKCS #7 padding”, it just happens to define CBC and not PKCS #7).

> 5.2.2.2
> Does "the PKCS #7 padding is removed" entail checking all of its bytes?

No change.

> 6.2.1
> Is the intention that the sentence containing "point compression is not
> supported" also applies to any future registered value of "crv"?
> A similar comment applies to other specifications in 6.2.1.x, e.g.,
> the reference to SEC1 representation to x and y.

No change.

> 6.2.1.1
> »Additional "crv" values MAY be
> used, provided they are understood by implementations using that
> Elliptic Curve key.«
> How are conflicts between such implementation defined values and
> future registered values handled?

No change.

And so on.

> 6.3.2:
> The MAY accept partially overrides the MUST include?
> Is the latter thus really a SHOULD?
> 
> 7.1:
> It is interesting that a mere registration (vetted only by a DE) can
> change the IETF consensus base specifications by making an algorithm
> "Required".
> 
> 8.
> I am unable to find a "security considerations" section in NIST SP 800-38A.
> 800-38D at least has a "practical considerations" section, is that meant?
> (Etc., I haven't checked all the references.)
> In general, I believe a security considerations section is most useful
> where it provides more directed guidance instead of saying the
> equivalent of "here is a textbook".
> 
> 8.7 is not clear: is it NOT RECOMMENDED to reuse an entire set of key
> material (including IV), or to reuse any part of it?
> 
> 
> Nits/editorial comments:
> 
> 6.3.2.x:
> The constant repetition of »It is represented as the base64url encoding of
> the value's unsigned big endian representation as an octet sequence.
> The octet sequence MUST utilize the minimum number of octets to
> represent the value.« almost ensures that an implementer will stop
> reading the details (well, I did, and I did not write a program to
> verify the same phrase is used everywhere; if any parameter were using a
> different encoding, that sure would be missed).  Why not define
> another abstraction like base64url and use this?
> 
> 6.2.3.1: This is not a positive integer?  6.2.3.x mentions this otherwise.
> 
> 7.1.1
> »Example description« is not a useful example for an "Algorithm Description".
> (Same comment for 7.x.1.)
> 
> 8.3:
> s/because it/because it is/
> 
> [sec1]
> (Given the date, this is probably referencing V2.0 of this spec.)
> 
> [usascii]
> The reference to ANSI X3.4:1986 should probably be replaced by a
> reference to RFC 20.  There is little reason to reference a somewhat
> hard to obtain external document ($60!) when we have an RFC about the
> same subject.
> 
> (Tables in Appendix A need some formatting.)
> 
> _______________________________________________
> apps-discuss mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/apps-discuss
> 
>