Add method to use ed25519 ssh key #104

slickwarren · 2023-05-27T00:19:18Z

addresses: #96

tested locally using ubuntu 20.04, and am able to create + login using an ed25519 key instead of rsa by having the user set coral_ssh_key_type

igomez06

looks great just one nitpick

cmd/create.go

igomez06

LGTM

slickwarren · 2023-06-10T00:29:22Z

I had to re-work some of this PR; my initial testing wasn't sufficient.
In this new approach, I needed to add a way to support OPENSSH keys, a specific type of PKSC8 key that can be recognized by ssh-keygen. Otherwise, if an actual user tries to use a generated key from their machine, the key is marked as malformatted, since most machines use ssh cli by default.

The only problem is, openssh creation / marshaling isn't supported upstream directly, so the package I'm using is 3rd party.

igomez06 · 2023-06-13T18:49:52Z

cmd/create.go

 	"sync"

+	"github.com/mikesmitty/edkey"


This mod hasn't been up modified in 6 years. I think it would be better to copy over the code so we can at least maintain it. Will refer to Jake though on what he thinks is best.

Even though the code is MIT we should never copy code wholesale without attribution. Also, we should prefer forking if a repo isn't properly maintained, and only if we plan to modify it in some way.

Sounds good Jake understood. Well in this instance the repo doesn't have a go.mod file, it's causing failures in the validate checks for the PR. Do you recommend forking and adding a go.mod for our purposes?

Did a little digging, according to the docs x509.MarshalPKCS8PrivateKey should work: https://cs.opensource.google/go/go/+/go1.20.5:src/crypto/x509/pkcs8.go;l=101

MarshalPKCS8PrivateKey converts a private key to PKCS #8, ASN.1 DER form.

The following key types are currently supported: *rsa.PrivateKey, *ecdsa.PrivateKey, ed25519.PrivateKey (not a pointer), and *ecdh.PrivateKey. Unsupported key types result in an error.

This way we can use the go standard library and not have an external dependency at all.

@slickwarren Circling back on this, were you able to test the above method?

I tried that at the time of original testing, but it was not working as expected for me. I can try this again after the upcoming release to make sure I didn't miss anything. But I worked at it for awhile before finding this alternative.

okay, it appears that this works if you cast the key to its expected type before sending to PKCS8, tested and working with both ed25519 and rsa keys now without the custom code

jakefhyde · 2023-08-18T21:46:03Z

cmd/create.go

+	var privDER []byte
+	var err error
+	if strings.Contains(blockType, "OPENSSH") {
+		// Necessary to cast type in order to correctly marshal the key for OpenSSH
+		privDER, _ = x509.MarshalPKCS8PrivateKey(key.(ed25519.PrivateKey))
+	} else if strings.Contains(blockType, "RSA") {
+		privDER = x509.MarshalPKCS1PrivateKey(key.(*rsa.PrivateKey))
+	} else {
+		privDER, err = x509.MarshalPKCS8PrivateKey(key)
+	}
+
+	if err != nil {
+		logrus.Fatal("failed to marshal PKCS8 private key: ", err)
+	}


Suggested change

var privDER []byte

var err error

if strings.Contains(blockType, "OPENSSH") {

// Necessary to cast type in order to correctly marshal the key for OpenSSH

privDER, _ = x509.MarshalPKCS8PrivateKey(key.(ed25519.PrivateKey))

} else if strings.Contains(blockType, "RSA") {

privDER = x509.MarshalPKCS1PrivateKey(key.(*rsa.PrivateKey))

} else {

privDER, err = x509.MarshalPKCS8PrivateKey(key)

}

if err != nil {

logrus.Fatal("failed to marshal PKCS8 private key: ", err)

}

var privBytes []byte

var err error

if strings.Contains(blockType, "OPENSSH") {

// Necessary to cast type in order to correctly marshal the key for OpenSSH

privBytes, err = x509.MarshalPKCS8PrivateKey(key.(ed25519.PrivateKey))

if err != nil {

logrus.Fatalf("failed to marshal PKCS8 private key: %v", err)

}

} else if strings.Contains(blockType, "RSA") {

privBytes = x509.MarshalPKCS1PrivateKey(key.(*rsa.PrivateKey))

} else { // is this block expected? or should it fatal

privBytes, err = x509.MarshalPKCS8PrivateKey(key)

if err != nil {

logrus.Fatalf("failed to marshal PKCS8 private key: %v", err)

}

}

