feat(keys): add test vectors for protobuf encoding

[thomaseizinger]

@marten-seemann @MarcoPolo Would appreciate if you could cross-check these with the go implementation. Also cc @Menduist for nim-libp2p.

Here is the commit adding the tests to rust-libp2p: https://github.com/drHuangMHT/rust-libp2p/blob/92f6914bf5c6c8dc12146c64c967f407000354a0/identity/src/keypair.rs#L665-L733

Resolves #533.

[thomaseizinger]

Sorry for the noise, this isn't ready yet. Just found a bug in our encoding of RSA keys.

[thomaseizinger]

Okay, the first two test vectors are now ready. Could you please cross check them before we merge the PR on our end?

We've split these up into multiple PRs in rust-libp2p now so my plan is to extend this with more test cases as we go along but it would be great to already check them now that they are correct.

[marten-seemann]

@thomaseizinger Pretty busy right now, but if you want to send us a PR for go-libp2p, would be happy to review.

[Menduist]

This key is failing in nim-libp2p, here:
https://github.com/status-im/nim-libp2p/blob/a1eb53b1813df5d4fa1b343ef003ad9fb5ebb1d8/libp2p/crypto/ecnist.nim#L619-L620

I'm not familiar enough to know if it's a problem on our side or an invalid key, but can look deeper if no one knows

[Menduist]

Looked a bit more into it
https://www.rfc-editor.org/rfc/rfc5915
Specificies:

   ECPrivateKey ::= SEQUENCE {
     version        INTEGER { ecPrivkeyVer1(1) } (ecPrivkeyVer1),
     privateKey     OCTET STRING,
     parameters [0] ECParameters {{ NamedCurve }} OPTIONAL,
     publicKey  [1] BIT STRING OPTIONAL
   }

version specifies the syntax version number of the elliptic curve
private key structure. For this version of the document, it SHALL
be set to ecPrivkeyVer1, which is of type INTEGER and whose value
is one (1).

But this key contains 0:
https://lapo.it/asn1js/#MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgEi8z2tSed90aRnrEtLlDIHnTrAwRD5pgjob_MadVLlWhRANCAAStpBjE8fdxXvGjZdmuHoD11waatV7hnxxNd7ROqNycvaVY_ojcMnecaFc78zDa7q4fRBWKO31LMlIwqNrw1G3F
(uncheck "with definitions", first integer = 0)

So unless another spec is used, this is invalid

[drHuangMHT]

The first integer being 0 is defined here in RFC5208

PrivateKeyInfo ::= SEQUENCE {
  version                   Version,
  privateKeyAlgorithm       PrivateKeyAlgorithmIdentifier,
  privateKey                PrivateKey,
  attributes           [0]  IMPLICIT Attributes OPTIONAL 
}

version is the syntax version number, for compatibility with
future revisions of this document. It shall be 0 for this version
of the document.

as a part of PKCS#8 "header".
The 1 you are looking for is the next integer

SEQUENCE (3 elem)
  INTEGER 0 ---- Version
  SEQUENCE (2 elem) ---- PrivateKeyAlgorithmIdentifier 
    OBJECT IDENTIFIER 1.2.840.10045.2.1 ecPublicKey (ANSI X9.62 public key type)
    OBJECT IDENTIFIER 1.2.840.10045.3.1.7 prime256v1 (ANSI X9.62 named elliptic curve)
  OCTET STRING (109 byte) 306B0201010420122F33DAD… ---- PrivateKey
    SEQUENCE (3 elem)
      INTEGER 1    <<<< here
      OCTET STRING (32 byte) 122F33DAD49E77DD1A... ---- 32 bytes private key
      [1] (1 elem)
        BIT STRING (520 bit) 000001001010110110100... ---- 520 bit or 65 bytes public key

which is 1.
Additionally, the key generated by Rust implementation of secp256r1 also generates keys with first integer being 0.

[drHuangMHT]

The problem is I don't know what "DER-encoded PKIX" actually means. The key itself is valid, but I'm not sure whether the wrapper is correct. The spec should refer directly to RFC5208 and/or RFC5915.

[Menduist]

You are right, nim-libp2p expects the PrivateKey directly. I think go-libp2p does too, since we use some test vectors generated from their lib, and they look like this:
https://lapo.it/asn1js/#MHcCAQEEID5bH-lxLmwxSUKnUL1nSF3jwe_oWxv7Ugro-a49-kpMoAoGCCqGSM49AwEHoUQDQgAE3j0wD6Nq4Oj11TCJnYOrq0Sr8xYfFipLyQHY5uzaAg6LbV-NowUl5x1oUVEMCY5cR8ZGpZf7Tc7ANOn3fECeYg

[Menduist]

We just have some that we generated from go-libp2p here: https://github.com/status-im/nim-libp2p/blob/a1eb53b1813df5d4fa1b343ef003ad9fb5ebb1d8/tests/testpeerid.nim

[thomaseizinger]

We just have some that we generated from go-libp2p here: https://github.com/status-im/nim-libp2p/blob/a1eb53b1813df5d4fa1b343ef003ad9fb5ebb1d8/tests/testpeerid.nim

Awesome thanks!

I'll use those for this PR then!

@drHuangMHT Can you use those for our tests? We just need to rewrite them to assert against a peer ID instead of a public key.

[Menduist]

Well, that sure makes my job easier :D

[drHuangMHT]

Failed to decode provided keys in https://github.com/status-im/nim-libp2p/blob/a1eb53b1813df5d4fa1b343ef003ad9fb5ebb1d8/tests/testpeerid.nim. However by skipping the first 4 bytes the keys can be decoded, which is the key in

You are right, nim-libp2p expects the PrivateKey directly. I think go-libp2p does too, since we use some test vectors generated from their lib, and they look like this: https://lapo.it/asn1js/#MHcCAQEEID5bH-lxLmwxSUKnUL1nSF3jwe_oWxv7Ugro-a49-kpMoAoGCCqGSM49AwEHoUQDQgAE3j0wD6Nq4Oj11TCJnYOrq0Sr8xYfFipLyQHY5uzaAg6LbV-NowUl5x1oUVEMCY5cR8ZGpZf7Tc7ANOn3fECeYg

The online decoder also can't decode those keys.

[drHuangMHT]

Never mind the first 4 bytes come from protobuf encoding

[mxinden]

@thomaseizinger mind accepting the suggestion below adding an RSA fixture?

Also what is missing for this to be merged. I find it very useful.

[thomaseizinger]

@thomaseizinger mind accepting the suggestion below adding an RSA fixture?

I can't unfortunately. Can you open a PR to my fork?

Also what is missing for this to be merged. I find it very useful.

I'd like at least one other implementation to verify this before we merge here.

[mxinden]

@thomaseizinger mind accepting the suggestion below adding an RSA fixture?

I can't unfortunately. Can you open a PR to my fork?

For some reason I am able to. Hope you don't mind. a5926d7

Also what is missing for this to be merged. I find it very useful.

I'd like at least one other implementation to verify this before we merge here.

@Menduist given your involvement above, would you mind using these fixtures in your CI?

[Menduist]

Green on nim-libp2p

[thomaseizinger]

@mxinden do you mind approving this so we can merge it?

[mxinden]

Thank you for the work here @thomaseizinger, @drHuangMHT and @Menduist.