Repository: lima-vm/lima
Pull request #495 (2021-12-26 16:18:11)

Enabled symbolic link following in mounts
This update will allow for symbolic links to be followed in mounts in sshfs.

byepolr: Resolves: https://github.com/lima-vm/lima/issues/498
AkihiroSuda: > Leaving the merging to @AkihiroSuda in case there are any security concerns, but I can't think of any myself.

Can we have a test to ensure that the symlink is resolved inside the guest VFS, not in the host VFS?

https://github.com/lima-vm/lima/blob/03b4ac1896c0234dd716df81e7ff9e30ab7fdd8b/hack/test-example.sh#L149-L163
jandubois: > Can we have a test to ensure that the symlink is resolved inside the guest VFS, not in the host VFS?

I guess we can have a test, but it really would be testing `sshfs` functionality, and not `lima`.

But I just realized another potential issue: Since this option "hides" symlinks from the guest, it means certain commands (e.g `ls`, `find`, `tar`) will now recurse into parts of the filesystem it would previously exclude:

```console
$ ls -lR loop
total 0
-rw-r--r--  1 jan  staff  0 27 Dec 01:29 file
lrwxr-xr-x  1 jan  staff  1 27 Dec 01:30 loop -> .

$ lima ls -lR loop
loop:
total 4
-rw-r--r-- 1 jan dialout   0 Dec 27 09:29 file
drwxr-xr-x 1 jan dialout 128 Dec 27 09:30 loop

loop/loop:
total 4
-rw-r--r-- 1 jan dialout   0 Dec 27 09:29 file
drwxr-xr-x 1 jan dialout 128 Dec 27 09:30 loop

[...]

loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop:
ls: cannot access 'loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop': Operation not permitted
total 0
-rw-r--r-- 1 jan dialout 0 Dec 27 09:29 file
?????????? ? ?   ?       ?            ? loop

$ find loop -name file
loop/file

$ lima find loop -name file
loop/file
loop/loop/file
loop/loop/loop/file
loop/loop/loop/loop/file
loop/loop/loop/loop/loop/file
loop/loop/loop/loop/loop/loop/file
[...]
find: ‘loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop/loop’: Operation not permitted
```

I still think this is fine (or maybe it should be configurable?), but maybe needs to be documented.
AkihiroSuda: > I still think this is fine, but maybe needs to be documented or configurable.

Doesn't seem fine to me.
Probably we should have a new YAML field like `.[]mounts.sshfs.followSymlinks`.

jandubois: > Probably we should have a new YAML field like `.[]mounts.sshfs.followSymlinks`.

Do we really need the extra `sshfs` level in there? I would make it another property at the same level as `writable`.
AkihiroSuda: > > Probably we should have a new YAML field like `.[]mounts.sshfs.followSymlinks`.
> 
> Do we really need the extra `sshfs` level in there? I would make it another property at the same level as `writable`.

SSHFS will be abolished as soon as the 9p patch gets merged.
https://lists.gnu.org/archive/html/qemu-devel/2021-11/msg04325.html

So I'd prefer to have the `followSymlinks` property under `sshfs`.
Not sure how it will look like with 9p.

jandubois: > Not sure how it will look like with 9p.

Well, either we can support it with 9p, or if we can't, we "undocument" the field and throw an error during validation if it is still set to `true`.

I somewhat dislike leaking implementation details to the names of user settings. Furthermore I dislike adding additional struct levels with just a single field in them. But if you feel strongly about this, then that's ok too...
jjonesskytap: @jandubois / @AkihiroSuda: I've made the changes to allow for this to be configurable via a new SSHFS->follow_symlinks parameter in the mount section of the yaml file.

For example:
https://github.com/lima-vm/lima/pull/495/files#diff-184f50817a75384709c275a226e0bd6f24e229edc9a82644a75e11637453f196
byepolr: @jandubois / @AkihiroSuda: I've made the changes to allow for this to be configurable via a new SSHFS->follow_symlinks parameter in the mount section of the yaml file. If you would like an automated test for this, let me know which test(s) files you think would be most applicable to update.

For example:
https://github.com/lima-vm/lima/pull/495/files#diff-184f50817a75384709c275a226e0bd6f24e229edc9a82644a75e11637453f196
byepolr: > Can we have a test to ensure that the symlink is resolved inside the guest VFS, not in the host VFS?

@AkihiroSuda, perhaps I've mistaken the comment. But `follow_symlinks` will resolve symlinks on the host's file system. This will allow the guest OS to be able to traverse these symlinked directories. That said, from my understanding. User permissions on the host side will still be honored. Which is exactly what I think we want...
jandubois: It turns out that with `followSymlinks` enabled, you cannot create symlinks from inside the guest:

