Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 163 additions & 0 deletions openpgp/key_wrap.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
package openpgp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@balena balena Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

@balena balena Sep 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@balena balena Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@twiss twiss Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#227 might also partially solve this issue, perhaps?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we might still need the above as well.


import (
"crypto"
"crypto/ecdsa"
"crypto/ed25519"
"crypto/elliptic"
"crypto/rsa"

"github.com/ProtonMail/go-crypto/openpgp/eddsa"
"github.com/ProtonMail/go-crypto/openpgp/errors"
"github.com/ProtonMail/go-crypto/openpgp/internal/ecc"
"github.com/ProtonMail/go-crypto/openpgp/packet"

internalecdsa "github.com/ProtonMail/go-crypto/openpgp/ecdsa"
)

// NewSigningEntityFromKey returns an Entity that contains either a RSA, ECDSA
// or Ed25519 keypair passed by the user with a single identity composed of the
// given full name, comment and email, any of which may be empty but must not
// contain any of "()<>\x00". If config is nil, sensible defaults will be used.
// It is not required to assign any of the key type parameters in the config
// (in fact, they will be ignored); these will be set based on the passed key.
//
// The following key types are currently supported: *rsa.PrivateKey,
// *ecdsa.PrivateKey and ed25519.PrivateKey (not a pointer). Unsupported key
// types result in an error.
func NewSigningEntityFromKey(name, comment, email string, key crypto.PrivateKey, config *packet.Config) (*Entity, error) {
creationTime := config.Now()
keyLifetimeSecs := config.KeyLifetime()

primaryPrivRaw, err := newSignerFromKey(key, config)
if err != nil {
return nil, err
}
primary := packet.NewSignerPrivateKey(creationTime, primaryPrivRaw)
if config.V6() {
primary.UpgradeToV6()
}

e := &Entity{
PrimaryKey: &primary.PublicKey,
PrivateKey: primary,
Identities: make(map[string]*Identity),
Subkeys: []Subkey{},
Signatures: []*packet.Signature{},
}

if config.V6() {
// In v6 keys algorithm preferences should be stored in direct key signatures
selfSignature := createSignaturePacket(&primary.PublicKey, packet.SigTypeDirectSignature, config)
err = writeKeyProperties(selfSignature, creationTime, keyLifetimeSecs, config)
if err != nil {
return nil, err
}
err = selfSignature.SignDirectKeyBinding(&primary.PublicKey, primary, config)
if err != nil {
return nil, err
}
e.Signatures = append(e.Signatures, selfSignature)
e.SelfSignature = selfSignature
}

err = e.addUserId(name, comment, email, config, creationTime, keyLifetimeSecs, !config.V6())
if err != nil {
return nil, err
}

return e, nil
}

func (e *Entity) AddSigningSubkeyFromKey(key crypto.PrivateKey, config *packet.Config) error {
creationTime := config.Now()
keyLifetimeSecs := config.KeyLifetime()

subPrivRaw, err := newSignerFromKey(key, config)
if err != nil {
return err
}
sub := packet.NewSignerPrivateKey(creationTime, subPrivRaw)
sub.IsSubkey = true
if config.V6() {
sub.UpgradeToV6()
}

subkey := Subkey{
PublicKey: &sub.PublicKey,
PrivateKey: sub,
}
subkey.Sig = createSignaturePacket(e.PrimaryKey, packet.SigTypeSubkeyBinding, config)
subkey.Sig.CreationTime = creationTime
subkey.Sig.KeyLifetimeSecs = &keyLifetimeSecs
subkey.Sig.FlagsValid = true
subkey.Sig.FlagSign = true
subkey.Sig.EmbeddedSignature = createSignaturePacket(subkey.PublicKey, packet.SigTypePrimaryKeyBinding, config)
subkey.Sig.EmbeddedSignature.CreationTime = creationTime

err = subkey.Sig.EmbeddedSignature.CrossSignKey(subkey.PublicKey, e.PrimaryKey, subkey.PrivateKey, config)
if err != nil {
return err
}

err = subkey.Sig.SignKey(subkey.PublicKey, e.PrivateKey, config)
if err != nil {
return err
}

e.Subkeys = append(e.Subkeys, subkey)
return nil
}

func newSignerFromKey(key crypto.PrivateKey, config *packet.Config) (interface{}, error) {
switch key := key.(type) {
case *rsa.PrivateKey:
config.Algorithm = packet.PubKeyAlgoRSA
return key, nil
case *ecdsa.PrivateKey:
var c ecc.ECDSACurve
switch key.Curve {
case elliptic.P256():
c = ecc.NewGenericCurve(elliptic.P256())
config.Curve = packet.CurveNistP256
// The default hash SHA256 will serve here
case elliptic.P384():
c = ecc.NewGenericCurve(elliptic.P384())
config.Curve = packet.CurveNistP384
if config.DefaultHash == 0 {
config.DefaultHash = crypto.SHA384
}
case elliptic.P521():
c = ecc.NewGenericCurve(elliptic.P521())
config.Curve = packet.CurveNistP521
if config.DefaultHash == 0 {
config.DefaultHash = crypto.SHA512
}
default:
return nil, errors.InvalidArgumentError("unsupported elliptic curve")
}
priv := internalecdsa.NewPrivateKey(
*internalecdsa.NewPublicKey(c),
)
priv.PublicKey.X, priv.PublicKey.Y, priv.D = key.X, key.Y, key.D
config.Algorithm = packet.PubKeyAlgoECDSA
return priv, nil
case ed25519.PrivateKey:
if config.V6() {
// Implementations MUST NOT accept or generate v6 key material
// using the deprecated OIDs.
return nil, errors.InvalidArgumentError("EdDSALegacy cannot be used for v6 keys")
}
curve := ecc.FindEdDSAByGenName(string(packet.Curve25519))
if curve == nil {
return nil, errors.InvalidArgumentError("unsupported curve")
}
priv := eddsa.NewPrivateKey(*eddsa.NewPublicKey(curve))
priv.PublicKey.X = key.Public().(ed25519.PublicKey)[:]
priv.D = key.Seed()
config.Algorithm = packet.PubKeyAlgoEdDSA
return priv, nil
default:
return nil, errors.InvalidArgumentError("unsupported public key algorithm")
}
}