Subject: [secdir] Review of draft-merkle-tls-brainpool-03

From: Simon Josefsson <[email-masked]>
Date: Fri, 5 Jul 2013 00:42:18 +0200

I have reviewed draft-merkle-tls-brainpool-03 and consider the document
to be "Ready with nits".  I support its publication.

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the
IESG.  These comments were written primarily for the benefit of the
security area directors.  Document editors and WG chairs should treat
these comments just like any other last call comments.

I haven't verified the test vectors, but as an implementer I'm happy
that they are present and they improve the credibility of the draft.

I believe the document would be improved by addressing the suggestions
below, but these comments are not critical.

1)

When I read the document, it seems to be missing its "gut".  There is
one section "Introduction" and then you would expect the actual
specification.  But instead comes the Security Considerations and the
rest of the usual IETF boiler plate.  The contribution of this document
is hidden in the IANA Considerations.

In particular, there is no TLS presentation language of the new fields.

Adding new TLS enum types is done by several other documents, and they
usually contain a bit more detail.  Compare how RFC5878 introduces new
enum types in section 2.  For an alternative approach, look at how
rfc6042 introduces new enum types.

Further, I feel it is more appropriate to put the comment about DTLS
compatibility in this new section rather than in the IANA
Considerations.

I would propose to add a new section after "Introduction":

--->>>
2. Brainpool NamedCurve Types

This document adds three new NamedCurve types as follows.

        enum {
	     brainpoolP256r1(TBD1),
	     brainpoolP384r1(TBD2),
	     brainpoolP512r1(TBD3)
        } NamedCurve;

These curves are suitable for use with DTLS [RFC6347].
<<<---

2)

In section 1, remove a whitespace after the RFC5480 citation.  It
causes a comma to appear standalone.

OLD:
   certificates according to [RFC3279] and [RFC5480] , their negotiation
NEW:
   certificates according to [RFC3279] and [RFC5480], their negotiation

/Simon

---

From: Johannes Merkle <[email-masked]>
Date: Fri, 05 Jul 2013 17:11:42 +0200

Simon,

thank you for the thorough and competent review. Some of your points nees discussion.

> that they are present and they improve the credibility of the draft.

the test vectors are identical to those used in draft-merkle-ikev2-ke-brainpool and in RFC 6932, and they have been
verified by Dan Harkins.

> is hidden in the IANA Considerations.

If you have a look at version 01, you see that such a section 2 was present. Sean suggested to remove it. It seems that
your tastes as to the structure differ... I am willing to change the structure and wording as long as there is consensus
on that.

> These curves are suitable for use with DTLS [RFC6347].
This is much less verbose as our previous section 2. Maybe this could be a compromise between Sean's preference of being
compact and your preference of the draft having "guts".

But isn't this syntax incorrect as there are already code points defined for namedCurve? Your text defines namedCurve as
comprising of three values which is not true.

-- 
Johannes

---

From: Simon Josefsson <[email-masked]>
Date: Sat, 6 Jul 2013 09:53:38 +0200

You wrote:

> > When I read the document, it seems to be missing its "gut".  There
> > is one section "Introduction" and then you would expect the actual
> > specification.  But instead comes the Security Considerations and
> > the rest of the usual IETF boiler plate.  The contribution of this
> > document is hidden in the IANA Considerations.
> 
> If you have a look at version 01, you see that such a section 2 was
> present. Sean suggested to remove it. It seems that your tastes as to
> the structure differ... I am willing to change the structure and
> wording as long as there is consensus on that.

I wasn't aware of this earlier discussion -- it was just my reaction
reading the current document, so you shouldn't take it too seriously.
Version 01 seems to contain a lot more normative language in that
section, maybe that was the issue.
 
> This is much less verbose as our previous section 2. Maybe this could
> be a compromise between Sean's preference of being compact and your
> preference of the draft having "guts".

Yes.
 
> But isn't this syntax incorrect as there are already code points
> defined for namedCurve? Your text defines namedCurve as comprising of
> three values which is not true.

No, the TLS meta language is not strictly defined, and the above is
fine. If you compare other TLS documents inroduce new types, this is
how you often express adding new enums.  Compare RFC5878 and RFC6042.

/Simon