Fix parsing for PrivateKeyDer

[Alvenix]

This following key is supposed to be pkcs8 key:

-----BEGIN PRIVATE KEY-----
MFMCAQEwBQYDK2VwBCIEIC2pHJYjFHhK8V7mj6BnHWUVMS4CRolUlDdRXKCtguDuoSMDIQDrvH/x8Nx9untsuc6ET+ce3w7PSuLY8BLWcHdXDGvkQA==
-----END PRIVATE KEY-----

it is included in one of rcgen tests for ring, so it is indeed probably pkcs8. The problem is that the version is set as 1, so the previous parsing considered it Sec1.

I am opening this PR so I can test it with rcgen to check again if it work correctly. I am not sure if this fix the issue. The tests in rcgen works now with this patch.

This may be relevant.

[cpu]

The problem is that the version is set as 1, so the previous parsing considered it Sec1.

Indeed, its version is encoded as 02 01 01 (INTEGER of len 1 with value 1), but RFC 5208 Appendix A defines the Version field as:

Version ::= INTEGER {v1(0)} (v1,...)

I think this is a similar error as I've seen elsewhere (x509 cert version for e.g.): version one is encoded as value zero, not value one. The right encoding for PKCS8 Version v1 is 02 01 00. (Later discussion highlights RFC 5958 and Version 1 being appropriate here).

it is included in one of rcgen tests for ring, so it is indeed probably pkcs8

Aha, specifically ED25519_TEST_KEY_PAIR_PEM_V2, which has a comment explaining its provenance as coming from a debug print in a unit test, presumably using ring 🤔

I'm curious how the version was encoded incorrectly here, but in either case your fix to relax the heuristic seems OK to me in the presence of real world keys like this.

Thanks for digging in!

[cpu]

Thanks

[cpu]

This may be relevant.
...
I'm curious how the version was encoded incorrectly here,

Oh! I missed this link entirely. The response there does seem like a relevant explanation.

[djc]

I'm curious how the version was encoded incorrectly here,

Oh! I missed this link entirely. The response there does seem like a relevant explanation.

So should we not encode that as Pkcs8, but either yield an error or maybe even add a fourth variant?

[Alvenix]

I'm curious how the version was encoded incorrectly here,

Oh! I missed this link entirely. The response there does seem like a relevant explanation.

So should we not encode that as Pkcs8, but either yield an error or maybe even add a fourth variant?

If other crate consider parse it a Pkcs8 or Pkcs8 like, I feel it would add unnecessary complexity to separate them. I assume not, as ring at least which is used on the tests support only pkcs8.

Perhaps a change in the documentation is more appropriate.

[cpu]

So should we not encode that as Pkcs8, but either yield an error or maybe even add a fourth variant?

I'm not sure yet.

RFC 5958 says:

If any items tagged as version
-- 2 are used, the version must be v2, else the version should be
-- v1. When v1, PrivateKeyInfo is the same as it was in [RFC5208]

So it sounds like you could emit a RFC5958 PrivateKeyInfo that was compatible with PKCS8 by avoiding the new fields.

In this case, the PrivateKeyInfo's first and only member OneAsymmetricKey is using a --2 tagged field; it includes the optional implicitly tagged [1] PublicKey at the end. So it should, and does, use version 2 and it isn't strictly speaking PKCS8.

That said: do the downstream APIs we feed these bytes to care? or are they all just treating this as PKCS8 anyway? 🤔

[djc]

Yeah, I guess for our purposes ring and aws-lc-rs are the authorities on what counts as PKCS#8.

[cpu]

So it should, and does, use version 2 and it isn't strictly speaking PKCS8.

I take this back, I think since 5958 obsoletes 5208 it would be fair to still consider it PKCS8.

Ring is specifically citing 5958 for it's definition of PKCS8 so I think in the end this is semantics and we shouldn't have over-fit our heuristic on the RFC 5208 version number.

I think we should ship the fix we have and not split the enum further.

[Alvenix]

The key fails to parse on openssl binding but works with ring.

Here.

[djc]

Ring is specifically citing 5958 for it's definition of PKCS8 so I think in the end this is semantics and we shouldn't have over-fit our heuristic on the RFC 5208 version number.

I think we should ship the fix we have and not split the enum further.

SGTM.

[cpu]

For completeness, it looks like aws-lc-rs is citing the older RFC, but also recognizes V2 and the fn for decoding is described as supporting v1 and v2 through the same entrypoint.