New feature: Added the function NewEntityFromKey that allows passing pre-created RSA, ECDSA or Ed25519 keys.

[balena]

The objective of this PR is to add the possibility of passing pre-created RSA, ECDSA or Ed25519 keys, possibly coming from deterministic sources such as used in key derivation.

As other elliptic curves such as secp256k1 aren't supported by Go standard libraries, I left them aside.

[tv2-mnilsen]

Wanted feature! Right now its very hard to just use ur existing ssh keypair for the signed commits

[twiss]

Apologies for the long delay in reviewing.

Could you add this in openpgp/v2, instead, please? This package is meant as a drop-in replacement of x/crypto/openpgp, only.

[balena]

As you opted by implementing your own elliptic curves cryptography package as an internal one, implementors simply can't use Go standard libraries when dealing with ProtonMail/go-crypto/openpgp as drop in replacement...

Another option would be to make 'ecc' a public package rather than internal so that implementors can do whatever they need to integrate standard ECC.

[twiss]

I'm not sure I fully understand your comment. This functionality isn't available in x/crypto/openpgp either, right?

All I'm really asking is to move this to openpgp/v2/key_wrap.go, under the openpgp/v2 package. Would that work for you, or is there some reason you can't switch to openpgp/v2?

[balena]

If you inspect x/crypto/openpgp, one can use standard library ECDSA keys for instance in packet.PrivateKey. Whereas ProtonMail/go-crypto does not, instead you can see it has been opted using ProtonMail/go-crypto/openpgp/ecdsa instead.

I can certainly do the work of porting to your v2 branch; just wanted to ensure you understand the above as it is incompatible with the "drop-in" argument.

[twiss]

OK, I see what you mean, thanks. Arguably, that's also a breaking change NewECDSAPrivateKey, for example, that we might want to revert. Or we could accept both variants there, perhaps. And yeah, making ecc public might also be reasonable.

Do you want to make a PR for either or both of those? Otherwise I can take a stab at it as well.

[balena]

Time is (always) short. But in order to be effective, I would first decide what's best.

• NewECDSAPrivateKey to accept both ProtonMail/go-crypto/openpgp/ecdsa and crypto/ecdsa implies transforming the parameter type of priv from *ecdsa.PrivateKey to something else. A natural candidate is any, and documenting the function accepts both inputs, but ergonomically speaking you may open space for undesired bugs and extraneous work such as handling both ecdsa.PrivateKey and the pointer version *ecdsa.PrivateKey. Or find some common interface between both implementations.
• Another option is just build even another builder function with a different name. I don't know, maybe NewStdECDSAPrivateKey... I don't personally see this as a "breaking change", as you're not breaking any existing code with it, but giving the opportunity to them to use the standard libraries.
• Of course there's a more in-depth change, which is to replace ECDSA support by the standard libraries... I know the consequences, this is certainly a breaking change. But would worth understanding why there's yet this another ECDSA implementation here when there's a very good one and well supported at the standard libraries (maybe historical reasons?).
• Or just make ecc public, so that implementors do the weight lifting to support standard libraries out of ProtonMail/go-crypto.

[twiss]

• NewECDSAPrivateKey to accept both ProtonMail/go-crypto/openpgp/ecdsa and crypto/ecdsa implies transforming the parameter type of priv from *ecdsa.PrivateKey to something else. A natural candidate is any, and documenting the function accepts both inputs, but ergonomically speaking you may open space for undesired bugs and extraneous work such as handling both ecdsa.PrivateKey and the pointer version *ecdsa.PrivateKey. Or find some common interface between both implementations.

Yeah, I agree it's a bit ugly.

• Another option is just build even another builder function with a different name. I don't know, maybe NewStdECDSAPrivateKey... I don't personally see this as a "breaking change", as you're not breaking any existing code with it, but giving the opportunity to them to use the standard libraries.

What I meant to say before is that, given that go-crypto/openpgp is meant to be a drop-in replacement to x/crypto/openpgp, changing NewECDSAPrivateKey to accept an internal ecdsa key rather than a standard one was a breaking change, and arguably was a mistake at the time (although the goals of this package were less clearly defined at the time), and thus perhaps should be reverted. That would be another breaking change now (if we don't accept both), but arguably the function in its current state isn't very useful.

• Of course there's a more in-depth change, which is to replace ECDSA support by the standard libraries... I know the consequences, this is certainly a breaking change. But would worth understanding why there's yet this another ECDSA implementation here when there's a very good one and well supported at the standard libraries (maybe historical reasons?).

The internal ecc package only implements curves that the standard library doesn't, like Brainpool. For the NIST curves it calls out to the standard library. Using the internal package for all curves just makes the code more consistent.

• Or just make ecc public, so that implementors do the weight lifting to support standard libraries out of ProtonMail/go-crypto.

I think it's not entirely unreasonable, but at the same time the other solutions might be better in case we want to change the internal representation of ecc keys in the future, for example. So if you have time I'd go for those (or if not once again lmk, and I can take a stab at it).

---

Probably the cleanest solution is to make all New{ECDSA,ECDH,{Ed,X}{25519,448}}{Private,Public}Key functions private (but still accepting the internal key representations), and make new public versions that convert from the standard representation (where available) to the internal one, and then call the private function. This library itself can call the private functions directly.

Does that seem reasonable?

[twiss]

#227 might also partially solve this issue, perhaps?

[balena]

It does solve, at least partially, for signatures. Not sure right now if it would solve asymmetric encryption cases (w/ ECDH).

[twiss]

Right, we might still need the above as well.