[crypto] openpgp: support GNU dummy S2K for missing private keys
Péter Szilágyi (Gerrit)
Péter Szilágyi would like Adam Langley to review openpgp: support GNU dummy S2K for missing private keys.
openpgp: support GNU dummy S2K for missing private keys GNU defines an extension to the S2K algorithms where the private key of a PGP is missing and only subkeys are present fully. These incomplete keys are useful in scenarios where a user distributes various subkeys to individual places (e.g. various build servers) while retaining the master key. The extension is described at: http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109 Fixes golang/go#13605 Change-Id: I901d7c902d255a6b73a787a6b2a9c6181c4de048 --- M openpgp/keys_test.go M openpgp/packet/private_key.go M openpgp/s2k/s2k.go M openpgp/s2k/s2k_test.go 4 files changed, 104 insertions(+), 16 deletions(-)
diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go
index fbc8fc2..3725960 100644
--- a/openpgp/keys_test.go
+++ b/openpgp/keys_test.go
@@ -102,6 +102,23 @@
}
}
+func TestGNUDummyPrivateKey(t *testing.T) {
+ // This public key has a signing subkey, but has a dummy placeholder
+ // instead of the real private key. It's used in scenarios where the
+ // main private key is witheld and only signing is allowed (e.g. build
+ // servers).
+ keys, err := ReadArmoredKeyRing(bytes.NewBufferString(onlySubkeyNoPrivateKey))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(keys) != 1 {
+ t.Errorf("Failed to accept key with dummy private key, %d", len(keys))
+ }
+ if len(keys[0].Subkeys) != 1 {
+ t.Errorf("Failed to accept good subkey, %d", len(keys[0].Subkeys))
+ }
+}
+
// TestExternallyRevokableKey attempts to load and parse a key with a third party revocation permission.
func TestExternallyRevocableKey(t *testing.T) {
kring, _ := ReadKeyRing(readerFromHex(subkeyUsageHex))
@@ -402,3 +419,33 @@
MtgVijRGXR/lGLGETPg2X3Afwn9N9bLMBkBprKgbBqU7lpaoPupxT61bL70=
=vtbN
-----END PGP PUBLIC KEY BLOCK-----`
+
+const onlySubkeyNoPrivateKey = `-----BEGIN PGP PRIVATE KEY BLOCK-----
+Version: GnuPG v1
+
+lQCVBFggvocBBAC7vBsHn7MKmS6IiiZNTXdciplVgS9cqVd+RTdIAoyNTcsiV1H0
+GQ3QtodOPeDlQDNoqinqaobd7R9g3m3hS53Nor7yBZkCWQ5x9v9JxRtoAq0sklh1
+I1X2zEqZk2l6YrfBF/64zWrhjnW3j23szkrAIVu0faQXbQ4z56tmZrw11wARAQAB
+/gdlAkdOVQG0CUdOVSBEdW1teYi4BBMBAgAiBQJYIL6HAhsDBgsJCAcDAgYVCAIJ
+CgsEFgIDAQIeAQIXgAAKCRCd1xxWp1CYAnjGA/9synn6ZXJUKAXQzySgmCZvCIbl
+rqBfEpxwLG4Q/lONhm5vthAE0z49I8hj5Gc5e2tLYUtq0o0OCRdCrYHa/efOYWpJ
+6RsK99bePOisVzmOABLIgZkcr022kHoMCmkPgv9CUGKP1yqbGl+zzAwQfUjRUmvD
+ZIcWLHi2ge4GzPMPi50B2ARYIL6cAQQAxWHnicKejAFcFcF1/3gUSgSH7eiwuBPX
+M7vDdgGzlve1o1jbV4tzrjN9jsCl6r0nJPDMfBSzgLr1auNTRG6HpJ4abcOx86ED
+Ad+avDcQPZb7z3dPhH/gb2lQejZsHh7bbeOS8WMSzHV3RqCLd8J/xwWPNR5zKn1f
+yp4IGfopidMAEQEAAQAD+wQOelnR82+dxyM2IFmZdOB9wSXQeCVOvxSaNMh6Y3lk
+UOOkO8Nlic4x0ungQRvjoRs4wBmCuwFK/MII6jKui0B7dn/NDf51i7rGdNGuJXDH
+e676By1sEY/NGkc74jr74T+5GWNU64W0vkpfgVmjSAzsUtpmhJMXsc7beBhJdnVl
+AgDKCb8hZqj1alcdmLoNvb7ibA3K/V8J462CPD7bMySPBa/uayoFhNxibpoXml2r
+oOtHa5izF3b0/9JY97F6rqkdAgD6GdTJ+xmlCoz1Sewoif1I6krq6xoa7gOYpIXo
+UL1Afr+LiJeyAnF/M34j/kjIVmPanZJjry0kkjHE5ILjH3uvAf4/6n9np+Th8ujS
+YDCIzKwR7639+H+qccOaddCep8Y6KGUMVdD/vTKEx1rMtK+hK/CDkkkxnFslifMJ
+kqoqv3WUqCWJAT0EGAECAAkFAlggvpwCGwIAqAkQndccVqdQmAKdIAQZAQIABgUC
+WCC+nAAKCRDmGUholQPwvQk+A/9latnSsR5s5/1A9TFki11GzSEnfLbx46FYOdkW
+n3YBxZoPQGxNA1vIn8GmouxZInw9CF4jdOJxEdzLlYQJ9YLTLtN5tQEMl/19/bR8
+/qLacAZ9IOezYRWxxZsyn6//jfl7A0Y+FV59d4YajKkEfItcIIlgVBSW6T+TNQT3
+R+EH5HJ/A/4/AN0CmBhhE2vGzTnVU0VPrE4V64pjn1rufFdclgpixNZCuuqpKpoE
+VVHn6mnBf4njKjZrAGPs5kfQ+H4NsM7v3Zz4yV6deu9FZc4O6E+V1WJ38rO8eBix
+7G2jko106CC6vtxsCPVIzY7aaG3H5pjRtomw+pX7SzrQ7FUg2PGumg==
+=F/T0
+-----END PGP PRIVATE KEY BLOCK-----`
diff --git a/openpgp/packet/private_key.go b/openpgp/packet/private_key.go
index 34734cc..c3caffb 100644
--- a/openpgp/packet/private_key.go
+++ b/openpgp/packet/private_key.go
@@ -112,6 +112,9 @@
if s2kType == 254 {
pk.sha1Checksum = true
}
+ if pk.s2k == nil {
+ return
+ }
default:
return errors.UnsupportedError("deprecated s2k function in private key")
}
@@ -233,7 +236,7 @@
// Decrypt decrypts an encrypted private key using a passphrase.
func (pk *PrivateKey) Decrypt(passphrase []byte) error {
- if !pk.Encrypted {
+ if !pk.Encrypted || pk.s2k == nil {
return nil
}
diff --git a/openpgp/s2k/s2k.go b/openpgp/s2k/s2k.go
index 4b9a44c..75ac50e 100644
--- a/openpgp/s2k/s2k.go
+++ b/openpgp/s2k/s2k.go
@@ -7,6 +7,7 @@
package s2k // import "golang.org/x/crypto/openpgp/s2k"
import (
+ "bytes"
"crypto"
"hash"
"io"
@@ -151,32 +152,46 @@
}
}
+// hashIdToHash returns an instance of the hasher specified by the id.
+func hashIdToHash(id byte) (hash.Hash, error) {
+ hash, ok := HashIdToHash(id)
+ if !ok {
+ return nil, errors.UnsupportedError("hash for S2K function: " + strconv.Itoa(int(id)))
+ }
+ if !hash.Available() {
+ return nil, errors.UnsupportedError("hash not available: " + strconv.Itoa(int(hash)))
+ }
+ return hash.New(), nil
+}
+
// Parse reads a binary specification for a string-to-key transformation from r
// and returns a function which performs that transform.
func Parse(r io.Reader) (f func(out, in []byte), err error) {
- var buf [9]byte
+ var (
+ buf [9]byte
+ h hash.Hash
+ )
_, err = io.ReadFull(r, buf[:2])
if err != nil {
return
}
- hash, ok := HashIdToHash(buf[1])
- if !ok {
- return nil, errors.UnsupportedError("hash for S2K function: " + strconv.Itoa(int(buf[1])))
- }
- if !hash.Available() {
- return nil, errors.UnsupportedError("hash not available: " + strconv.Itoa(int(hash)))
- }
- h := hash.New()
-
switch buf[0] {
case 0:
+ h, err = hashIdToHash(buf[1])
+ if err != nil {
+ return
+ }
f := func(out, in []byte) {
Simple(out, h, in)
}
return f, nil
case 1:
+ h, err = hashIdToHash(buf[1])
+ if err != nil {
+ return
+ }
_, err = io.ReadFull(r, buf[:8])
if err != nil {
return
@@ -186,6 +201,10 @@
}
return f, nil
case 3:
+ h, err = hashIdToHash(buf[1])
+ if err != nil {
+ return
+ }
_, err = io.ReadFull(r, buf[:9])
if err != nil {
return
@@ -195,8 +214,19 @@
Iterated(out, h, in, buf[:8], count)
}
return f, nil
+ case 101:
+ _, err = io.ReadFull(r, buf[:4])
+ if err != nil {
+ return
+ }
+ if bytes.Compare(buf[:3], []byte("GNU")) != 0 {
+ return nil, errors.UnsupportedError("S2K private function")
+ }
+ if buf[3] != 0x01 {
+ return nil, errors.UnsupportedError("GNU S2K function mode")
+ }
+ return nil, nil
}
-
return nil, errors.UnsupportedError("S2K function")
}
diff --git a/openpgp/s2k/s2k_test.go b/openpgp/s2k/s2k_test.go
index 183d260..04c3c8f 100644
--- a/openpgp/s2k/s2k_test.go
+++ b/openpgp/s2k/s2k_test.go
@@ -70,13 +70,16 @@
var parseTests = []struct {
spec, in, out string
+ nof bool
}{
/* Simple with SHA1 */
- {"0002", "hello", "aaf4c61d"},
+ {"0002", "hello", "aaf4c61d", false},
/* Salted with SHA1 */
- {"01020102030405060708", "hello", "f4f7d67e"},
+ {"01020102030405060708", "hello", "f4f7d67e", false},
/* Iterated with SHA1 */
- {"03020102030405060708f1", "hello", "f2a57b7c"},
+ {"03020102030405060708f1", "hello", "f2a57b7c", false},
+ /* GNU dummy S2K */
+ {"6502474e5501", "", "", true},
}
func TestParse(t *testing.T) {
@@ -88,7 +91,12 @@
t.Errorf("%d: Parse returned error: %s", i, err)
continue
}
-
+ if test.nof {
+ if f != nil {
+ t.Errorf("%d: GNU dummy parse returned non-nil function", i)
+ }
+ continue
+ }
expected, _ := hex.DecodeString(test.out)
out := make([]byte, len(expected))
f(out, []byte(test.in))
To view, visit this change. To unsubscribe, visit settings.
Niklas Merz (Gerrit)
I would love to use these types of keys with Go
Patch set 1:Code-Review +1
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Adam Langley (Gerrit)
Adam Langley uploaded patch set #2 to the change originally created by Péter Szilágyi.
openpgp: support GNU dummy S2K for missing private keys
GNU defines an extension to the S2K algorithms where the private
key of a PGP is missing and only subkeys are present. These
incomplete keys are useful in scenarios where a user distributes
various subkeys to individual places (e.g. various build servers)
while retaining the master key.
The extension is described at: http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
Fixes golang/go#13605
Change-Id: I901d7c902d255a6b73a787a6b2a9c6181c4de048
---
M openpgp/errors/errors.go
M openpgp/keys_test.go
M openpgp/packet/encrypted_key.go
M openpgp/packet/private_key.go
M openpgp/packet/signature.go
M openpgp/packet/symmetric_key_encrypted.go
M openpgp/s2k/s2k.go
M openpgp/s2k/s2k_test.go
8 files changed, 127 insertions(+), 21 deletions(-)
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Adam Langley (Gerrit)
Adam Langley uploaded patch set #3 to the change originally created by Péter Szilágyi.
openpgp: support GNU dummy S2K for missing private keys
GNU defines an extension to the S2K algorithms where the private
key of a PGP is missing and only subkeys are present. These
incomplete keys are useful in scenarios where a user distributes
various subkeys to individual places (e.g. various build servers)
while retaining the master key.
The extension is described at: http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
Fixes golang/go#13605
Change-Id: I901d7c902d255a6b73a787a6b2a9c6181c4de048
---
M openpgp/errors/errors.go
M openpgp/keys_test.go
M openpgp/packet/encrypted_key.go
M openpgp/packet/private_key.go
M openpgp/packet/signature.go
M openpgp/packet/symmetric_key_encrypted.go
M openpgp/s2k/s2k.go
M openpgp/s2k/s2k_test.go
8 files changed, 126 insertions(+), 20 deletions(-)
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Adam Langley (Gerrit)
Please see whether this version meets your needs.
Patch set 3:Run-TryBot +1
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=e1691ad5
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 3:TryBot-Result +1
Brad Fitzpatrick (Gerrit)
1 comment:
Patch Set #3, Line 160:
func Parse(r io.Reader) (f func(out, in []byte), keyIsMissing bool, err error) {what about keeping this signature unchanged, and adding a new Parse variant with this signature?
Then this one would wrap the full one, but return an error on keyIsMissing?
Or, easier: keep this signature unchanged, don't add anything new, but document that for the GNU extension, the err value will be ErrGNUNoPrivateKey or something?
Then callers unprepared for a nil f won't be surprised on err == nil, but will still get the same information out of the function.
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Adam Langley (Gerrit)
1 comment:
Patch Set #3, Line 160:
func Parse(r io.Reader) (f func(out, in []byte), keyIsMissing bool, err error) {
what about keeping this signature unchanged, and adding a new Parse variant with this signature? […]
Sure, but I'm assuming that nobody is calling this package outside of the rest of the openpgp code. Is there a suggestion that's not true? (GitHub doesn't seem to do site-wide search any longer.)
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
I see that this review has no activity in the past 1.5 years. I'm curious if anyone is working to get this merged. This patch is needed for https://go-review.googlesource.com/c/crypto/+/161817 which supports exporting only subkeys and has issues due to missing dummy S2K support. It would be great if anyone could point me to any reviewers who can help this get merged.
Shashank Yadav (Gerrit)
I cherry-picked this patch, fixed the merge conflict and added support to PrivateKey.Serialize() for serialising Stub private keys compatible with gnupg in my fork on github (https://github.com/syadav2015/crypto/tree/dummy-s2k).
@Peter It would be great if you could let me know on whether you are planning to incorporate the changes from my fork into this review and have this review merge or should I go ahead and open a new review with contents from my fork?
Péter Szilágyi (Gerrit)
Péter Szilágyi abandoned this change.
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
Shashank Yadav has uploaded this change for review.
openpgp: support GNU dummy S2K for missing private keys
GNU defines an extension to the S2K algorithms where the private
key of a PGP is missing and only subkeys are present. These
incomplete keys are useful in scenarios where a user distributes
various subkeys to individual places (e.g. various build servers)
while retaining the master key. Picking up https://go-review.googlesource.com/c/crypto/+/32797/
after it was abandoned.
The extension is described at: http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
Change-Id: I2de52a4f86b113eb7debf92579a6f79836d86c3c
---
M openpgp/errors/errors.go
M openpgp/keys.go
M openpgp/keys_data_test.go
M openpgp/keys_test.go
M openpgp/packet/encrypted_key.go
M openpgp/packet/private_key.go
M openpgp/packet/signature.go
M openpgp/packet/symmetric_key_encrypted.go
M openpgp/s2k/s2k.go
M openpgp/s2k/s2k_test.go
10 files changed, 183 insertions(+), 41 deletions(-)
diff --git a/openpgp/errors/errors.go b/openpgp/errors/errors.go
index eb0550b..682dc49 100644
--- a/openpgp/errors/errors.go
+++ b/openpgp/errors/errors.go
@@ -6,6 +6,7 @@
package errors // import "golang.org/x/crypto/openpgp/errors"
import (
+ "errors"
"strconv"
)
@@ -70,3 +71,8 @@
func (upte UnknownPacketTypeError) Error() string {
return "openpgp: unknown packet type: " + strconv.Itoa(int(upte))
}
+
+// ErrStubPrivateKey results when operations are attempted on a private key
+// that is just a stub. See
+// https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
+var ErrStubPrivateKey = errors.New("openpgp: private key is a stub")
diff --git a/openpgp/keys.go b/openpgp/keys.go
index faa2fb3..0495488 100644
--- a/openpgp/keys.go
+++ b/openpgp/keys.go
@@ -599,9 +599,11 @@
if err != nil {
return
}
- err = ident.SelfSignature.SignUserId(ident.UserId.Id, e.PrimaryKey, e.PrivateKey, config)
- if err != nil {
- return
+ if !e.PrivateKey.Stub {
+ err = ident.SelfSignature.SignUserId(ident.UserId.Id, e.PrimaryKey, e.PrivateKey, config)
+ if err != nil {
+ return
+ }
}
err = ident.SelfSignature.Serialize(w)
if err != nil {
@@ -613,9 +615,11 @@
if err != nil {
return
}
- err = subkey.Sig.SignKey(subkey.PublicKey, e.PrivateKey, config)
- if err != nil {
- return
+ if !e.PrivateKey.Stub {
+ err = subkey.Sig.SignKey(subkey.PublicKey, e.PrivateKey, config)
+ if err != nil {
+ return
+ }
}
err = subkey.Sig.Serialize(w)
if err != nil {
diff --git a/openpgp/keys_data_test.go b/openpgp/keys_data_test.go
index 7779bd9..59f5fe0 100644
--- a/openpgp/keys_data_test.go
+++ b/openpgp/keys_data_test.go
@@ -198,3 +198,33 @@
=bNRo
diff --git a/openpgp/keys_test.go b/openpgp/keys_test.go
index 0eb1a9e..64dc299 100644
--- a/openpgp/keys_test.go
+++ b/openpgp/keys_test.go
@@ -132,6 +132,42 @@
}
}
+func TestGNUDummyPrivateKey(t *testing.T) {
+ // This public key has a signing subkey, but has a dummy placeholder
+ // instead of the real private key. It's used in scenarios where the
+ // main private key is withheld and only signing is allowed (e.g. build
+ // servers).
+ keys, err := ReadArmoredKeyRing(bytes.NewBufferString(onlySubkeyNoPrivateKey))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if len(keys) != 1 {
+ t.Errorf("Failed to accept key with dummy private key, %d", len(keys))
+ }
+ if !keys[0].PrivateKey.Stub {
+ t.Errorf("Primary private key should be marked as a stub")
+ }
+ if len(keys[0].Subkeys) != 1 {
+ t.Errorf("Failed to accept good subkey, %d", len(keys[0].Subkeys))
+ }
+
+ // Test serialization of stub private key via entity.SerializePrivate().
+ serializedEntity := bytes.NewBuffer(nil)
+ keys[0].SerializePrivate(serializedEntity, nil)
+
+ entity, err := ReadEntity(packet.NewReader(bytes.NewBuffer(serializedEntity.Bytes())))
+ if err != nil {
+ t.Fatal(err)
+ }
+
+ if !entity.PrivateKey.Stub {
+ t.Errorf("Primary private key should be marked as a stub after serialisation")
+ }
+ if len(entity.Subkeys) != 1 {
+ t.Errorf("Failed to accept good subkey, %d", len(keys[0].Subkeys))
+ }
+}
+
// TestExternallyRevokableKey attempts to load and parse a key with a third party revocation permission.
func TestExternallyRevocableKey(t *testing.T) {
kring, err := ReadKeyRing(readerFromHex(subkeyUsageHex))
diff --git a/openpgp/packet/encrypted_key.go b/openpgp/packet/encrypted_key.go
index 02b372c..cd104d2 100644
--- a/openpgp/packet/encrypted_key.go
+++ b/openpgp/packet/encrypted_key.go
@@ -71,6 +71,10 @@
// private key must have been decrypted first.
// If config is nil, sensible defaults will be used.
func (e *EncryptedKey) Decrypt(priv *PrivateKey, config *Config) error {
+ if priv.Stub {
+ return errors.ErrStubPrivateKey
+ }
+
var err error
var b []byte
diff --git a/openpgp/packet/private_key.go b/openpgp/packet/private_key.go
index 6f8ec09..59b151c 100644
--- a/openpgp/packet/private_key.go
+++ b/openpgp/packet/private_key.go
@@ -28,6 +28,7 @@
type PrivateKey struct {
PublicKey
Encrypted bool // if true then the private key is unavailable until Decrypt has been called.
+ Stub bool // indicates that the private key is a stub. This is a GNU extension.
encryptedData []byte
cipher CipherFunction
s2k func(out, in []byte)
@@ -109,11 +110,15 @@
return
}
pk.cipher = CipherFunction(buf[0])
- pk.Encrypted = true
- pk.s2k, err = s2k.Parse(r)
- if err != nil {
+ var missingKey bool
+ if pk.s2k, missingKey, err = s2k.Parse(r); err != nil {
return
}
+ if missingKey {
+ pk.Stub = true
+ return
+ }
+ pk.Encrypted = true
if s2kType == 254 {
pk.sha1Checksum = true
}
@@ -160,26 +165,39 @@
if err != nil {
return
}
- buf.WriteByte(0 /* no encryption */)
+ if pk.Stub {
+ b := []byte{255, 0, 101, 0}
+ b = append(b, []byte("GNU")...)
+ b = append(b, 1)
+ buf.Write(b)
+ } else {
+ buf.WriteByte(0 /* no encryption */)
+ }
privateKeyBuf := bytes.NewBuffer(nil)
- switch priv := pk.PrivateKey.(type) {
- case *rsa.PrivateKey:
- err = serializeRSAPrivateKey(privateKeyBuf, priv)
- case *dsa.PrivateKey:
- err = serializeDSAPrivateKey(privateKeyBuf, priv)
- case *elgamal.PrivateKey:
- err = serializeElGamalPrivateKey(privateKeyBuf, priv)
- case *ecdsa.PrivateKey:
- err = serializeECDSAPrivateKey(privateKeyBuf, priv)
- default:
- err = errors.InvalidArgumentError("unknown private key type")
- }
- if err != nil {
- return
+ if !pk.Stub {
+ switch priv := pk.PrivateKey.(type) {
+ case *rsa.PrivateKey:
+ err = serializeRSAPrivateKey(privateKeyBuf, priv)
+ case *dsa.PrivateKey:
+ err = serializeDSAPrivateKey(privateKeyBuf, priv)
+ case *elgamal.PrivateKey:
+ err = serializeElGamalPrivateKey(privateKeyBuf, priv)
+ case *ecdsa.PrivateKey:
+ err = serializeECDSAPrivateKey(privateKeyBuf, priv)
+ default:
+ err = errors.InvalidArgumentError("unknown private key type")
+ }
+ if err != nil {
+ return
+ }
}
+ if pk.Stub {
+ // Do not write any private key data for a stub key.
+ privateKeyBuf = bytes.NewBuffer(nil)
+ }
ptype := packetTypePrivateKey
contents := buf.Bytes()
privateKeyBytes := privateKeyBuf.Bytes()
@@ -194,6 +212,9 @@
if err != nil {
return
}
+ if pk.Stub {
+ return
+ }
_, err = w.Write(privateKeyBytes)
if err != nil {
return
@@ -238,6 +259,9 @@
// Decrypt decrypts an encrypted private key using a passphrase.
func (pk *PrivateKey) Decrypt(passphrase []byte) error {
+ if pk.Stub {
+ return errors.ErrStubPrivateKey
+ }
if !pk.Encrypted {
return nil
}
diff --git a/openpgp/packet/signature.go b/openpgp/packet/signature.go
index b2a24a5..5181796 100644
--- a/openpgp/packet/signature.go
+++ b/openpgp/packet/signature.go
@@ -509,6 +509,9 @@
// On success, the signature is stored in sig. Call Serialize to write it out.
// If config is nil, sensible defaults will be used.
func (sig *Signature) Sign(h hash.Hash, priv *PrivateKey, config *Config) (err error) {
+ if priv.Stub {
+ return errors.ErrStubPrivateKey
+ }
sig.outSubpackets = sig.buildSubpackets()
digest, err := sig.signPrepareHash(h)
if err != nil {
@@ -576,6 +579,9 @@
// Serialize to write it out.
// If config is nil, sensible defaults will be used.
func (sig *Signature) SignUserId(id string, pub *PublicKey, priv *PrivateKey, config *Config) error {
+ if priv.Stub {
+ return errors.ErrStubPrivateKey
+ }
h, err := userIdSignatureHash(id, pub, sig.Hash)
if err != nil {
return err
@@ -587,6 +593,9 @@
// success, the signature is stored in sig. Call Serialize to write it out.
// If config is nil, sensible defaults will be used.
func (sig *Signature) SignKey(pub *PublicKey, priv *PrivateKey, config *Config) error {
+ if priv.Stub {
+ return errors.ErrStubPrivateKey
+ }
h, err := keySignatureHash(&priv.PublicKey, pub, sig.Hash)
if err != nil {
return err
diff --git a/openpgp/packet/symmetric_key_encrypted.go b/openpgp/packet/symmetric_key_encrypted.go
index 744c2d2..7f233ee 100644
--- a/openpgp/packet/symmetric_key_encrypted.go
+++ b/openpgp/packet/symmetric_key_encrypted.go
@@ -44,11 +44,15 @@
}
var err error
- ske.s2k, err = s2k.Parse(r)
- if err != nil {
+ var missingKey bool
+ if ske.s2k, missingKey, err = s2k.Parse(r); err != nil {
return err
}
+ if missingKey {
+ return errors.UnsupportedError("missing key GNU extension in session key")
+ }
+
encryptedKey := make([]byte, maxSessionKeySizeInBytes)
// The session key may follow. We just have to try and read to find
// out. If it exists then we limit it to maxSessionKeySizeInBytes.
diff --git a/openpgp/s2k/s2k.go b/openpgp/s2k/s2k.go
index 4b9a44c..bbf5449 100644
--- a/openpgp/s2k/s2k.go
+++ b/openpgp/s2k/s2k.go
@@ -151,9 +151,13 @@
}
}
-// Parse reads a binary specification for a string-to-key transformation from r
-// and returns a function which performs that transform.
-func Parse(r io.Reader) (f func(out, in []byte), err error) {
+// Parse reads a binary specification for a string-to-key transformation from r.
+// It returns either:
+// nil, false, err — on error.
+// nil, true, nil — if the S2K is a special GNU extension that indicates that
+// the private key is missing.
+// f, false, nil — f is a function that performs the S2K transformation.
+func Parse(r io.Reader) (f func(out, in []byte), keyIsMissing bool, err error) {
var buf [9]byte
_, err = io.ReadFull(r, buf[:2])
@@ -161,12 +165,24 @@
return
}
+ if buf[0] == 101 {
+ // This is a GNU extension. See
+ // https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
+ if _, err = io.ReadFull(r, buf[:4]); err != nil {
+ return nil, false, err
+ }
+ if buf[0] == 'G' && buf[1] == 'N' && buf[2] == 'U' && buf[3] == 1 {
+ return nil, true, nil
+ }
+ return nil, false, errors.UnsupportedError("GNU S2K extension")
+ }
+
hash, ok := HashIdToHash(buf[1])
if !ok {
- return nil, errors.UnsupportedError("hash for S2K function: " + strconv.Itoa(int(buf[1])))
+ return nil, false, errors.UnsupportedError("hash for S2K function: " + strconv.Itoa(int(buf[1])))
}
if !hash.Available() {
- return nil, errors.UnsupportedError("hash not available: " + strconv.Itoa(int(hash)))
+ return nil, false, errors.UnsupportedError("hash not available: " + strconv.Itoa(int(hash)))
}
h := hash.New()
@@ -175,7 +191,7 @@
f := func(out, in []byte) {
Simple(out, h, in)
}
- return f, nil
+ return f, false, nil
case 1:
_, err = io.ReadFull(r, buf[:8])
if err != nil {
@@ -184,7 +200,7 @@
f := func(out, in []byte) {
Salted(out, h, in, buf[:8])
}
- return f, nil
+ return f, false, nil
case 3:
_, err = io.ReadFull(r, buf[:9])
if err != nil {
@@ -194,10 +210,10 @@
f := func(out, in []byte) {
Iterated(out, h, in, buf[:8], count)
}
- return f, nil
+ return f, false, nil
}
- return nil, errors.UnsupportedError("S2K function")
+ return nil, false, errors.UnsupportedError("S2K function")
}
// Serialize salts and stretches the given passphrase and writes the
diff --git a/openpgp/s2k/s2k_test.go b/openpgp/s2k/s2k_test.go
index 183d260..aa5d4c5 100644
--- a/openpgp/s2k/s2k_test.go
+++ b/openpgp/s2k/s2k_test.go
@@ -70,25 +70,34 @@
var parseTests = []struct {
spec, in, out string
+ missingKey bool
}{
/* Simple with SHA1 */
- {"0002", "hello", "aaf4c61d"},
+ {"0002", "hello", "aaf4c61d", false},
/* Salted with SHA1 */
- {"01020102030405060708", "hello", "f4f7d67e"},
+ {"01020102030405060708", "hello", "f4f7d67e", false},
/* Iterated with SHA1 */
- {"03020102030405060708f1", "hello", "f2a57b7c"},
+ {"03020102030405060708f1", "hello", "f2a57b7c", false},
+ /* GNU dummy S2K */
+ {"6502474e5501", "", "", true},
}
func TestParse(t *testing.T) {
for i, test := range parseTests {
spec, _ := hex.DecodeString(test.spec)
buf := bytes.NewBuffer(spec)
- f, err := Parse(buf)
+ f, missingKey, err := Parse(buf)
if err != nil {
t.Errorf("%d: Parse returned error: %s", i, err)
continue
}
-
+ if test.missingKey != missingKey {
+ t.Errorf("%d: missingKey should be %t, but got the opposite", i, test.missingKey)
+ continue
+ }
+ if missingKey {
+ continue
+ }
expected, _ := hex.DecodeString(test.out)
out := make([]byte, len(expected))
f(out, []byte(test.in))
@@ -124,7 +133,7 @@
return
}
- f, err := Parse(buf)
+ f, _, err := Parse(buf)
if err != nil {
t.Errorf("failed to reparse: %s", err)
return
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
Just for reference, changes from this review are now planned to be merged via https://go-review.googlesource.com/c/crypto/+/191658.
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Huigens (Gerrit)
6 comments:
File openpgp/keys_data_test.go:
Patch Set #1, Line 202:
const onlySubkeyNoPrivateKey = `-----BEGIN PGP PRIVATE KEY BLOCK-----Nit: perhaps name this gnuDummyPrivateKey or some such as well?
Patch Set #1, Line 158:
entity, err := ReadEntity(packet.NewReader(bytes.NewBuffer(serializedEntity.Bytes())))Perhaps you could try to serialize and parse the entire key, instead of just this packet, in order to catch the `+2` bug below? (And/or compare the serialized key with the original key, if it comes out identical.)
File openpgp/packet/private_key.go:
Patch Set #1, Line 32:
encryptedData []byteVery nitpicky, but: `Encrypted` and `encryptedData` are sort of a pair of fields; I wouldn't add `Stub` between them, but perhaps after `s2k`.
Also, I'd call it `Dummy` instead, as I think the term "gnu-dummy" is more common for these keys.
This `if` block is not necessary, `privateKeyBuf` already has that value. The comment is useful though IMO, you could move it above the previous `if` block.
Patch Set #1, Line 207:
err = serializeHeader(w, ptype, len(contents)+len(privateKeyBytes)+2)For a GNU-dummy key, this `+2` seems incorrect, since you're not writing the checksum below.
Patch Set #1, Line 160:
func Parse(r io.Reader) (f func(out, in []byte), keyIsMissing bool, err error) {Although this function is probably not used much outside openpgp/, it's still part of the public API, so it might be preferable to return `(nil, errors.ErrStubPrivateKey)` for GNU-dummy keys here instead of changing the signature.
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
Shashank Yadav uploaded patch set #2 to this change.
openpgp: support GNU dummy S2K for missing private keys
GNU defines an extension to the S2K algorithms where the private
key of a PGP is missing and only subkeys are present. These
incomplete keys are useful in scenarios where a user distributes
various subkeys to individual places (e.g. various build servers)
while retaining the master key. Picking up https://go-review.googlesource.com/c/crypto/+/32797/
after it was abandoned.
The extension is described at: http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
Change-Id: I2de52a4f86b113eb7debf92579a6f79836d86c3c
---
M openpgp/errors/errors.go
M openpgp/keys.go
M openpgp/keys_data_test.go
M openpgp/keys_test.go
M openpgp/packet/encrypted_key.go
M openpgp/packet/private_key.go
M openpgp/packet/signature.go
M openpgp/packet/symmetric_key_encrypted.go
M openpgp/s2k/s2k.go
M openpgp/s2k/s2k_test.go
10 files changed, 190 insertions(+), 34 deletions(-)
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
6 comments:
Patch Set #1, Line 202:
const onlySubkeyNoPrivateKey = `-----BEGIN PGP PRIVATE KEY BLOCK-----Nit: perhaps name this gnuDummyPrivateKey or some such as well?
All the other keys in the file seem to be named according to the testcase they are intended for. I'm planning to follow the same since it is helpful in associating the keys with testcases.
Patch Set #1, Line 158:
entity, err := ReadEntity(packet.NewReader(bytes.NewBuffer(serializedEntity.Bytes())))
Perhaps you could try to serialize and parse the entire key, instead of just this packet, in order t […]
We cannot check for exact equality between serialized and original keyrings because the Serialize* methods in the library re-sign identities and signatures leading to the output be different. That's why we check for the contents of the key rings to make sure contents are as expected. Otherwise modified the test to serialise and deserialise an armored key.
File openpgp/packet/private_key.go:
Patch Set #1, Line 32:
encryptedData []byte
Very nitpicky, but: `Encrypted` and `encryptedData` are sort of a pair of fields; I wouldn't add `St […]
Done
This `if` block is not necessary, `privateKeyBuf` already has that value. […]
Done
Patch Set #1, Line 207:
err = serializeHeader(w, ptype, len(contents)+len(privateKeyBytes)+2)For a GNU-dummy key, this `+2` seems incorrect, since you're not writing the checksum below.
Patch Set #1, Line 160:
func Parse(r io.Reader) (f func(out, in []byte), keyIsMissing bool, err error) {
Although this function is probably not used much outside openpgp/, it's still part of the public API […]
Done
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
paul deauna (Gerrit)
fixed it already? kindly check
To view, visit change 32797. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
Patch Set 3:
fixed it already? kindly check
The review was abandoned in favour of https://go-review.googlesource.com/c/crypto/+/191658 and is actively being reviewed.
Shashank Yadav (Gerrit)
Shashank Yadav uploaded patch set #3 to this change.
openpgp: support GNU dummy S2K for missing private keys
GNU defines an extension to the S2K algorithms where the private
key of a PGP is missing and only subkeys are present. These
incomplete keys are useful in scenarios where a user distributes
various subkeys to individual places (e.g. various build servers)
while retaining the master key. Picking up https://go-review.googlesource.com/c/crypto/+/32797/
after it was abandoned.
The extension is described at: http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
Change-Id: I2de52a4f86b113eb7debf92579a6f79836d86c3c
---
M openpgp/errors/errors.go
M openpgp/keys.go
M openpgp/keys_data_test.go
M openpgp/keys_test.go
M openpgp/packet/encrypted_key.go
M openpgp/packet/private_key.go
M openpgp/packet/signature.go
M openpgp/packet/symmetric_key_encrypted.go
M openpgp/s2k/s2k.go
M openpgp/s2k/s2k_test.go
10 files changed, 190 insertions(+), 34 deletions(-)
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Huigens (Gerrit)
3 comments:
Patch Set #3, Line 603:
err = ident.SelfSignature.SignUserId(ident.UserId.Id, e.PrimaryKey, e.PrivateKey, config)Adding this `if` (and the next one) scares me slightly. The comment above this functions says,
Identities and subkeys are re-signed in case they changed since NewEntry. (sic)
So, users of this function have the expectation that if, let's say, they change `ident.SelfSignature.KeyLifetimeSecs`, and then call `entity.SerializePrivate()`, their change will have effect. Now for a dummy key it won't, and there won't be any indication or warning that it didn't. This may have security implications (the key may expire later than they expect). So I would just leave this function as-is and return an error for dummy keys.
If you still need a way to serialize them, I would add a separate function that's very explicit in what it does. I see a few options for that:
- `SerializePrivateWithoutSigning` (might have to come up with a better name), which could be more generally useful for non-dummy keys as well for performance reasons, since `NewEntity` already signs everything, so there's no need to sign things again
- `SerializePrivateSubkeys`, which would export the primary key as a dummy key even if it isn't currently, similar to the `gpg --export-secret-subkeys` option that started this all
- Both of the above
- None of the above; since all the functions that `SerializePrivate` calls are exported, you could just write a function in your own code that does exactly what you need and don't add it to this PR; although I personally think both of the above could be useful for others.
File openpgp/packet/private_key.go:
Patch Set #3, Line 34:
GnuDummy bool // if true then the private key is a dummy key. This is a GNU extension.I know I said these keys are often referred to as "gnu-dummy", but I wouldn't refer to "gnu" in the exported name here. If this extension ever gets added to the spec itself (and it seems useful enough to do so) then we'd have to change this name again. Similarly for `ErrGnuDummyPrivateKey`, I'd call it `ErrDummyPrivateKey` instead.
This is called either packetLength or pLength elsewhere, I'd follow that. Also, instead of duplicating the calculation, you could do:
packetLength := len(contents)+len(privateKeyBytes)
if !pk.Dummy {
packetLength += 2
}
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
Shashank Yadav uploaded patch set #4 to this change.
openpgp: support GNU dummy S2K for missing private keys
GNU defines an extension to the S2K algorithms where the private
key of a PGP is missing and only subkeys are present. These
incomplete keys are useful in scenarios where a user distributes
various subkeys to individual places (e.g. various build servers)
while retaining the master key. Picking up https://go-review.googlesource.com/c/crypto/+/32797/
after it was abandoned.
The extension is described at: http://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=fe55ae16ab4e26d8356dc574c9e8bc935e71aef1;hb=23191d7851eae2217ecdac6484349849a24fd94a#l1109
Change-Id: I2de52a4f86b113eb7debf92579a6f79836d86c3c
---
M openpgp/errors/errors.go
M openpgp/keys.go
M openpgp/keys_data_test.go
M openpgp/keys_test.go
M openpgp/packet/encrypted_key.go
M openpgp/packet/private_key.go
M openpgp/packet/signature.go
M openpgp/packet/symmetric_key_encrypted.go
M openpgp/s2k/s2k.go
M openpgp/s2k/s2k_test.go
10 files changed, 211 insertions(+), 39 deletions(-)
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
Thanks for the review, Daniel. I've uploaded a new patch set that addresses the comments.
3 comments:
Patch Set #3, Line 603:
err = ident.SelfSignature.SignUserId(ident.UserId.Id, e.PrimaryKey, e.PrivateKey, config)
Adding this `if` (and the next one) scares me slightly. The comment above this functions says, […]
I agree with the idea of removing the if from here to preserve the original intention of the API. I'm onboard with having a SerializePrivateWithoutSigning. I had to implement this in one of the libraries I'm working on and this method is needed for the unit test written for verifying proper serialization for dummy keys. IMO SerializePrivateSubkeys is not needed as the output of SerializePrivateWithoutSigning and SerializePrivateSubkeys should be the same for a dummy key.
File openpgp/packet/private_key.go:
Patch Set #3, Line 34:
GnuDummy bool // if true then the private key is a dummy key. This is a GNU extension.
I know I said these keys are often referred to as "gnu-dummy", but I wouldn't refer to "gnu" in the […]
Agreed. Changed GnuDummy to Dummy.
This is called either packetLength or pLength elsewhere, I'd follow that. […]
Done
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Shashank Yadav (Gerrit)
1 comment:
Patch Set #3, Line 603:
err = ident.SelfSignature.SignUserId(ident.UserId.Id, e.PrimaryKey, e.PrivateKey, config)I agree with the idea of removing the if from here to preserve the original intention of the API. […]
Done
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Huigens (Gerrit)
Thanks for addressing all the comments, looks good to me!
Patch set 4:Code-Review +1
Filippo Valsorda (Gerrit)
Filippo Valsorda abandoned this change.
To view, visit change 191658. To unsubscribe, or for help writing mail filters, visit settings.