Curve25519

Curve25519

[mt, 2016-08-12 03:40:48]

Sorry this took so long.

I didn't review the curve code extensively, I expect that you will find a second
reviewer (and that your tests and interoperability will be the measure of
correctness).

I did find a couple of bugs and some places where more rigorous checking is
needed.

https://codereview.appspot.com/302400043/diff/1/cmd/ecperf/ecperf.c
File cmd/ecperf/ecperf.c (right):

https://codereview.appspot.com/302400043/diff/1/cmd/ecperf/ecperf.c#newcode512
cmd/ecperf/ecperf.c:512: if (ecCurve_map[curve]->usage & 1) {
Please create an enum for the bits you are using.  You could maybe use the
keyUsage bits here:

http://searchfox.org/nss/rev/0c9572bf1d038dc933133836e0f09d25a7684881/lib/cer...

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c
File cmd/ectest/ectest.c (right):

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode78
cmd/ectest/ectest.c:78: return;
I think that I would include some sort of marker for this.  "(null)" might be
enough.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode255
cmd/ectest/ectest.c:255: /* Prints help information. */
Comments are for why, not what.  Remove this one.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode260
cmd/ectest/ectest.c:260: "-p: usepkcs11\n-n: NIST curves\n-d: non-NIST
curves\n", prog);
The usage line misses -d.

Put this message over several lines.  And add \t before the lines describing the
options:

printf("Usage: %s [-f] [-p] [-n] [-d]\n"
       "\t-f: use freebl curve implementations directly\n"
       "\t-p: use PKCS11 implementations\n"
       "\t-n: test NIST curves\n"
       "\t-d: test non-NIST curves\n", prog);

I don't know if the formatter will mess with that, but it's neater this way
either.

Please also explain that you have to specify at least one of -f and -p, at at
least one of -n and -d.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode280
cmd/ectest/ectest.c:280: /* read command-line arguments */
Useless comment, remove it.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode292
cmd/ectest/ectest.c:292: return 0;
return 1

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode295
cmd/ectest/ectest.c:295: if ((!usepkcs11 && !usefreebl) || !(nist || nonnist)) {
So these two tests are equivalent, but you have used different forms for each. 
Please be consistent.  I prefer the former test form to the latter.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode350
cmd/ectest/ectest.c:350: printf("not okay - PK11 test\n");
This section should identify the failed/passed test.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode364
cmd/ectest/ectest.c:364: return rv;
BUG: If you have more than one test enabled only the result of the last one of
those tests affects the return code.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/testvecs.h
File cmd/ectest/testvecs.h (right):

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/testvecs.h#newcode760
cmd/ectest/testvecs.h:760: //   "curve: 25519 vector: 1000000 iterations" },
Rather than just comment this out, can you use #ifdef NSS_ENABLE_EXPENSIVE_TESTS
or something and add a comment explaining that this isn't enabled by default
because it takes too long to run.  (You don't have to add bits to the Makefile
to turn this on, which makes this a bit like #if 0.)

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/keyhi.h
File lib/cryptohi/keyhi.h (right):

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/keyhi.h#newcode269
lib/cryptohi/keyhi.h:269: /* Set the curve name of a SECKEYPublicKey from the
OID */
This needs much better documentation.

Specifically: Under what circumstances do you believe that this function is
useful or necessary?  I see that you have called it when constructing a key in a
couple of places, but in other cases it is called by an NSS function for you. 
If this is really just for specialized use (i.e., by internal NSS things), then
maybe this can remain a private function.


It might help to explain the contract that is fulfilled by instances of
SECKEYPublicKey that are created by NSS functions and how this function helps
with that contract.

If the point is that all SECKEYPublicKey instances of the four EC curves we
support require this field to be set, then it should be the case that we
populate the value whenever we create a key with those curves.  That means that
this function wouldn't need to be exported, though it might be made available to
other NSS modules for their use (since TLS uses it when constructing key
shares).

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/seckey.c
File lib/cryptohi/seckey.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/seckey.c#newcode554
lib/cryptohi/seckey.c:554: * encodings for each curve. If the name is not set,
many things will fail.
See previous comment about documentation.

This description doesn't help me understand what fails.

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/seckey.c#newcode...
lib/cryptohi/seckey.c:1206: copyk->u.ec.name = pubk->u.ec.name;
Can you PORT_Assert that the name is correct here?  It seems like it might be
tricky to do so.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ec.c
File lib/freebl/ec.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ec.c#newcode261
lib/freebl/ec.c:261: if (ecParams->name == ECCurve25519) {
In what ways could ->name be unset when we reach this point?

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ec.c#newcode430
lib/freebl/ec.c:430: 
You should check that ->name is correctly set before proceeding here.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ec.c#newcode539
lib/freebl/ec.c:539: *      all this.
This is fine, if this function turns into a giant switch statement, I can live
with that.  The XXX is unnecessary, just say: "Perform named-curve-specific
handling for those curves that we have specific code for."  For now, the set of
specially-handled curves has 1 entry.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecl-exp.h
File lib/freebl/ecl/ecl-exp.h (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecl-exp.h#newc...
lib/freebl/ecl/ecl-exp.h:16: EC_dh_dsa = 3
EC_dh_dsa = EC_dh | EC_dsa

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c
File lib/freebl/ecl/ecp_25519.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:22: static unsigned char forbiddenValues[11][32] = {
This should be moved to the validate function.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:26: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 },
So if this is supposed to be 1, then it's not in little-endian form.

Do you have validation tests?

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:73: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
You know that you can do this:

static PRUint8 basePoint[32] = { 9 };

Don't know if that helps any.

This should be moved to the _pt_mul function.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:92: py.len = 32;
Don't calloc a buffer just to avoid initializing one more entry in
forbiddenValues.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:100: }
If this encoding is a thing (please include a reference to where this is
defined), then:

Make the check for encoding more precise:

if (px->len == 34 && px->data[0] == 0x04 && px->data[1] == 0x20)
  p = px->data + 2;
else
  p = px->data;

You don't need to copy anything to p.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:107: for (int i = 0; i < 11; ++i) {
s/11/PR_ARRAY_SIZE(forbiddenValues)/

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:128: unsigned char *px;
Any reason why we can't use PRUint8 throughout?

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:130: if (!P) {
Please check that P.len == 32 (assert and error return both).

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/uint128.c
File lib/freebl/ecl/uint128.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/uint128.c#newc...
lib/freebl/ecl/uint128.c:11: return x.lo & 0x7ffffffffffff;
Should this be 0x7ffffffffffffULL ?

Please make a #define for this value.

https://codereview.appspot.com/302400043/diff/1/lib/softoken/pkcs11.c
File lib/softoken/pkcs11.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/softoken/pkcs11.c#newcode...
lib/softoken/pkcs11.c:1769: || pubKey->u.ec.ecParams.name == ECCurve25519) {
Since ecParams.pointSize is 32 for 25519, I'd be happier if the length check
applied on both branches, i.e.:

pubKey->u.ec.publicValue.len == keyLen &&
(25519 || data[0] == 0x04)

Then you might want to assert that the name isn't 25519 below (before you move
on to the X9.62 decode.

https://codereview.appspot.com/302400043/diff/1/lib/util/secoid.c
File lib/util/secoid.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/util/secoid.c#newcode591
lib/util/secoid.c:591: CONST_OID curve25519[] = { 0x2B, 0x06, 0x01, 0x04, 0x01,
0x5A, 0x47, 0x15, 0x01};
I get a different answer for the above oid:

    2B 06 01 04 01 DA 47 0F 01
                   ^     ^^

One or other is wrong.

[mt, 2016-08-15 06:51:44]

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecl-curve.h
File lib/freebl/ecl/ecl-curve.h (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecl-curve.h#ne...
lib/freebl/ecl/ecl-curve.h:45: 1, 256, 133, EC_dh_dsa
is this supposed to be security level of 256 or 260?

[franziskus, 2016-08-16 13:57:59]

thanks for the review. I hope I addressed everything and will upload a new
version shortly.

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c
File cmd/ectest/ectest.c (right):

https://codereview.appspot.com/302400043/diff/1/cmd/ectest/ectest.c#newcode364
cmd/ectest/ectest.c:364: return rv;
On 2016/08/12 03:40:47, mt wrote:
> BUG: If you have more than one test enabled only the result of the last one of
> those tests affects the return code.

indeed!

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/keyhi.h
File lib/cryptohi/keyhi.h (right):

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/keyhi.h#newcode269
lib/cryptohi/keyhi.h:269: /* Set the curve name of a SECKEYPublicKey from the
OID */
On 2016/08/12 03:40:48, mt wrote:
> This needs much better documentation.
> 
> Specifically: Under what circumstances do you believe that this function is
> useful or necessary?  I see that you have called it when constructing a key in
a
> couple of places, but in other cases it is called by an NSS function for you. 
> If this is really just for specialized use (i.e., by internal NSS things),
then
> maybe this can remain a private function.
> 
> 
> It might help to explain the contract that is fulfilled by instances of
> SECKEYPublicKey that are created by NSS functions and how this function helps
> with that contract.
> 
> If the point is that all SECKEYPublicKey instances of the four EC curves we
> support require this field to be set, then it should be the case that we
> populate the value whenever we create a key with those curves.  That means
that
> this function wouldn't need to be exported, though it might be made available
to
> other NSS modules for their use (since TLS uses it when constructing key
> shares).

We expect every SECKEYPublicKey that holds an EC key to have this name field
set. It is required in pk11_ECPubKeySize to determine the key/point length. The
alternative would be to extract the curve from the public key's DEREncodedParams
each time pk11_ECPubKeySize gets called.  I think setting the name on the key is
the nicer way, but it requires calling the function.

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/seckey.c
File lib/cryptohi/seckey.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/seckey.c#newcode...
lib/cryptohi/seckey.c:1206: copyk->u.ec.name = pubk->u.ec.name;
On 2016/08/12 03:40:48, mt wrote:
> Can you PORT_Assert that the name is correct here?  It seems like it might be
> tricky to do so.

I can set the name again from the params and assert that it's equal to the
original name.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ec.c
File lib/freebl/ec.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ec.c#newcode261
lib/freebl/ec.c:261: if (ecParams->name == ECCurve25519) {
On 2016/08/12 03:40:48, mt wrote:
> In what ways could ->name be unset when we reach this point?

It could not be set at all, or to any random value. The name is passed in with
ecParams from the caller (through EC_NewKey in this case). The caller is
responsible to get this right. (I hope we can remove all the old curves soon,
then none of this is necessary...)

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ec.c#newcode430
lib/freebl/ec.c:430: 
On 2016/08/12 03:40:48, mt wrote:
> You should check that ->name is correctly set before proceeding here.

Well, the only thing I can do is stop processing if the name is not set. I added
the name to all checks for bad args.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c
File lib/freebl/ecl/ecp_25519.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:26: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 },
On 2016/08/12 03:40:48, mt wrote:
> So if this is supposed to be 1, then it's not in little-endian form.
> 
> Do you have validation tests?

It is not little endian.
I added tests for invalid points to ectest.c

https://codereview.appspot.com/302400043/diff/1/lib/util/secoid.c
File lib/util/secoid.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/util/secoid.c#newcode591
lib/util/secoid.c:591: CONST_OID curve25519[] = { 0x2B, 0x06, 0x01, 0x04, 0x01,
0x5A, 0x47, 0x15, 0x01};
On 2016/08/12 03:40:48, mt wrote:
> I get a different answer for the above oid:
> 
>     2B 06 01 04 01 DA 47 0F 01
>                    ^     ^^
> 
> One or other is wrong.

Looks like you're right. Not sure what happened here.

[mt, 2016-08-18 02:49:55]

Looking better.  I hope that I'm not the only reviewer for this stuff.

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/keyhi.h
File lib/cryptohi/keyhi.h (right):

https://codereview.appspot.com/302400043/diff/1/lib/cryptohi/keyhi.h#newcode269
lib/cryptohi/keyhi.h:269: /* Set the curve name of a SECKEYPublicKey from the
OID */
On 2016/08/16 13:57:59, franziskus wrote:
> On 2016/08/12 03:40:48, mt wrote:
> > This needs much better documentation.
> > 
> > Specifically: Under what circumstances do you believe that this function is
> > useful or necessary?  I see that you have called it when constructing a key
in
> a
> > couple of places, but in other cases it is called by an NSS function for
you. 
> > If this is really just for specialized use (i.e., by internal NSS things),
> then
> > maybe this can remain a private function.
> > 
> > 
> > It might help to explain the contract that is fulfilled by instances of
> > SECKEYPublicKey that are created by NSS functions and how this function
helps
> > with that contract.
> > 
> > If the point is that all SECKEYPublicKey instances of the four EC curves we
> > support require this field to be set, then it should be the case that we
> > populate the value whenever we create a key with those curves.  That means
> that
> > this function wouldn't need to be exported, though it might be made
available
> to
> > other NSS modules for their use (since TLS uses it when constructing key
> > shares).
> 
> We expect every SECKEYPublicKey that holds an EC key to have this name field
> set. It is required in pk11_ECPubKeySize to determine the key/point length.
The
> alternative would be to extract the curve from the public key's
DEREncodedParams
> each time pk11_ECPubKeySize gets called.  I think setting the name on the key
is
> the nicer way, but it requires calling the function.

The question I have is: does this need to be exported to the public NSS API.  It
looks like you could avoid doing that, since the guarantee is that NSS always
sets the name on keys that it creates.

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c
File lib/freebl/ecl/ecp_25519.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/freebl/ecl/ecp_25519.c#ne...
lib/freebl/ecl/ecp_25519.c:26: 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01 },
On 2016/08/16 13:57:59, franziskus wrote:
> On 2016/08/12 03:40:48, mt wrote:
> > So if this is supposed to be 1, then it's not in little-endian form.
> > 
> > Do you have validation tests?
> 
> It is not little endian.
> I added tests for invalid points to ectest.c

That's inconsistent because the base point IS little endian.  I thought that the
25519 worked exclusively with little ending values.

I see that you removed comments about things being little-endian; are we
operating on big-endian numbers?  If we are, you should note that difference
from normal 25519 implementations in the code somewhere.

https://codereview.appspot.com/302400043/diff/1/lib/softoken/pkcs11.c
File lib/softoken/pkcs11.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/softoken/pkcs11.c#newcode1
lib/softoken/pkcs11.c:1: /* This Source Code Form is subject to the terms of the
Mozilla Public
Can you highlight your changes in this file?  I can't see them through the
clang-format noise.

https://codereview.appspot.com/302400043/diff/1/lib/softoken/pkcs11c.c
File lib/softoken/pkcs11c.c (right):

https://codereview.appspot.com/302400043/diff/1/lib/softoken/pkcs11c.c#newcode1
lib/softoken/pkcs11c.c:1: /* This Source Code Form is subject to the terms of
the Mozilla Public
Please confirm that there are no new changes in this file.

https://codereview.appspot.com/302400043/diff/40001/cmd/ectest/ectest.c
File cmd/ectest/ectest.c (right):

https://codereview.appspot.com/302400043/diff/40001/cmd/ectest/ectest.c#newco...
cmd/ectest/ectest.c:95: #define INIT_TEST                                       
              \
Why is this a macro?  Sure, the ECDH_KAT and ECDH_BAD structs are different, but
you are only reading ->curve from each.  Better to have a function that takes a
pointer to ecParams and a curve name.  It's a few more lines of code to invoke,
but much easier to work with.

SECStatus init_parameters(ECParams *ecParams, ECCurveName curve) { ... }

https://codereview.appspot.com/302400043/diff/40001/cmd/ectest/ectest.c#newco...
cmd/ectest/ectest.c:266: ectest_validate_point(ECDH_BAD *kat)
It might pay to validate the good points as well.  Just to ensure that they are
OK.

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/keyhi.h
File lib/cryptohi/keyhi.h (right):

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/keyhi.h#newc...
lib/cryptohi/keyhi.h:274: SECStatus SECKEY_SetCurveName(PLArenaPool *arena,
SECKEYPublicKey *pubKey);
Can this be moved to keyi.h ?

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/seckey.c
File lib/cryptohi/seckey.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/seckey.c#new...
lib/cryptohi/seckey.c:555: * of EC public keys will fail.
Isn't it also true that DH operations will fail?

https://codereview.appspot.com/302400043/diff/40001/lib/freebl/ecl/ecp_25519.c
File lib/freebl/ecl/ecp_25519.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/freebl/ecl/ecp_25519....
lib/freebl/ecl/ecp_25519.c:106: PRUint8 basePoint[32] = { 9 };
This point is little endian?

https://codereview.appspot.com/302400043/diff/40001/lib/nss/nss.def
File lib/nss/nss.def (right):

https://codereview.appspot.com/302400043/diff/40001/lib/nss/nss.def#newcode1102
lib/nss/nss.def:1102: SECKEY_SetCurveName;
I would rather this were a private function.

https://codereview.appspot.com/302400043/diff/40001/lib/pk11wrap/pk11akey.c
File lib/pk11wrap/pk11akey.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/pk11wrap/pk11akey.c#n...
lib/pk11wrap/pk11akey.c:407: */
How is this going to work with EdDSA?

https://codereview.appspot.com/302400043/diff/40001/lib/softoken/pkcs11.c
File lib/softoken/pkcs11.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/softoken/pkcs11.c#new...
lib/softoken/pkcs11.c:1802: * and SEC_ASN1_OCTET_STRING are 0x04 */
What are the two cases?

[franziskus, 2016-08-18 14:28:16]

> Looking better.  I hope that I'm not the only reviewer for this stuff.

I hope so as well. I'll ping Bob again when the new version is up.

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/keyhi.h
File lib/cryptohi/keyhi.h (right):

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/keyhi.h#newc...
lib/cryptohi/keyhi.h:274: SECStatus SECKEY_SetCurveName(PLArenaPool *arena,
SECKEYPublicKey *pubKey);
On 2016/08/18 02:49:55, mt wrote:
> Can this be moved to keyi.h ?

if we want to have it internally. But that requires to reimplement the function
for ssl. If that's the way to go, that's fine with me. I did it for now. Let's
see what Bob says to this.

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/seckey.c
File lib/cryptohi/seckey.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/seckey.c#new...
lib/cryptohi/seckey.c:555: * of EC public keys will fail.
On 2016/08/18 02:49:55, mt wrote:
> Isn't it also true that DH operations will fail?

hm, I don't think so. Currently it's only used in pk11_ECPubKeySize.

https://codereview.appspot.com/302400043/diff/40001/lib/freebl/ecl/ecp_25519.c
File lib/freebl/ecl/ecp_25519.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/freebl/ecl/ecp_25519....
lib/freebl/ecl/ecp_25519.c:106: PRUint8 basePoint[32] = { 9 };
On 2016/08/18 02:49:55, mt wrote:
> This point is little endian?

yeah... you were right last time with the other invalid points above. they
should've been little endian and this one is as well. Somehow forgot to fix that
last time... Everything should be little endian now.

https://codereview.appspot.com/302400043/diff/40001/lib/pk11wrap/pk11akey.c
File lib/pk11wrap/pk11akey.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/pk11wrap/pk11akey.c#n...
lib/pk11wrap/pk11akey.c:407: */
On 2016/08/18 02:49:55, mt wrote:
> How is this going to work with EdDSA?

Ed25519 (other EdDSA curves would have a different keyLen)? The public key is of
the same length there (31 bit for y coordinate and 1 bit for x) and doesn't get
any additional encoding (looking at [1]). So this should work in that case as
well, though the value will be interpreted differently later.

[1] https://tools.ietf.org/html/draft-irtf-cfrg-eddsa-06

https://codereview.appspot.com/302400043/diff/40001/lib/softoken/pkcs11.c
File lib/softoken/pkcs11.c (right):

https://codereview.appspot.com/302400043/diff/40001/lib/softoken/pkcs11.c#new...
lib/softoken/pkcs11.c:1802: * and SEC_ASN1_OCTET_STRING are 0x04 */
On 2016/08/18 02:49:55, mt wrote:
> What are the two cases?

EC_POINT_FORM_UNCOMPRESSED and SEC_ASN1_OCTET_STRING. I clarified the comment.

[mt, 2016-09-01 13:13:02]

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/keyhi.h
File lib/cryptohi/keyhi.h (right):

https://codereview.appspot.com/302400043/diff/40001/lib/cryptohi/keyhi.h#newc...
lib/cryptohi/keyhi.h:274: SECStatus SECKEY_SetCurveName(PLArenaPool *arena,
SECKEYPublicKey *pubKey);
On 2016/08/18 14:28:15, franziskus wrote:
> On 2016/08/18 02:49:55, mt wrote:
> > Can this be moved to keyi.h ?
> 
> if we want to have it internally. But that requires to reimplement the
function
> for ssl. If that's the way to go, that's fine with me. I did it for now. Let's
> see what Bob says to this.

 PRIVATE_EXPORTS = \
+        keyi.h    \
         $(NULL)

And then ssl can use it.