feat(cmds): add cleartext PEM/PKCS8 for key import/export

[schomatis]

The main blocker of this PR that needs to be decided on is how to implement key encryption during export:

Encryption at the PEM layer has been deprecated. Instead, encryption at the PKCS8 layer is recommneded but it is currently not supported. There is a third-party implementation available but we should decide if we can rely on it.

Implements unencrypted PEM support described in #8594.

[lidel]

Thank you for taking a stab at this! What a rabbit hole 🕳️

PKCS8 encryption state in golang is unfortunate.
I did some poking around Ed25519 (cyrrent default key type) and js-ipfs side of things and those also make it difficult to come up with something that covers all needs. Elaborated on each, but you can just jump to my suggestion to reduce the scope at the end of this comment.

Bummer: js-ipfs uses a custom export format for Ed25519 keys

TLDR: js-ipfs (ipfs-core v0.13) encrypts Ed25519 keys using using aes-128-gcm and then wraps them in base64, which is a custom format, not PEM. They use PEM only for RSA keys.

Click to expand details

Seems that in js-ipfs, PEM export format was only used for RSA, and since there was no convention for Ed25519 keys at the time, those are exported in custom 'libp2p-key' format (docs):

RSA keys will be exported as password protected PEM by default. Ed25519 and Secp256k1 keys will be exported as password protected AES-GCM base64 encoded strings ('libp2p-key' format).

I did some digging and low level JS code that does encrypted keys resides in js-libp2p-crypto/src/keys/exporter.ts and importer.ts, which in turn use aes-gcm encryption code from encrypt/decrypt in js-libp2p-crypto/src/ciphers/aes-gcm.ts.

In general, JS side of things looks half-baked for Ed25519, and we most likely need to add PEM support there too at some point. It should not be a template for go-ipfs work.

Possible next steps

I think there are different levels of improvements we can ship here.
In order of importance:

1. MVP is to make it possible to import/export keys in PEM format (cleartext).

   • This is the most important imo. It does not need to be encrypted. It does not need to be the default. Could be opt-in via --format pem-cleartext flag, that is fine: we want to support a standard for interop.
   • If we do this, users will be able to import key produced by openssl genpkey -algorithm ED25519 > ed25519.pem and reading public key via openssl pkey -in ed25519.pem -pubout should work on a PEM exported from go-ipfs.

2. Change the default format to be encrypted PEM (PKCS8?)

   • This makes sense only if a no-brainer industry standard for encrypting ed25519 exists and has wide support. It sounds like it may be a good idea to wait for golang stdlib instead of pulling in third party libraries. In the meantime, users can pipe to gpg or similar tool, if encryption at rest is paramount.

3. Key import/export interop with js-ipfs.

   • This is "nice to have". Ideally, users should be able to move encrypted keys between go and js implementations, at least for the default key type (Ed25519).
   • I hoped that js-ipfs uses PEM/PKCS8 standard for all key types, but it does not seem to be the case. For non-RSA keys it uses custom key encryption based on aes-gcm (details above), which is not PEM. I don't think go-ipfs should implement another custom key format.
   • If we care about interop between go-ipfs and js-ipfs key formats, then js-ipfs should implement pem-cleartext from (1) or some industry standard for (2).

Proposed scope for this PR

@schomatis I suggest we reduce the scope of this PR and only do (1) – pem-cleartext – as opt-in feature via --format flag for now, and keep the default behavior as-is. Remove the encrypted formats.

Rationale: (2) and (3) require more work than anticipated, and it is better to spend the time elsewhere.

[schomatis]

Pasting here the current diff before applying changes above just to have a backup in case we want to reincorporate some it later.

Diff with WIP PKCS8

