On 12/15/2017 11:31 AM, Tobias Wagner wrote:
Hi,
in our current project, we have the requirement to support brainpool curves for
TLS connections (RFC 7027).
As part of this requirement, we introduced the brainpoolP*r1 curves to SunEC,
as they are already known in sun.security.util.CurveDB. It does not introduce
the twisted curves from RFC 5639. We want to share this patch, hoping it might
be useful for others. Especially for public funded projects (e.g. health care
or eID) in Europe, the use or at least support for these curves is often
mandatory.
Great! You are going to make it a happy Christmas for a lot of people. I
would be happy to sponsor this change, and I have a few initial
requests/comments/questions before I look at the patch in more detail:
1) Please include a test which checks the new curves against some test
vectors. Ideally, these vectors come from a standard (e.g. RFC 7027 has
some).
2) The existing tests will check for some form of correctness for all
enabled curves, but this doesn't ensure that the Brainpool curves are
enabled. So you should also add a test that ensures that the new curves
are enabled. This can be combined with the previously mentioned test
against the test vectors.
3) We can't push this change to the repo without also fixing
JDK-8189594. So I think you have a couple of options. Either submit a
separate patch for JDK-8189594 first (which may be tricky because you
will need to find a way to write a test for it) or include the fix for
JDK-8189594 in this patch.
4) How important are the 224-bit and smaller curves? We can't use them
in TLS, and I don't think we should add curves that are already obsolete
unless there is a good reason.