```console
jan@lima-default:/Users/jan$ ln -s .lima lima
ln: failed to create symbolic link 'lima': Input/output error
```

This is a strong argument against making this enabled by default, and makes me question if this change is desirable after all. It is only helpful for symlinks that point outside the mapped tree, so maybe adding the symlink targets explicitly to the mounts is a better option (which was one of the reasons I wanted the `default.yaml` and `override.yaml` mechanism, to have those mounted in all configs).
jjonesskytap: > This is a strong argument against making this enabled by default, and makes me question if this change is desirable after all.

I can make that change to the default.yaml to ensure that in the examples it's default off. That said, I think this feature is very useful. At my company, we have people wanting files to be specified from any number of locations. To automate this, we have a specific source folder that they can just add a symbolic link so that nothing needs to be edited with lima. This also meeds nothing to restart or to reload, it just works.

jjonesskytap: @jandubois, regarding creating symbolic links in the guest. One can actually do this, but I think it would probably make more sense to actually do this on the host side of things as it would be more sticky. that said, I did this with the default config with no changes.

Guest VM - Readonly: Rightfully fails because mounted as read-only by default
```
jwehrlich@lima-default:/Users/jwehrlich$ ln -s /Users/jwehrlich/.lima/ my-lima
ln: failed to create symbolic link 'my-lima': Read-only file system
```

Guest VM - Readonly: Creating a symlink outside a mounted location works
```
jwehrlich@lima-default:/Users/jwehrlich$ cd /Users/
jwehrlich@lima-default:/Users/jwehrlich$ sudo ln -s /Users/jwehrlich/.lima/ my-lima
jwehrlich@lima-default:/Users$ ls
jwehrlich  my-lima
jwehrlich@lima-default:/Users$ ls my-lima
_config  archlinux  default
```

Gest VM - Writable: Creating symlink works since mount is read writable
```
jwehrlich@lima-default:/Users/jwehrlich$ ln -s /Users/jwehrlich/.lima /Users/jwehrlich/my-lima
jwehrlich@lima-default:/Users/jwehrlich$ ls my-lima
_config  archlinux  default
```

Host - With the execution of the last "Guest VM" will show that the symbolic link exists on the host machine
```
~/src/lima$ ls /Users/jwehrlich/my-lima
_config   archlinux default   flexd
```
jandubois: > Gest VM - Writable: Creating symlink works since mount is read writable
>
> ```
> jwehrlich@lima-default:/Users/jwehrlich$ ln -s /Users/jwehrlich/.lima /Users/jwehrlich/my-lima
> ```

Interesting, it does actually. I wonder why I get an error though:

```console
$ lima
jan@lima-default:/Users/jan$ ln -s .lima lima; echo $?
ln: failed to create symbolic link 'lima/.lima': Operation not permitted
1
jan@lima-default:/Users/jan$ ls -ld lima
drwx------ 1 jan dialout 288 Dec 27 22:15 lima
jan@lima-default:/Users/jan$ rm lima
rm: cannot remove 'lima': Is a directory
```

And of course, even though you can create the symlink, you cannot delete it, because inside the guest it is a directory, not a symlink.

This kind of violates the lima premise that the environment on the guest and the host behave as similar as possible (so it looks like you run `nerdctl` on the host, when it really runs inside the guest).

I'm not opposed to keeping the feature, I'm just saying it should not be the default in the builtin template anymore. And the caveats need to be clearly documented.

Also remember that @AkihiroSuda said that this feature may not be supportable when we switch from `sshfs` to `9p`. Maybe using `9p` will be an option instead of a replacement, but it is too early to tell.
jandubois: Please squash commits, to eliminate the refactoring from the history!
jjonesskytap: > Please squash commits, to eliminate the refactoring from the history!
Out of curiosity, why don't you just let Github handle this for you? Instead of `Merge pull request` you can click on the dropdown on the right and select `Squash and merge`
byepolr: > Please squash commits, to eliminate the refactoring from the history!
Out of curiosity, why don't you just let Github handle this for you? Instead of `Merge pull request` you can click on the dropdown on the right and select `Squash and merge`

jandubois: > Out of curiosity, why don't you just let Github handle this for you? Instead of Merge pull request you can click on the dropdown on the right and select Squash and merge

Because it feels kind of wrong to attach a DCO sign-off to a merge-commit that has not been created directly by the author. Maybe it is fine; idk what kind of policy the big foundations (Linux Foundations, CNCF) have. Letting the author squash is keeping it on the safe side.