diff --git a/core/commands/keystore.go b/core/commands/keystore.go
index bd3146ca5..372929222 100644
--- a/core/commands/keystore.go
+++ b/core/commands/keystore.go
@@ -2,6 +2,10 @@ package commands
 
 import (
 	"bytes"
+	"crypto/ed25519"
+	"crypto/rand"
+	"crypto/x509"
+	"encoding/pem"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -135,6 +139,15 @@ var keyGenCmd = &cmds.Command{
 	Type: KeyOutput{},
 }
 
+const (
+	// Key format options used both for importing and exporting.
+	keyFormatOptionName             = "format"
+	keyFormatPemEncryptedOption     = "pem-pkcs8-encrypted"
+	keyFormatPemCleartextOption     = "pem-pkcs8-cleartext"
+	keyFormatLibp2pCleartextOption  = "libp2p-protobuf-cleartext"
+	keyEncryptionPasswordOptionName = "password"
+)
+
 var keyExportCmd = &cmds.Command{
 	Helptext: cmds.HelpText{
 		Tagline: "Export a keypair",
@@ -150,6 +163,10 @@ path can be specified with '--output=<path>' or '-o=<path>'.
 	},
 	Options: []cmds.Option{
 		cmds.StringOption(outputOptionName, "o", "The path where the output should be stored."),
+		cmds.StringOption(keyFormatOptionName, "f", "The format of the exported private key.").WithDefault(keyFormatLibp2pCleartextOption),
+		cmds.StringOption(keyEncryptionPasswordOptionName, "p", "The password to encrypt the exported key with (for the encrypted variant only)."),
+		// FIXME(BLOCKING): change default to keyFormatPemEncryptedOption once it
+		//  is implemented and the sharness tests (if any) are adapted.
 	},
 	NoRemote: true,
 	Run: func(req *cmds.Request, res cmds.ResponseEmitter, env cmds.Environment) error {
@@ -186,12 +203,38 @@ path can be specified with '--output=<path>' or '-o=<path>'.
 			return fmt.Errorf("key with name '%s' doesn't exist", name)
 		}
 
-		encoded, err := crypto.MarshalPrivateKey(sk)
-		if err != nil {
-			return err
+		exportFormat, _ := req.Options[keyFormatOptionName].(string)
+		var formattedKey []byte
+		switch exportFormat {
+		case keyFormatPemEncryptedOption, keyFormatPemCleartextOption:
+			stdKey, err := crypto.PrivKeyToStdKey(sk)
+			if err != nil {
+				return fmt.Errorf("converting libp2p private key to std Go key: %w", err)
+
+			}
+			// For some reason the ed25519.PrivateKey does not use pointer
+			// receivers, so we need to convert it for MarshalPKCS8PrivateKey.
+			// (We should probably change this upstream in PrivKeyToStdKey).
+			if ed25519KeyPointer, ok := stdKey.(*ed25519.PrivateKey); ok {
+				stdKey = *ed25519KeyPointer
+			}
+			// This function supports a restricted list of public key algorithms,
+			// but we generate and use only the RSA and ed25519 types that are on that list.
+			formattedKey, err = x509.MarshalPKCS8PrivateKey(stdKey)
+			if err != nil {
+				return fmt.Errorf("marshalling key to PKCS8 format: %w", err)
+			}
+
+		case keyFormatLibp2pCleartextOption:
+			formattedKey, err = crypto.MarshalPrivateKey(sk)
+			if err != nil {
+				return err
+			}
+		default:
+			return fmt.Errorf("unrecognized export format: %s", exportFormat)
 		}
 
-		return res.Emit(bytes.NewReader(encoded))
+		return res.Emit(bytes.NewReader(formattedKey))
 	},
 	PostRun: cmds.PostRunMap{
 		cmds.CLI: func(res cmds.Response, re cmds.ResponseEmitter) error {
@@ -208,8 +251,16 @@ path can be specified with '--output=<path>' or '-o=<path>'.
 			}
 
 			outPath, _ := req.Options[outputOptionName].(string)
+			exportFormat, _ := req.Options[keyFormatOptionName].(string)
 			if outPath == "" {
-				trimmed := strings.TrimRight(fmt.Sprintf("%s.key", req.Arguments[0]), "/")
+				var fileExtension string
+				switch exportFormat {
+				case keyFormatPemEncryptedOption, keyFormatPemCleartextOption:
+					fileExtension = "pem"
+				case keyFormatLibp2pCleartextOption:
+					fileExtension = "key"
+				}
+				trimmed := strings.TrimRight(fmt.Sprintf("%s.%s", req.Arguments[0], fileExtension), "/")
 				_, outPath = filepath.Split(trimmed)
 				outPath = filepath.Clean(outPath)
 			}
@@ -221,9 +272,46 @@ path can be specified with '--output=<path>' or '-o=<path>'.
 			}
 			defer file.Close()
 
-			_, err = io.Copy(file, outReader)
-			if err != nil {
-				return err
+			switch exportFormat {
+			case keyFormatPemEncryptedOption, keyFormatPemCleartextOption:
+				privKeyBytes, err := ioutil.ReadAll(outReader)
+				if err != nil {
+					return err
+				}
+
+				var pemBlock *pem.Block
+				if exportFormat == keyFormatPemEncryptedOption {
+					keyEncPassword, ok := req.Options[keyEncryptionPasswordOptionName].(string)
+					if !ok {
+						return fmt.Errorf("missing password to encrypt the key with, set it with --%s",
+							keyEncryptionPasswordOptionName)
+					}
+					// FIXME(BLOCKING): Using deprecated security function.
+					pemBlock, err = x509.EncryptPEMBlock(rand.Reader,
+						"ENCRYPTED PRIVATE KEY",
+						privKeyBytes,
+						[]byte(keyEncPassword),
+						x509.PEMCipherAES256)
+					if err != nil {
+						return fmt.Errorf("encrypting PEM block: %w", err)
+					}
+				} else { // cleartext
+					pemBlock = &pem.Block{
+						Type:  "PRIVATE KEY",
+						Bytes: privKeyBytes,
+					}
+				}
+
+				err = pem.Encode(file, pemBlock)
+				if err != nil {
+					return fmt.Errorf("encoding PEM block: %w", err)
+				}
+
+			case keyFormatLibp2pCleartextOption:
+				_, err = io.Copy(file, outReader)
+				if err != nil {
+					return err
+				}
 			}
 
 			return nil
@@ -237,6 +325,9 @@ var keyImportCmd = &cmds.Command{
 	},
 	Options: []cmds.Option{
 		ke.OptionIPNSBase,
+		cmds.StringOption(keyFormatOptionName, "f", "The format of the private key to import.").WithDefault(keyFormatLibp2pCleartextOption),
+		// FIXME: Attempt to figure out the import format.
+		cmds.StringOption(keyEncryptionPasswordOptionName, "p", "The password to decrypt the imported key with (for the encrypted variant only)."),
 	},
 	Arguments: []cmds.Argument{
 		cmds.StringArg("name", true, false, "name to associate with key in keychain"),
@@ -265,9 +356,59 @@ var keyImportCmd = &cmds.Command{
 			return err
 		}
 
-		sk, err := crypto.UnmarshalPrivateKey(data)
-		if err != nil {
-			return err
+		importFormat, _ := req.Options[keyFormatOptionName].(string)
+		var sk crypto.PrivKey
+		switch importFormat {
+		case keyFormatPemEncryptedOption, keyFormatPemCleartextOption:
+			pemBlock, rest := pem.Decode(data)
+			if pemBlock == nil {
+				return fmt.Errorf("PEM block not found in input data:\n%s", rest)
+			}
+
+			if pemBlock.Type != "PRIVATE KEY" && pemBlock.Type != "ENCRYPTED PRIVATE KEY" {
+				return fmt.Errorf("expected [ENCRYPTED] PRIVATE KEY type in PEM block but got: %s", pemBlock.Type)
+			}
+
+			var privKeyBytes []byte
+			if importFormat == keyFormatPemEncryptedOption {
+				keyDecPassword, ok := req.Options[keyEncryptionPasswordOptionName].(string)
+				if !ok {
+					return fmt.Errorf("missing password to decrypt the key with, set it with --%s",
+						keyEncryptionPasswordOptionName)
+				}
+				privKeyBytes, err = x509.DecryptPEMBlock(pemBlock,
+					[]byte(keyDecPassword))
+				if err != nil {
+					return fmt.Errorf("decrypting PEM block: %w", err)
+				}
+			} else { // cleartext
+				privKeyBytes = pemBlock.Bytes
+			}
+
+			stdKey, err := x509.ParsePKCS8PrivateKey(privKeyBytes)
+			if err != nil {
+				return fmt.Errorf("parsing PKCS8 format: %w", err)
+			}
+
+			// In case ed25519.PrivateKey is returned we need the pointer for
+			// conversion to libp2p (see export command for more details).
+			if ed25519KeyPointer, ok := stdKey.(ed25519.PrivateKey); ok {
+				stdKey = &ed25519KeyPointer
+			}
+
+			sk, _, err = crypto.KeyPairFromStdKey(stdKey)
+			if err != nil {
+				return fmt.Errorf("converting std Go key to libp2p key : %w", err)
+
+			}
+		case keyFormatLibp2pCleartextOption:
+			sk, err = crypto.UnmarshalPrivateKey(data)
+			if err != nil {
+				return err
+			}
+
+		default:
+			return fmt.Errorf("unrecognized import format: %s", importFormat)
 		}
 
 		cfgRoot, err := cmdenv.GetConfigRoot(env)
diff --git a/test/sharness/t0165-keystore.sh b/test/sharness/t0165-keystore.sh
index ad4b6a6c7..dad93dd29 100755
--- a/test/sharness/t0165-keystore.sh
+++ b/test/sharness/t0165-keystore.sh
@@ -63,24 +63,14 @@ ipfs key rm key_ed25519
     echo $rsahash > rsa_key_id
   '
 
+  test_key_import_export_all_formats rsa_key
+
   test_expect_success "create a new ed25519 key" '
     edhash=$(ipfs key gen generated_ed25519_key --type=ed25519)
     echo $edhash > ed25519_key_id
   '
 
-  test_expect_success "export and import rsa key" '
-    ipfs key export generated_rsa_key &&
-    ipfs key rm generated_rsa_key &&
-    ipfs key import generated_rsa_key generated_rsa_key.key > roundtrip_rsa_key_id &&
-    test_cmp rsa_key_id roundtrip_rsa_key_id
-  '
-
-  test_expect_success "export and import ed25519 key" '
-    ipfs key export generated_ed25519_key &&
-    ipfs key rm generated_ed25519_key &&
-    ipfs key import generated_ed25519_key generated_ed25519_key.key > roundtrip_ed25519_key_id &&
-    test_cmp ed25519_key_id roundtrip_ed25519_key_id
-  '
+  test_key_import_export_all_formats ed25519_key
 
   test_expect_success "test export file option" '
     ipfs key export generated_rsa_key -o=named_rsa_export_file &&
@@ -176,15 +166,13 @@ ipfs key rm key_ed25519
   '
 
   # export works directly on the keystore present in IPFS_PATH
-  test_expect_success "export and import ed25519 key while daemon is running" '
-    edhash=$(ipfs key gen exported_ed25519_key --type=ed25519)
+  test_expect_success "prepare ed25519 key while daemon is running" '
+    edhash=$(ipfs key gen generated_ed25519_key --type=ed25519)
     echo $edhash > ed25519_key_id
-    ipfs key export exported_ed25519_key &&
-    ipfs key rm exported_ed25519_key &&
-    ipfs key import exported_ed25519_key exported_ed25519_key.key > roundtrip_ed25519_key_id &&
-    test_cmp ed25519_key_id roundtrip_ed25519_key_id
   '
 
+  test_key_import_export_all_formats ed25519_key
+
   test_expect_success "key export over HTTP /api/v0/key/export is not possible" '
     ipfs key gen nohttpexporttest_key --type=ed25519 &&
     curl -X POST -sI "http://$API_ADDR/api/v0/key/export&arg=nohttpexporttest_key" | grep -q "^HTTP/1.1 404 Not Found"
@@ -214,6 +202,35 @@ test_check_ed25519_sk() {
   }
 }
 
+test_key_import_export_all_formats() {
+  KEY_NAME=$1
+  test_key_import_export $KEY_NAME pem-pkcs8-cleartext
+  test_key_import_export $KEY_NAME pem-pkcs8-encrypted
+  test_key_import_export $KEY_NAME libp2p-protobuf-cleartext
+}
+
+test_key_import_export() {
+  local KEY_NAME FORMAT
+  KEY_NAME=$1
+  FORMAT=$2
+  ORIG_KEY="generated_$KEY_NAME"
+  if [ $FORMAT == "pem-pkcs8-encrypted" ]; then
+    KEY_PASSWORD="--password=fake-test-password"
+  fi
+  if [ $FORMAT == "libp2p-protobuf-cleartext" ]; then
+    FILE_EXT="key"
+  else
+    FILE_EXT="pem"
+  fi
+
+  test_expect_success "export and import $KEY_NAME with format $FORMAT" '
+    ipfs key export $ORIG_KEY --format=$FORMAT $KEY_PASSWORD &&
+    ipfs key rm $ORIG_KEY &&
+    ipfs key import $ORIG_KEY $ORIG_KEY.$FILE_EXT --format=$FORMAT $KEY_PASSWORD > imported_key_id &&
+    test_cmp ${KEY_NAME}_id imported_key_id
+  '
+}
+
 test_key_cmd
 
 test_done

[schomatis]

@lidel Removed the encrypted variant and left the protobuf as default.

[lidel]

Thank you @schomatis!

• added helptext for CLI (needs 👁️) and a small hint that improvex UX when user tries to import PEM without passing --format.
• looks and works as expected 👍

@guseggert or @aschmahmann – ready for your final review.
This improves interop with other software and is fairly safe change (PEM support is opt-in, protobuf remains as the default). (mainly if helptext makes sense).

[schomatis]

This example is the best remainder of why we're doing this. It's pretty cool to be able to easily interoperate with openssl commands 👍

[aschmahmann]

LGTM. Thanks 🙏

[aschmahmann]

Wow, very weird that MarshalPKCS8PrivateKey has docs calling out ed25519 pointers as special:

The following key types are currently supported: *rsa.PrivateKey, *ecdsa.PrivateKey and ed25519.PrivateKey. Unsupported key types result in an error.

[aschmahmann]

We don't have to add openssl as a dependency, but we could add a test fixture here that verifies we're compatible by dumping a private key into the sharness test folder (like we have for CAR import/export tests). We could then do some round-tripping tests to make sure everything is as expected (e.g. openssl -> import -> export as libp2p -> import -> export as pem -> check equality).

Hopefully should be relatively straightforward, but if not it's not strictly necessary.

[lidel]

There is js-ipfs issue to support PEM: ipfs/js-ipfs#2887
When we have that resolved, go-ipfs and js-ipfs will have key import|export interop.