From: Mike Jones <Michael.Jones@microsoft.com>
To: Carsten Bormann <cabo@tzi.org>
Thread-Topic: [apps-discuss] Appsdir review for
 draft-ietf-jose-json-web-algorithms-33
Thread-Index: Ac/oVSxcdPW/TNiPTRiX1p7gtScCIgACOMIAAD/dNXA=
Date: Thu, 16 Oct 2014 16:52:37 +0000
Message-ID: <4E1F6AAD24975D4BA5B16804296739439BB137D2@TK5EX14MBXC286.redmond.corp.microsoft.com>
References: <4E1F6AAD24975D4BA5B16804296739439BB0FB8B@TK5EX14MBXC286.redmond.corp.microsoft.com>
 <AD74AEDE-AC3E-46D9-A6C7-99B009548D26@tzi.org>
In-Reply-To: <AD74AEDE-AC3E-46D9-A6C7-99B009548D26@tzi.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Received-SPF: Pass (protection.outlook.com: domain of microsoft.com designates
 131.107.125.37 as permitted sender)
 receiver=protection.outlook.com; 
 client-ip=131.107.125.37; helo=mail.microsoft.com;
Archived-At: http://mailarchive.ietf.org/arch/msg/jose/rFd2mV0HNkeCIk0eygzUDG0lKeE
Cc: "draft-ietf-jose-json-web-algorithms.all@tools.ietf.org"
 <draft-ietf-jose-json-web-algorithms.all@tools.ietf.org>,
 "jose@ietf.org" <jose@ietf.org>, "iesg@ietf.org" <iesg@ietf.org>,
 "apps-discuss@ietf.org" <apps-discuss@ietf.org>,
 Matt Miller <mamille2@cisco.com>
Subject: Re: [jose] [apps-discuss] Appsdir review for
 draft-ietf-jose-json-web-algorithms-33
Precedence: list

Replies to your replies are inline below.  Thanks again for your thoughtful=
 and detailed review.

				-- Mike

> -----Original Message-----
> From: Carsten Bormann [mailto:cabo@tzi.org]
> Sent: Wednesday, October 15, 2014 2:55 AM
> To: Mike Jones
> Cc: apps-discuss@ietf.org; draft-ietf-jose-json-web-
> algorithms.all@tools.ietf.org; Matt Miller; iesg@ietf.org; jose@ietf.org
> Subject: Re: [apps-discuss] Appsdir review for draft-ietf-jose-json-web-
> algorithms-33
>=20
> Hi Mike,
>=20
> thanks for getting to this review - I don't envy you for this herculean j=
ob.
> A few more comments/clarifications inline below.
>=20
> Gr=FC=DFe, Carsten
>=20
> On 15 Oct 2014, at 10:51, Mike Jones <Michael.Jones@microsoft.com> wrote:
>=20
> > Thanks for your review, Carsten.  I apologize for not responding to it =
until
> now.  It had gotten sorted into a folder and I wasn't aware of it until K=
athleen
> brought it to my attention.  Replies are inline below...
> >
> >> -----Original Message-----
> >> From: apps-discuss [mailto:apps-discuss-bounces@ietf.org] On Behalf
> >> Of Carsten Bormann
> >> Sent: Thursday, October 02, 2014 12:22 AM
> >> To: apps-discuss@ietf.org; draft-ietf-jose-json-web-
> >> algorithms.all@tools.ietf.org
> >> Cc: iesg@ietf.org; jose@ietf.org
> >> Subject: [apps-discuss] Appsdir review for
> >> draft-ietf-jose-json-web-algorithms-
> >> 33
> >>
> >> 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/ApplicationsAreaDirecto
> >> rate
> >> ).
> >>
> >> 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.
> >
> > I'll plan on adding a reference to RFC 2315 for this.
>=20
> That would work (section reference would be 10.3) Alternatively, you coul=
d
> reference RFC 5652 (section 6.3), which is an Internet Standard (STD70).
>=20
> >> 5.2.2.2
> >> Does "the PKCS #7 padding is removed" entail checking all of its bytes=
?
> >
> > Should this be changed to "the PKCS #7 padding bytes are checked and th=
en
> removed"?
>=20
> I think it is good to alert the reader to this need.
>=20
> >> 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.
> >
> > It would apply to any future curves registered that use the "x", "y" po=
int
> representation.  Others could define new key types that use a different
> representation or we could refine the definition of "kty":"EC" to make th=
e
> representation specific to the "crv" value.
> >
> > A discussion happened on the JOSE thread "JWK Elliptic Curve key
> representations and new curves".  The conclusion suggested by Richard Bar=
nes
> and seconded by Stephen Farrell was to table defining any new key
> representations for new elliptic curves until the CRFG recommendations to=
 TLS