The other reason though is that not all commits should be squashed for every PR. We want to squash false starts and refactoring, but not commits that have been intentionally created to address different aspects of the PR.
byepolr: @jandubois / @AkihiroSuda: I think all your concerns should now be addressed in the latest iteration.
byepolr: @jandubois, I've got the refactored Writable ready to be turned into a PR as soon as this one goes out. :)

Patch:
diff --git a/pkg/hostagent/mount.go b/pkg/hostagent/mount.go
index 400c0710..f13461c8 100644
--- a/pkg/hostagent/mount.go
+++ b/pkg/hostagent/mount.go
@@ -40,16 +40,20 @@ func (a *HostAgent) setupMount(ctx context.Context, m limayaml.Mount) (*mount, e
 	if err := os.MkdirAll(expanded, 0755); err != nil {
 		return nil, err
 	}
+	// NOTE: allow_other requires "user_allow_other" in /etc/fuse.conf
+	sshfsOptions := "allow_other"
+	if *m.SSHFS.FollowSymlinks {
+		sshfsOptions = sshfsOptions + ",follow_symlinks"
+	}
 	logrus.Infof("Mounting %q", expanded)
 	rsf := &reversesshfs.ReverseSSHFS{
-		SSHConfig:  a.sshConfig,
-		LocalPath:  expanded,
-		Host:       "127.0.0.1",
-		Port:       a.sshLocalPort,
-		RemotePath: expanded,
-		Readonly:   !m.Writable,
-		// NOTE: allow_other requires "user_allow_other" in /etc/fuse.conf
-		SSHFSAdditionalArgs: []string{"-o", "allow_other"},
+		SSHConfig:           a.sshConfig,
+		LocalPath:           expanded,
+		Host:                "127.0.0.1",
+		Port:                a.sshLocalPort,
+		RemotePath:          expanded,
+		Readonly:            !m.Writable,
+		SSHFSAdditionalArgs: []string{"-o", sshfsOptions},
 	}
 	if err := rsf.Prepare(); err != nil {
 		return nil, fmt.Errorf("failed to prepare reverse sshfs for %q: %w", expanded, err)
diff --git a/pkg/limayaml/default.yaml b/pkg/limayaml/default.yaml
index 0eef97cd..3a57f959 100644
--- a/pkg/limayaml/default.yaml
+++ b/pkg/limayaml/default.yaml
@@ -42,8 +42,17 @@ mounts:
     # CAUTION: `writable` SHOULD be false for the home directory.
     # Setting `writable` to true is possible, but untested and dangerous.
     writable: false
+    # SSHFS has an optional flag called 'follow_symlinks'. This allows mounts
+    # to be properly resolved in the guest os and allow for access to the
+    # contents of the symlnk. This defaults to false if not supplied. As a result,
+    # symlinked files & folders on the Host system will look and feel like regular
+    # files directories in the Guest OS.
+    sshfs:
+      followSymlinks: false
   - location: "/tmp/lima"
     writable: true
+    sshfs:
+      followSymlinks: false
 
 ssh:
   # A localhost port of the host. Forwarded to port 22 of the guest.
diff --git a/pkg/limayaml/defaults.go b/pkg/limayaml/defaults.go
index 943a2803..ffc96b91 100644
--- a/pkg/limayaml/defaults.go
+++ b/pkg/limayaml/defaults.go
@@ -298,6 +298,9 @@ func FillDefault(y, d, o *LimaYAML, filePath string) {
 	location := make(map[string]int)
 	for _, mount := range append(append(d.Mounts, y.Mounts...), o.Mounts...) {
 		if i, ok := location[mount.Location]; ok {
+			if mount.SSHFS.FollowSymlinks != nil {
+				mounts[i].SSHFS.FollowSymlinks = mount.SSHFS.FollowSymlinks
+			}
 			mounts[i].Writable = mount.Writable
 		} else {
 			location[mount.Location] = len(mounts)
@@ -306,6 +309,13 @@ func FillDefault(y, d, o *LimaYAML, filePath string) {
 	}
 	y.Mounts = mounts
 
+	for i := range y.Mounts {
+		mount := &y.Mounts[i]
+		if mount.SSHFS.FollowSymlinks == nil {
+			mount.SSHFS.FollowSymlinks = pointer.Bool(false)
+		}
+	}
+
 	// Note: DNS lists are not combined; highest priority setting is picked
 	if len(y.DNS) == 0 {
 		y.DNS = d.DNS
diff --git a/pkg/limayaml/limayaml.go b/pkg/limayaml/limayaml.go
index 4bdbc5b9..01118de1 100644
--- a/pkg/limayaml/limayaml.go
+++ b/pkg/limayaml/limayaml.go
@@ -45,6 +45,11 @@ type File struct {
 type Mount struct {
 	Location string `yaml:"location" json:"location"` // REQUIRED
 	Writable bool   `yaml:"writable,omitempty" json:"writable,omitempty"`
+	SSHFS    SSHFS  `yaml:"sshfs,omitempty" json:"sshfs,omitempty"`
+}
+
+type SSHFS struct {
+	FollowSymlinks *bool `yaml:"followSymlinks,omitempty" json:"followSymlinks,omitempty"`
 }
 
 type SSH struct {
diff --git a/pkg/qemu/qemu.go b/pkg/qemu/qemu.go
index 95601577..5181de2b 100644
--- a/pkg/qemu/qemu.go
+++ b/pkg/qemu/qemu.go
@@ -208,24 +208,12 @@ func Cmdline(cfg Config) (string, []string, error) {
 	}
 	switch *y.Arch {
 	case limayaml.X8664:
-		cpu := "qemu64"
+		cpu := "Haswell-v4"
 		if isNativeArch(*y.Arch) {
 			cpu = "host"
 		}
 		args = appendArgsIfNoConflict(args, "-cpu", cpu)
-		if isNativeArch(*y.Arch) {
-			args = appendArgsIfNoConflict(args, "-machine", "q35,accel="+accel)
-		} else {
-			// use q35 machine with vmware io port disabled.
-			args = appendArgsIfNoConflict(args, "-machine", "q35,vmport=off")
-			// use tcg accelerator with multi threading with 512MB translation block size
-			// https://qemu-project.gitlab.io/qemu/devel/multi-thread-tcg.html?highlight=tcg
-			// https://qemu-project.gitlab.io/qemu/system/invocation.html?highlight=tcg%20opts
-			// this will make sure each vCPU will be backed by 1 host user thread.
-			args = appendArgsIfNoConflict(args, "-accel", "tcg,thread=multi,tb-size=512")
-			// This will disable CPU S3 state.
-			args = append(args, "-global", "ICH9-LPC.disable_s3=1")
-		}
+		args = appendArgsIfNoConflict(args, "-machine", "q35,accel="+accel)
 	case limayaml.AARCH64:
 		cpu := "cortex-a72"
 		if isNativeArch(*y.Arch) {
@@ -336,9 +324,9 @@ func Cmdline(cfg Config) (string, []string, error) {
 	default:
 		// QEMU does not seem to support virtio-vga for aarch64
 		args = append(args, "-vga", "none", "-device", "ramfb")
-		args = append(args, "-device", "qemu-xhci,id=usb-bus")
-		args = append(args, "-device", "usb-kbd,bus=usb-bus.0")
-		args = append(args, "-device", "usb-mouse,bus=usb-bus.0")
+		args = append(args, "-device", "usb-ehci")
+		args = append(args, "-device", "usb-kbd")
+		args = append(args, "-device", "usb-mouse")
 	}
 
 	// Parallel


Test patch:
diff --git a/pkg/limayaml/defaults_test.go b/pkg/limayaml/defaults_test.go
index 7c5cc10a..e1939dab 100644
--- a/pkg/limayaml/defaults_test.go
+++ b/pkg/limayaml/defaults_test.go
@@ -116,6 +116,7 @@ func TestFillDefault(t *testing.T) {
 
 	expect := builtin
 	expect.Mounts = y.Mounts
+	expect.Mounts[0].SSHFS.FollowSymlinks = pointer.Bool(false)
 	// Only missing Mounts field is Writable, and the default value is also the null value: false
 
 	expect.Provision = y.Provision
@@ -235,6 +236,7 @@ func TestFillDefault(t *testing.T) {
 	expect = d
 	// Also verify that archive arch is filled in
 	expect.Containerd.Archives[0].Arch = *d.Arch
+	expect.Mounts[0].SSHFS.FollowSymlinks = pointer.Bool(false)
 
 	y = LimaYAML{}
 	FillDefault(&y, &d, &LimaYAML{}, filePath)
@@ -302,6 +304,7 @@ func TestFillDefault(t *testing.T) {
 			{
 				Location: "/var/log",
 				Writable: true,
+				SSHFS:    SSHFS{FollowSymlinks: pointer.Bool(true)},
 			},
 		},
 		Provision: []Provision{
@@ -358,6 +361,7 @@ func TestFillDefault(t *testing.T) {
 	// o.Mounts just makes d.Mounts[0] writable because the Location matches
 	expect.Mounts = append(d.Mounts, y.Mounts...)
 	expect.Mounts[0].Writable = true
+	expect.Mounts[0].SSHFS.FollowSymlinks = pointer.Bool(true)
 
 	// o.Networks[1] is overriding the d.Networks[0].Lima entry for the "def0" interface
 	expect.Networks = append(append(d.Networks, y.Networks...), o.Networks[0])