-
Notifications
You must be signed in to change notification settings - Fork 103
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
balena
wants to merge
5
commits into
ProtonMail:main
Choose a base branch
from
balena:wrap-keys
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
db6d440
Added the function `NewEntityFromKey` that allows passing precreated …
balena 2eb6c1f
Added support to `crypto/ed25519` too.
balena 4a3ea54
Fixed the comment adding support to Ed25519 too.
balena 497e4b2
Assigning also the `DefaultHash` to correspond to the ECDSA key size.
balena 3f24ede
* Fixed wrapping of `crypto/ed25519` keys.
balena File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,163 @@ | ||
package openpgp | ||
|
||
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") | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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, makingecc
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.
There was a problem hiding this comment.
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 ofpriv
from*ecdsa.PrivateKey
to something else. A natural candidate isany
, and documenting the function accepts both inputs, but ergonomically speaking you may open space for undesired bugs and extraneous work such as handling bothecdsa.PrivateKey
and the pointer version*ecdsa.PrivateKey
. Or find some common interface between both implementations.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.ecc
public, so that implementors do the weight lifting to support standard libraries out of ProtonMail/go-crypto.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree it's a bit ugly.
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 internalecdsa
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.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.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.