Private key imported from bytearray should be imported as unsigned

[romanstrobl]

We should use unsigned conversion when converting byte[] representation of the d number to BigInteger. See Bouncy Castle utility method:

https://github.com/bcgit/bc-java/blob/ff19d41ba1968fe381d67794d32d959aca307406/core/src/main/java/org/bouncycastle/util/BigIntegers.java#L108

[hvge]

There's also export to unsigned byte array. It basically skips that leading zero byte. One byte could save the world :)

[petrdvorak]

@romanstrobl Does this have any impact on server_private_key we already store in the database?

[hvge]

I think that import is without an impact, but once we save key as unsigned, then the older code will not be able to load it correctly (e.g., imported key will be different). That might cause problems during our own development only. For example, If you'll deploy older server to "dev" by accident, but will work against database already filled with keys from newer code.

But this is in general irrelevant, because the only problem so far is with test vectors, generated from mobile SDK. But I agree, that mathematically correct is to use unsigned integers.

[romanstrobl]

See how BC handles decoding of points: https://github.com/bcgit/bc-java/blob/master/core/src/main/java/org/bouncycastle/math/ec/ECCurve.java#L389

We use this BC method in method convertBytesToPublicKey. This is why we never had problems with sign byte in public keys.

[romanstrobl]

You are right the problem is only when we use test vectors from mobile SDK, which is rare. The other question is compatibility with BC library which uses unsigned byte[] for conversions of all raw keys (see usages of fromUnsignedByteArray and asUnsignedByteArray methods in BC). Perhaps we should postpone investigating this issue for next release, so there is enough time to think this change through. But I think it is better to read/store the keys in compatible way with BC library.