Use user identify from $LIMA_HOME/_config/user #83

jandubois · 2021-06-26T00:02:43Z

By default lima uses the public keys from ~/.ssh/*.pub.

This change makes it try $LIME_HOME/_config/user.pub first and only fall back on the ~/.ssh/*.pub keys when that one does not exist. It will create the key under $LIMA_HOME/_config if there aren't any public keys under ~/.ssh.

Fixes #78

pkg/sshutil/sshutil.go

AkihiroSuda · 2021-06-26T00:50:09Z

pkg/sshutil/sshutil.go

-		return nil
+	} else {
+		// Check that private key exists and then try to read public key
+		if _, err := os.Stat(privateKeyPath); err == nil {


Return err if !error.Is(err, os.ErrNorExist)

DefaultPubKeys doesn't return any errors; it just ignores key files it cannot read. So not sure what to do here.

In the end the user would be told to run ssh-keygen when DefaultPubKeys returns an empty list, so not sure that aborting on the first error is really helpful.

AkihiroSuda · 2021-06-26T00:50:35Z

pkg/sshutil/sshutil.go

@@ -83,5 +124,12 @@ func SSHArgs(instDir string) ([]string, error) {
 		"-o", "Compression=no",
 		"-o", "BatchMode=yes",
 	}
+	configDir, err := store.LimaConfigDir()
+	if err == nil {


Return err if !error.Is(err, os.ErrNorExist)

AkihiroSuda · 2021-06-26T00:50:50Z

pkg/sshutil/sshutil.go

+	configDir, err := store.LimaConfigDir()
+	if err == nil {
+		privateKeyPath := filepath.Join(configDir, "user")
+		if _, err := os.Stat(privateKeyPath); err == nil {


Return err if !error.Is(err, os.ErrNorExist)

pkg/sshutil/sshutil.go

AkihiroSuda · 2021-06-26T00:53:32Z

pkg/store/store.go

@@ -29,6 +29,20 @@ func LimaDir() (string, error) {
 	return dir, nil
 }

+// LimaConfigDir returns the path of the config directory, $LIMA_HOME/_config.
+// It will create the directory if it doesn't yet exist.


I’m not sure this function should make the dir.

Yeah, it is inconsistent with what LimaDir does, but I was thinking about letting that one create the directory too. Would that simplify the calling code?

AkihiroSuda · 2021-06-26T00:54:42Z

pkg/store/store.go

+	if err != nil {
+		return "", err
+	}
+	configDir := filepath.Join(limaDir, "_config")


“_config” should be a constant.

Yes, I was tempted to put it into store/filenames, but that package claims it only defines names for the instance directories. So where should _config be defined? New package?

pkg/store.ConfigDir

jandubois · 2021-06-26T02:09:29Z

@AkihiroSuda I believe I've addresses all issues you pointed out. Can you please confirm?

But I'm wondering now if the user filename shouldn't be a constant too, but again don't know where to put it.

Should store/filenames be extended to cover both config and instance filenames?

jandubois · 2021-06-26T06:13:34Z

Sorry, I refactored DefaultPubKeys once more, so that it can actually reflect an error back to the caller.

By tightening the error handling I've also reduced the logic nesting level a bit, as it was getting out of hand.

I've also gone ahead and added other file and directory names to the filenames package. Let me know, if this is not how you want it to be!

I believe I'm done with it now (unless you have more feedback).

By default lima uses the public keys from ~/.ssh/*.pub This change makes it try $LIME_HOME/_config/user.pub first and only fall back on the ~/.ssh/*.pub keys when that one does not exist. It will create the key under $LIMA_HOME/_config if there aren't any public keys under ~/.ssh. Signed-off-by: Jan Dubois <jan.dubois@suse.com>

jandubois · 2021-06-28T22:24:54Z

@AkihiroSuda The latest CI failure is different, but I've seen a lot of errors like this before:

time="2021-06-28T21:49:58Z" level=fatal msg="failed to resolve reference \"ghcr.io/stargz-containers/nginx:1.19-alpine-org\": failed to do request: Head \"https://ghcr.io/v2/stargz-containers/nginx/manifests/1.19-alpine-org\": net/http: TLS handshake timeout"

Any idea why that is happening? You would think the connection between gha and ghcr should be pretty solid...

AkihiroSuda · 2021-06-29T02:11:53Z

I guess macOS instances might be running on a different infra that isn't optimal for ghcr?

AkihiroSuda · 2021-06-29T05:10:15Z

pkg/sshutil/sshutil.go

+		if err := os.MkdirAll(configDir, 0700); err != nil {
+			return nil, errors.Wrapf(err, "could not create %q directory", configDir)
+		}
+		keygenCmd := exec.Command("ssh-keygen", "-t", "ed25519", "-q", "-N", "", "-f",


We should lock the directory. I can open a follow-up PR.

AkihiroSuda · 2021-06-29T05:13:16Z

pkg/sshutil/sshutil.go

 	args := []string{
+		"-i", privateKeyPath,


Question: would it be possible to let ssh read both ~/.lima/_config keys and ~/.ssh keys?

Otherwise this will break existing instances.

Question: would it be possible to let ssh read both ~/.lima/_config keys and ~/.ssh keys?

Yes, you can specify multiple -i options, and the identities will be tried in sequence. I think the server may be limiting the number of attempts to 3, so it could still fail if the user added multiple keys after creating the instance. But that is already a problem with the current implementation, so doesn't change anything. Just another reason to have a lima-specific identity, and always specify that one first.

Question: would it be possible to let ssh read both ~/.lima/_config keys and ~/.ssh keys?

I've pushed the change in a separate commit for easier review; lmk if you want me to squash it!

Otherwise instances created with the previous release of lima would become inaccessible by the `limactl shell` command. Signed-off-by: Jan Dubois <jan.dubois@suse.com>

AkihiroSuda

Thanks

AkihiroSuda reviewed Jun 26, 2021

View reviewed changes

pkg/sshutil/sshutil.go Show resolved Hide resolved

AkihiroSuda reviewed Jun 26, 2021

View reviewed changes

pkg/sshutil/sshutil.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jun 26, 2021

View reviewed changes

pkg/sshutil/sshutil.go Outdated Show resolved Hide resolved

AkihiroSuda reviewed Jun 26, 2021

View reviewed changes

jandubois force-pushed the ssh-key branch 2 times, most recently from b8c126b to 6242841 Compare June 26, 2021 01:48

jandubois force-pushed the ssh-key branch from 3c6c103 to 933cceb Compare June 26, 2021 06:07

jandubois force-pushed the ssh-key branch from 933cceb to 986089c Compare June 26, 2021 06:16

jandubois mentioned this pull request Jun 26, 2021

Make ssh keys configurable #78

Closed

jandubois force-pushed the ssh-key branch 3 times, most recently from 0eb0b94 to e329055 Compare June 28, 2021 21:53

jandubois force-pushed the ssh-key branch from e329055 to 7c4ce9e Compare June 28, 2021 22:00

jandubois requested a review from AkihiroSuda June 28, 2021 22:25

AkihiroSuda reviewed Jun 29, 2021

View reviewed changes

AkihiroSuda approved these changes Jun 29, 2021

View reviewed changes

AkihiroSuda reviewed Jun 29, 2021

View reviewed changes

AkihiroSuda added this to the v0.5.0 milestone Jun 29, 2021

Include private keys from ~/.ssh for backwards compatibility

0dfd10d

Otherwise instances created with the previous release of lima would become inaccessible by the `limactl shell` command. Signed-off-by: Jan Dubois <jan.dubois@suse.com>

AkihiroSuda approved these changes Jun 29, 2021

View reviewed changes

AkihiroSuda merged commit 11da9c1 into lima-vm:master Jun 29, 2021

jandubois deleted the ssh-key branch June 29, 2021 06:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use user identify from $LIMA_HOME/_config/user #83

Use user identify from $LIMA_HOME/_config/user #83

jandubois commented Jun 26, 2021

AkihiroSuda Jun 26, 2021

jandubois Jun 26, 2021

AkihiroSuda Jun 26, 2021

AkihiroSuda Jun 26, 2021

AkihiroSuda Jun 26, 2021

jandubois Jun 26, 2021

AkihiroSuda Jun 26, 2021

jandubois Jun 26, 2021

AkihiroSuda Jun 26, 2021

jandubois commented Jun 26, 2021

jandubois commented Jun 26, 2021

jandubois commented Jun 28, 2021

AkihiroSuda commented Jun 29, 2021

AkihiroSuda Jun 29, 2021

AkihiroSuda Jun 29, 2021

jandubois Jun 29, 2021

jandubois Jun 29, 2021

AkihiroSuda left a comment

Use user identify from $LIMA_HOME/_config/user #83

Use user identify from $LIMA_HOME/_config/user #83

Conversation

jandubois commented Jun 26, 2021

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

jandubois commented Jun 26, 2021

jandubois commented Jun 26, 2021

jandubois commented Jun 28, 2021

AkihiroSuda commented Jun 29, 2021

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

AkihiroSuda left a comment

Choose a reason for hiding this comment