> have been made.  That ended the discussion for now.
>=20
> Right, I didn't want to anticipate that discussion; I would be happy if i=
t didn't
> appear that the avenue for any evolution on point compression/compact poi=
nts
> were closed.

I can add language saying that future curve registrations may use different=
 parameters to represent the curve value.  That may get a little convoluted=
 because currently it's the "kty" that determines which parameters are used=
.  But I agree with trying to accommodate new key representations for new c=
urves.

> >> 6.2.1.1
> >> =BBAdditional "crv" values MAY be
> >> used, provided they are understood by implementations using that
> >> Elliptic Curve key.=AB How are conflicts between such implementation
> >> defined values and future registered values handled?
> >
> > Conflicts are avoided using the IANA JSON Web Key Elliptic Curve regist=
ry
> defined in Section 7.6.
>=20
> Oh, but that's not what the text says.  I read this as a general license =
to use
> random locally defined crv values.  Maybe the need for registration needs=
 to be
> clarified.

I'll delete the sentence "Additional 'crv' values MAY be used, provided the=
y are understood by implementations using that Elliptic Curve key."  The re=
gistry is already talked about in the previous sentences.

> >> 6.3.2:
> >> The MAY accept partially overrides the MUST include?
> >> Is the latter thus really a SHOULD?
> >
> > "d" is REQUIRED.  The others SHOULD be included, and if any are include=
d, the
> others MUST be included.  Those are requirements on producers.
> >
> > Consumers may choose to accept keys that don't include all the CRT
> parameters.  Given that they can be computed from "n", "e", and "d" and
> sometimes they're not available, a previous reviewer had asked this langu=
age
> about consumers to be included.  It's a case of "Be conservative in what =
you
> send, be liberal in what you accept".
>=20
> [I'm not inserting my usual rant here that this bon mot may apply to
> implementations, but never to specifications.]
>=20
> > It's not clear to me that there's any value in relaxing the requirement=
s on
> producers.
>=20
> Well, if the producer MUST send "all of the others" if "any of the other"
> parameters are present, it is not clear to me what the point of "MAY choo=
se
>    to accept an RSA private key that does not contain a complete set of
>    the private key parameters other than "d"" is other than enabling prod=
ucers to
> send just that.
> (More importantly, it is not clear when a producer should be taking that =
license.)

I can delete the sentence " The consumer of a JWK MAY choose to accept an R=
SA private key that does not contain a complete set of the private key para=
meters..."

> >> 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".
> >
> > It is expected that the designated experts will be appointed, in part, =
for their
> cryptographic expertise.
>=20
> There has been more discussion about the requirements levels here that I =
don't
> want to repeat.
> My comment here was that a DE (even with the best crypto knowledge) is in=
 a
> hard place updating the consensus about *desirability of implementation* =
-
> there are more aspects to this than the technical ones.

Thinking about this some more, I agree that "Required" is a special case.  =
I'm thinking that a reasonable safeguard for this case is to require that a=
pproval of both a designated expert and a sitting security area director be=
 required to make an algorithm "Required" or to change it from "Required" t=
o something else.  Kathleen and Carsten, would that work for you?