I believe we should leave it in, in case someone wants to use some other key type? Otherwise I can rip this out and just fatal.

jakefhyde · 2023-08-18T21:56:35Z

cmd/create.go

-		pubkey, _ := generatePublicKey(&privkey.PublicKey)
-		corr.PrivateKey = string(encodePrivateKeyToPEM(privkey))
-		corr.PublicKey = string(pubkey)
+		if corr.Vars["corral_ssh_key_type"] == ED25519_KEY_TYPE {


we should probably check these errors as well, even if we weren't before

igomez06

LGTM

jakefhyde · 2023-08-29T15:06:08Z

cmd/create.go

@@ -35,6 +37,7 @@ corral create k3s ghcr.io/rancher/k3s
 corral create k3s-ha -v controlplane_count=3 ghcr.io/rancher/k3s
 corral create k3s-custom /home/rancher/issue-1234
 `
+const ED25519_KEY_TYPE = "ed25519"


Suggested change

const ED25519_KEY_TYPE = "ed25519"

const ed25519KeyType = "ed25519"

Once that's fixed feel free to merge

igomez06

Looks good

slickwarren mentioned this pull request May 31, 2023

add support for ubuntu, docker install, and 1.25+ rancherlabs/corral-packages#11

Closed

igomez06 reviewed Jun 2, 2023

View reviewed changes

cmd/create.go Outdated Show resolved Hide resolved

slickwarren force-pushed the cwarren/ed25519 branch from f2aeab3 to 35301f6 Compare June 2, 2023 18:32

slickwarren requested a review from igomez06 June 2, 2023 18:33

igomez06 approved these changes Jun 2, 2023

View reviewed changes

slickwarren force-pushed the cwarren/ed25519 branch from 35301f6 to 7909dd5 Compare June 10, 2023 00:26

slickwarren requested a review from igomez06 June 10, 2023 00:29

slickwarren force-pushed the cwarren/ed25519 branch 2 times, most recently from bc0e62f to f9ff225 Compare June 13, 2023 18:28

igomez06 reviewed Jun 13, 2023

View reviewed changes

slickwarren force-pushed the cwarren/ed25519 branch from f9ff225 to 7bfdea6 Compare June 13, 2023 22:19

slickwarren force-pushed the cwarren/ed25519 branch from 7bfdea6 to c2783e2 Compare August 18, 2023 21:05

slickwarren requested review from jakefhyde and igomez06 August 18, 2023 21:17

jakefhyde requested changes Aug 18, 2023

View reviewed changes

slickwarren force-pushed the cwarren/ed25519 branch from c2783e2 to 634c205 Compare August 18, 2023 22:32

igomez06 approved these changes Aug 21, 2023

View reviewed changes

jakefhyde requested changes Aug 29, 2023

View reviewed changes

slickwarren force-pushed the cwarren/ed25519 branch from 634c205 to ec69e75 Compare August 29, 2023 15:08

slickwarren requested a review from jakefhyde August 29, 2023 15:09

Add method to use ed25519 ssh key

e6cc016

slickwarren force-pushed the cwarren/ed25519 branch from ec69e75 to e6cc016 Compare August 29, 2023 15:10

igomez06 approved these changes Aug 29, 2023

View reviewed changes

igomez06 merged commit cab0689 into rancherlabs:main Aug 29, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method to use ed25519 ssh key #104

Add method to use ed25519 ssh key #104

slickwarren commented May 27, 2023

igomez06 left a comment

igomez06 left a comment

slickwarren commented Jun 10, 2023

igomez06 Jun 13, 2023

jakefhyde Jun 21, 2023

igomez06 Jun 21, 2023

jakefhyde Jun 21, 2023

jakefhyde Aug 8, 2023

slickwarren Aug 9, 2023

slickwarren Aug 18, 2023

jakefhyde Aug 18, 2023

slickwarren Aug 18, 2023

jakefhyde Aug 18, 2023

slickwarren Aug 18, 2023

igomez06 left a comment

jakefhyde Aug 29, 2023

igomez06 left a comment

	const ED25519_KEY_TYPE = "ed25519"
	const ed25519KeyType = "ed25519"

Add method to use ed25519 ssh key #104

Add method to use ed25519 ssh key #104

Conversation

slickwarren commented May 27, 2023

igomez06 left a comment

Choose a reason for hiding this comment

igomez06 left a comment

Choose a reason for hiding this comment

slickwarren commented Jun 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igomez06 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igomez06 left a comment

Choose a reason for hiding this comment