Skip to content

Commit

Permalink
Merge pull request #335 from wallyworld/fix-ssh-keyfile
Browse files Browse the repository at this point in the history 
#335

The generation of the id25519 ssh keys was incorrect, such that they could not be used to ssh.

This PR fixes that and also ensure the generated filenames are correct.
  • Loading branch information
@jujubot
jujubot authored Feb 9, 2024
2 parents 56659c6 + b0823b9 commit 72bc90d
Show file tree
Hide file tree
Showing 6 changed files with 23 additions and 27 deletions.
2 changes: 1 addition & 1 deletion ssh/clientkeys.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
"github.com/juju/utils/v4"
)

const clientKeyName = "juju_id_rsa"
const clientKeyName = "juju_id_ed25519"

// PublicKeySuffix is the file extension for public key files.
const PublicKeySuffix = ".pub"
Expand Down
25 changes: 13 additions & 12 deletions ssh/clientkeys_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,10 @@ import (

gitjujutesting "github.com/juju/testing"
jc "github.com/juju/testing/checkers"
gc "gopkg.in/check.v1"

"github.com/juju/utils/v4"
"github.com/juju/utils/v4/ssh"
gc "gopkg.in/check.v1"
)

type ClientKeysSuite struct {
Expand All @@ -23,7 +24,7 @@ var _ = gc.Suite(&ClientKeysSuite{})
func (s *ClientKeysSuite) SetUpTest(c *gc.C) {
s.FakeHomeSuite.SetUpTest(c)
s.AddCleanup(func(*gc.C) { ssh.ClearClientKeys() })
generateKeyRestorer := overrideGenerateKey(c)
generateKeyRestorer := overrideGenerateKey()
s.AddCleanup(func(*gc.C) { generateKeyRestorer.Restore() })
}

Expand Down Expand Up @@ -51,7 +52,7 @@ func (s *ClientKeysSuite) TestPublicKeyFiles(c *gc.C) {
// and populate it with a key pair.
err := ssh.LoadClientKeys("~/.juju/ssh")
c.Assert(err, jc.ErrorIsNil)
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_rsa.pub")
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_ed25519.pub")
// All files ending with .pub in the client key dir get picked up.
priv, pub, err := ssh.GenerateKey("whatever")
c.Assert(err, jc.ErrorIsNil)
Expand All @@ -61,12 +62,12 @@ func (s *ClientKeysSuite) TestPublicKeyFiles(c *gc.C) {
c.Assert(err, jc.ErrorIsNil)
// The new public key won't be observed until the
// corresponding private key exists.
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_rsa.pub")
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_ed25519.pub")
err = ioutil.WriteFile(gitjujutesting.HomePath(".juju", "ssh", "whatever"), []byte(priv), 0600)
c.Assert(err, jc.ErrorIsNil)
err = ssh.LoadClientKeys("~/.juju/ssh")
c.Assert(err, jc.ErrorIsNil)
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_rsa.pub", "~/.juju/ssh/whatever.pub")
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_ed25519.pub", "~/.juju/ssh/whatever.pub")
}

func (s *ClientKeysSuite) TestPrivateKeyFiles(c *gc.C) {
Expand All @@ -75,7 +76,7 @@ func (s *ClientKeysSuite) TestPrivateKeyFiles(c *gc.C) {
// unless LoadClientKeys is called again.
err := ssh.LoadClientKeys("~/.juju/ssh")
c.Assert(err, jc.ErrorIsNil)
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_rsa")
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_ed25519")
priv, pub, err := ssh.GenerateKey("whatever")
c.Assert(err, jc.ErrorIsNil)
err = ioutil.WriteFile(gitjujutesting.HomePath(".juju", "ssh", "whatever"), []byte(priv), 0600)
Expand All @@ -84,22 +85,22 @@ func (s *ClientKeysSuite) TestPrivateKeyFiles(c *gc.C) {
c.Assert(err, jc.ErrorIsNil)
// The new private key won't be observed until the
// corresponding public key exists.
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_rsa")
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_ed25519")
err = ioutil.WriteFile(gitjujutesting.HomePath(".juju", "ssh", "whatever.pub"), []byte(pub), 0600)
c.Assert(err, jc.ErrorIsNil)
// new keys won't be reported until we call LoadClientKeys again
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_rsa.pub")
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_rsa")
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_ed25519.pub")
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_ed25519")
err = ssh.LoadClientKeys("~/.juju/ssh")
c.Assert(err, jc.ErrorIsNil)
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_rsa.pub", "~/.juju/ssh/whatever.pub")
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_rsa", "~/.juju/ssh/whatever")
checkPublicKeyFiles(c, "~/.juju/ssh/juju_id_ed25519.pub", "~/.juju/ssh/whatever.pub")
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_ed25519", "~/.juju/ssh/whatever")
}

func (s *ClientKeysSuite) TestLoadClientKeysDirExists(c *gc.C) {
err := os.MkdirAll(gitjujutesting.HomePath(".juju", "ssh"), 0755)
c.Assert(err, jc.ErrorIsNil)
err = ssh.LoadClientKeys("~/.juju/ssh")
c.Assert(err, jc.ErrorIsNil)
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_rsa")
checkPrivateKeyFiles(c, "~/.juju/ssh/juju_id_ed25519")
}
9 changes: 2 additions & 7 deletions ssh/generate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package ssh
import (
"crypto/ed25519"
"crypto/rand"
"crypto/x509"
"encoding/pem"
"fmt"
"strings"
Expand All @@ -28,15 +27,11 @@ func GenerateKey(comment string) (private, public string, err error) {
return "", "", errors.Trace(err)
}

keyData, err := x509.MarshalPKCS8PrivateKey(privateKey)
pemBlock, err := ssh.MarshalPrivateKey(privateKey, comment)
if err != nil {
return "", "", errors.Trace(err)
}
identity := pem.EncodeToMemory(
&pem.Block{
Type: "PRIVATE KEY",
Bytes: keyData,
})
identity := pem.EncodeToMemory(pemBlock)

public, err = PublicKey(identity, comment)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions ssh/generate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ var (

// overrideGenerateKey patches out rsa.GenerateKey to create a single testing
// key which is saved and used between tests to save computation time.
func overrideGenerateKey(c *gc.C) testing.Restorer {
func overrideGenerateKey() testing.Restorer {
restorer := testing.PatchValue(ssh.ED25519GenerateKey, func(random io.Reader) (ed25519.PublicKey, ed25519.PrivateKey, error) {
if pregeneratedKey != nil {
return ed25519.PublicKey{}, pregeneratedKey, nil
Expand Down Expand Up @@ -63,11 +63,11 @@ func generateDSAKey(random io.Reader) (*dsa.PrivateKey, error) {
}

func (s *GenerateSuite) TestGenerate(c *gc.C) {
defer overrideGenerateKey(c).Restore()
defer overrideGenerateKey().Restore()
private, public, err := ssh.GenerateKey("some-comment")
c.Check(err, jc.ErrorIsNil)
c.Check(private, jc.HasPrefix, "-----BEGIN PRIVATE KEY-----\n")
c.Check(private, jc.HasSuffix, "-----END PRIVATE KEY-----\n")
c.Check(private, jc.HasPrefix, "-----BEGIN OPENSSH PRIVATE KEY-----\n")
c.Check(private, jc.HasSuffix, "-----END OPENSSH PRIVATE KEY-----\n")
c.Check(public, jc.HasPrefix, "ssh-ed25519 ")
c.Check(public, jc.HasSuffix, " some-comment\n")
}
2 changes: 1 addition & 1 deletion ssh/ssh_gocrypto_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ func (s *SSHGoCryptoCommandSuite) SetUpSuite(c *gc.C) {
func (s *SSHGoCryptoCommandSuite) SetUpTest(c *gc.C) {
s.IsolationSuite.SetUpTest(c)

generateKeyRestorer := overrideGenerateKey(c)
generateKeyRestorer := overrideGenerateKey()
s.AddCleanup(func(*gc.C) { generateKeyRestorer.Restore() })

client, err := ssh.NewGoCryptoClient()
Expand Down
4 changes: 2 additions & 2 deletions ssh/ssh_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,12 +205,12 @@ func (s *SSHCommandSuite) TestCopy(c *gc.C) {
}

func (s *SSHCommandSuite) TestCommandClientKeys(c *gc.C) {
defer overrideGenerateKey(c).Restore()
defer overrideGenerateKey().Restore()
clientKeysDir := c.MkDir()
defer ssh.ClearClientKeys()
err := ssh.LoadClientKeys(clientKeysDir)
c.Assert(err, jc.ErrorIsNil)
ck := filepath.Join(clientKeysDir, "juju_id_rsa")
ck := filepath.Join(clientKeysDir, "juju_id_ed25519")
var opts ssh.Options
opts.SetIdentities("x", "y")
s.assertCommandArgs(c, s.commandOptions([]string{echoCommand, "123"}, &opts),
Expand Down

0 comments on commit 72bc90d

Please sign in to comment.