> >> 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 mea=
nt?
> >> (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 textb=
ook".
> >
> > There are 14 subsections with directed guidance, plus more in the JWS, =
JWE,
> and JWK specifications.  If there is other directed guidance you'd like t=
o see
> added, please supply proposed text.
> >
> > Having skimmed through it, I agree that 800-30A doesn't contain clear e=
nough
> security guidance to merit inclusion in the list.  I'll remove it.  If th=
ere are other
> specifications that you'd like to see added or removed, please let me kno=
w.
>=20
> This was more of a general comment that "Here is a reference to a textboo=
k"
> type security considerations are less useful than specific, directed refe=
rences.  I
> haven't checked all those referenced documents, so I'm not in a position =
to
> recommend changes here.  Maybe something for a checklist to apply to the =
next
> security spec.
>=20
> >
> >> 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?
> >
> > I agree that this is ambiguous, as worded.  Its advice seems to be so b=
road as
> to be impractical, as never reusing a key means that a new key would have=
 to be
> redistributed for each encryption, which then requires a key to use for t=
he
> distribution...  This text came from http://tools.ietf.org/html/draft-mil=
ler-jose-
> jwe-protected-jwk-02#section-8.1, which was incorporated into the documen=
t
> after a working group decision to do so.  Matt Miller, you were the autho=
r of
> this text.  Could you clarify what the intent was?  Thanks.
> >
> >> Nits/editorial comments:
> >>
> >> 6.3.2.x:
> >> The constant repetition of =BBIt 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.=AB 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 defin=
e
> another abstraction like base64url and use this?
> >
> > I did just verify that they are all consistent.  Yes, if you're reading=
 the spec
> linearly I'm sure it feels repetitive, but if you're an implementer going=
 straight to
> a definition to implement it, having a complete description all in one pl=
ace is
> valuable.  I understand your request but it's not clear to me that this r=
ises to the
> level that a separate definition that the developer then has to go find i=
s
> warranted.
>=20
> My point was corroborated by the tiny error Richard found.  Repeating the=
 same
> information again and again drowns out the important information.  A sing=
le
> paragraph at the end of 6.3's intro of the form:
>=20
>    Each of the positive integer parameters defined in this section
>    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.

OK - I'll see what I can do in this regard.  Editorially, it's a little bit=
 complicated because there are different subsections for public and private=
 key parameters.

> Note that the repetitive text already has a reference that needs to be lo=
oked up
> in that it uses the term "base64url encoding", which has a slightly refin=
ed
> meaning here from that in RFC 4648.  So an even better way to handle this
> would be to add a new term "posint encoding" or some such to section 1.1,
> turning e.g. the entirety of 6.3.2.2 into
>=20
>            The "p" (first prime factor) member contains the first prime f=
actor, in
> POSINT encoding.
>=20
>=20
> >
> >> 6.2.3.1: This is not a positive integer?  6.2.3.x mentions this otherw=
ise.
> >
> > I may not understand this remark.  I'm guessing you're referring to 6.3=
.2.1 and
> the fact that the description includes the word "unsigned".  This is ther=
e to make
> it clear that no sign bit will be present in the representation - which i=
s especially
> important since the high-order bit is always set for "n" and "d" for corr=
ectly
> generated keys.
>=20
> (Sorry, this is indeed about 6.3.2.x, not 6.2.3.x.) That was not what I m=
eant:  All
> 6.3.2.x have the form =BBThe "p" (first prime factor) member contains the=
 first
> prime factor, a positive integer.=AB
> 6.3.2.1 does not have the ", a positive integer", but I can't see a reaso=
n for that
> (d clearly cannot be zero, and it is still an unsigned value).
> This is likely to confuse an implementer that reads this text closely.
> (And I'm offering the fact that you are overlooking that inconsistency as=
 another
> corroboration of my previous point.)

I can delete the unnecessary phrase "a positive integer", subject to how it=
 does or doesn't fit into the edit above that you asked for.

> >> 7.1.1
> >> =BBExample description=AB is not a useful example for an "Algorithm De=
scription".
> >> (Same comment for 7.x.1.)
> >
> > See the actual registrations in Section 7.1.2 for more useful examples.
>=20
> Maybe just take out the not so useful examples in 7.x.1?

All other registration templates that I have seen include examples of all p=
arameters.  This is only one of the parameters in the template, and all nee=
d to be retained.  Would you be happier with the e.g. text "Example algorit=
hm"?

> >> 8.3:
> >> s/because it/because it is/
> >
> > Good catch
> >
> >> [sec1]
> >> (Given the date, this is probably referencing V2.0 of this spec.)
> >
> > Hmmm - the version I have cached locally came from
> http://www.secg.org/collateral/sec1_final.pdf and is dated September 20,
> 2000.  But the site appears to be down at present.  The version reference=
d by
> Wikipedia at http://www.secg.org/download/aid-385/sec1_final.pdf is missi=
ng
> too. http://secg.org/download/aid-780/sec1-v2.pdf is missing too.  Can yo=
u
> point me to a good link?
>=20
> I just had a look and embarrassingly found that the copy I'm using comes =
from
> the personal space of a researcher:
> http://perso.univ-rennes1.fr/sylvain.duquesne/master/standards/sec1-v2.pd=
f
> Not very satisfying as a reference.
>=20
> >
> > RFC 5915 includes this reference:
> >
> >   [SECG1]    Standards for Efficient Cryptography Group (SECG), "SEC 1:
> >              Elliptic Curve Cryptography", Version 2.0, May 2009.
>=20
> That looks good: We don't need a URL, as long as we are certain which ver=
sion
> we are referencing.
>=20
> > Worst comes to worst, I'll either reference that or explicitly referenc=
e the 1.0
> version.  But if I'm going to reference the 2.0 version, I'd like to be a=
ble to read a
> copy so that I can verify that the way we're using it is consistent with =
the actual
> spec, including the section numbers.
> >
> >> [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.
> >
> > OK
> >
> >> (Tables in Appendix A need some formatting.)
> >
> > Agreed.  It's on my to-do list.  Jim Schaad recently told me to "Add a =
width
> attribute to the ttcol with either "digits" (for fixed width) or "digits%=
" (for
> percent width) to the column in question".  I plan to give it a try.
> >
> >> _______________________________________________
> >> apps-discuss mailing list
> >> apps-discuss@ietf.org
> >> https://www.ietf.org/mailman/listinfo/apps-discuss
> >
> > 				Thanks again,
> > 				-- Mike
> >
> >
> >