Enabled symbolic link following in mounts #495

jwehrlich · 2021-12-26T16:18:11Z

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

jwehrlich · 2021-12-26T18:01:56Z

Resolves #498

jandubois

Thank you, this is great! Now I can remove all my extra mounts for symlink targets from override.yaml again. 😄

Leaving the merging to @AkihiroSuda in case there are any security concerns, but I can't think of any myself.

AkihiroSuda · 2021-12-27T05:04:43Z

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?

lima/hack/test-example.sh

Lines 149 to 163 in 03b4ac1

    
           if [[ -n ${CHECKS["mount-home"]} ]]; then 
        
           	hometmp="$HOME/lima-test-tmp" 
        
           	INFO "Testing home access (\"$hometmp\")" 
        
           	rm -rf "$hometmp" 
        
           	mkdir -p "$hometmp" 
        
           	defer "rm -rf \"$hometmp\"" 
        
           	echo "random-content-${RANDOM}" >"$hometmp/random" 
        
           	expected="$(cat "$hometmp/random")" 
        
           	got="$(limactl shell "$NAME" cat "$hometmp/random")" 
        
           	INFO "$hometmp/random: expected=${expected}, got=${got}" 
        
           	if [ "$got" != "$expected" ]; then 
        
           		ERROR "Home directory is not shared?" 
        
           		exit 1 
        
           	fi 
        
           fi

jandubois · 2021-12-27T09:42:32Z

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 they would previously exclude:

$ 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, but maybe needs to be documented or configurable.

AkihiroSuda · 2021-12-27T09:50:47Z

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.

AkihiroSuda

Thanks, but #495 (comment)

jandubois · 2021-12-27T09:54:34Z

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 · 2021-12-27T10:05:57Z

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 · 2021-12-27T10:15:38Z

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...

jwehrlich · 2021-12-27T20:14:47Z

@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

jwehrlich · 2021-12-27T20:18:27Z

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...

Reference:
• https://manpages.debian.org/experimental/sshfs/sshfs.1.en.html#o~9
• https://support.nesi.org.nz/hc/en-gb/articles/360000621135-Can-I-use-SSHFS-to-mount-the-cluster-filesystem-on-my-local-machine-

pkg/limayaml/limayaml.go

jandubois · 2021-12-27T20:57:14Z

pkg/limayaml/default.yaml

@@ -42,8 +42,15 @@ 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 in the symlnked folder. This defaults to false if not supplied.


Please add a note that explains that followSymlinks does not map the symlink end-points into the guest, but makes them look like regular files/directories. With the option enabled, there are no symlinks in the mounted tree, and symlinks in the tree cannot be created from the guest.

jandubois · 2021-12-27T21:02:37Z

It turns out that with followSymlinks enabled, you cannot create symlinks from inside the guest:

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).

jwehrlichskytap · 2021-12-27T21:23:00Z

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.

TLDR;
We really want to be able to specify a symlink that is not a child of the mounted directory. This is exactly the use case for the flag

jwehrlichskytap · 2021-12-27T21:54:56Z

@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 · 2021-12-27T22:17:58Z

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 work. I wonder why I get an error though:

$ 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 · 2021-12-27T22:26:43Z

pkg/limayaml/default.yaml

@@ -45,6 +45,8 @@ mounts:
    # 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 in the symlnked folder. This defaults to false if not supplied.
+    # As a result, symlinked folders on the Host system will look and feel like


It is both files and directories, not just directories. Please also mention that symlinks to directories cannot be deleted inside the guest.

jandubois · 2021-12-27T22:30:13Z

Please squash commits, to eliminate the refactoring from the history!

jandubois · 2021-12-27T22:43:10Z

pkg/limayaml/defaults.go

@@ -299,6 +299,7 @@ func FillDefault(y, d, o *LimaYAML, filePath string) {
 	for _, mount := range append(append(d.Mounts, y.Mounts...), o.Mounts...) {
 		if i, ok := location[mount.Location]; ok {
 			mounts[i].Writable = mount.Writable
+			mounts[i].SSHFS = mount.SSHFS


This is unfortunately not enough. Both mount.Writable and mount.SSHFS.ForwardSymlinks need to become pointers to booleans, so they can be independently defaulted/overridden. This should also be added to the defaults_test.go to make sure it works.

Not making mount.Writable a pointer to start with was a mistake already, but it becomes more obvious now that we have multiple properties on a mount.

jwehrlich · 2021-12-28T02:35:03Z

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

AkihiroSuda · 2021-12-28T02:49:02Z

pkg/hostagent/mount.go

@@ -40,16 +40,21 @@ 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
+	ssh_options := "allow_other"


s/ssh/sshfs/

We use camelCase, not snake_case

jandubois · 2021-12-28T04:57:26Z

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.

jandubois

The merging of followSymlinks does not work correctly (and has no tests).

I can make the updates for Mount#Writable, but I figured that probably warrants its own PR.

Yes, logically it should be a separate PR, but since it is closely related to how followSymlinks is handled, I wouldn't object to including it here. Your choice!

jandubois · 2021-12-28T05:02:32Z

pkg/limayaml/defaults.go

 		if i, ok := location[mount.Location]; ok {
+			mounts[i].SSHFS.FollowSymlinks = mount.SSHFS.FollowSymlinks


You only overwrite if the pointer is not nil:

Suggested change

mounts[i].SSHFS.FollowSymlinks = mount.SSHFS.FollowSymlinks

if mount.SSHFS.FollowSymlinks != nil {

mounts[i].SSHFS.FollowSymlinks = mount.SSHFS.FollowSymlinks

}

jandubois · 2021-12-28T05:04:45Z

pkg/limayaml/defaults.go

@@ -297,7 +297,11 @@ func FillDefault(y, d, o *LimaYAML, filePath string) {
 	mounts := make([]Mount, 0, len(d.Mounts)+len(y.Mounts)+len(o.Mounts))
 	location := make(map[string]int)
 	for _, mount := range append(append(d.Mounts, y.Mounts...), o.Mounts...) {
+		if mount.SSHFS.FollowSymlinks == nil {
+			mount.SSHFS.FollowSymlinks = pointer.Bool(false)
+		}


Setting the default cannot happen here; you need to merge from all the sources first, and then have another loop over the final result, filling in the missing values with their defaults. Look at how the networks are being merged, and copy that mechanism.

Adding test cases for this to defaults_test.go would help to show that the current logic does not work as expected.

jwehrlich · 2021-12-28T18:24:08Z

@jandubois / @AkihiroSuda: I think all your concerns should now be addressed in the latest iteration.

jwehrlich · 2021-12-28T21:12:34Z

@jandubois, I've got the refactored Writable ready to be turned into a PR as soon as this one goes out. :)

pkg/hostagent/mount.go

Signed-off-by: Jason W. Ehrlich <jwehrlich@outlook.com>

jwehrlich

Changed sshOptions to sshfsOptions

pkg/hostagent/mount.go

jandubois

Thanks, LGTM now. Waiting for @AkihiroSuda review before merging.

AkihiroSuda

Thanks

jwehrlich force-pushed the link-following branch from 02df955 to e73be4e Compare December 26, 2021 16:23

jwehrlich closed this Dec 26, 2021

jwehrlich deleted the link-following branch December 26, 2021 17:01

jwehrlich restored the link-following branch December 26, 2021 17:09

jwehrlich reopened this Dec 26, 2021

jwehrlich mentioned this pull request Dec 26, 2021

Symbolic links do not resolve in mounts #498

Closed

jandubois previously approved these changes Dec 26, 2021

View reviewed changes

AkihiroSuda self-requested a review December 27, 2021 01:12

AkihiroSuda added the component/sshfs label Dec 27, 2021

AkihiroSuda linked an issue Dec 27, 2021 that may be closed by this pull request

Symbolic links do not resolve in mounts #498

Closed

AkihiroSuda requested changes Dec 27, 2021

View reviewed changes

jwehrlich dismissed jandubois’s stale review via 685fb86 December 27, 2021 14:36

jwehrlich force-pushed the link-following branch 3 times, most recently from c309350 to e4741a8 Compare December 27, 2021 16:44

jandubois reviewed Dec 27, 2021

View reviewed changes

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

jandubois reviewed Dec 27, 2021

View reviewed changes

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

jandubois reviewed Dec 27, 2021

View reviewed changes

AkihiroSuda reviewed Dec 28, 2021

View reviewed changes

jandubois requested changes Dec 28, 2021

View reviewed changes

jwehrlich force-pushed the link-following branch 2 times, most recently from bb9bcde to b663c48 Compare December 28, 2021 17:51

jwehrlich requested review from jandubois and AkihiroSuda December 28, 2021 20:39

jandubois reviewed Dec 28, 2021

View reviewed changes

pkg/hostagent/mount.go Outdated Show resolved Hide resolved

jwehrlich force-pushed the link-following branch 2 times, most recently from 0dac92a to b709cb9 Compare December 29, 2021 00:15

Enable symlink following in mounts

3fa3af4

Signed-off-by: Jason W. Ehrlich <jwehrlich@outlook.com>

jwehrlich force-pushed the link-following branch from b709cb9 to 3fa3af4 Compare December 29, 2021 00:16

jwehrlich requested a review from jandubois December 29, 2021 00:18

jwehrlich commented Dec 29, 2021

View reviewed changes

pkg/hostagent/mount.go Outdated Show resolved Hide resolved

jandubois approved these changes Dec 29, 2021

View reviewed changes

AkihiroSuda approved these changes Dec 29, 2021

View reviewed changes

AkihiroSuda merged commit 595ed4f into lima-vm:master Dec 29, 2021

AkihiroSuda added this to the v0.8.1 milestone Dec 29, 2021

AkihiroSuda added the enhancement New feature or request label Dec 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabled symbolic link following in mounts #495

Enabled symbolic link following in mounts #495

jwehrlich commented Dec 26, 2021

jwehrlich commented Dec 26, 2021 •

edited

jandubois left a comment

AkihiroSuda commented Dec 27, 2021

jandubois commented Dec 27, 2021 •

edited

AkihiroSuda commented Dec 27, 2021

AkihiroSuda left a comment

jandubois commented Dec 27, 2021

AkihiroSuda commented Dec 27, 2021

jandubois commented Dec 27, 2021

jwehrlich commented Dec 27, 2021

jwehrlich commented Dec 27, 2021 •

edited

jandubois Dec 27, 2021

jandubois commented Dec 27, 2021

jwehrlichskytap commented Dec 27, 2021 •

edited

jwehrlichskytap commented Dec 27, 2021

jandubois commented Dec 27, 2021 •

edited

jandubois Dec 27, 2021

jandubois commented Dec 27, 2021

jandubois Dec 27, 2021

jwehrlich commented Dec 28, 2021 •

edited

AkihiroSuda Dec 28, 2021

jandubois commented Dec 28, 2021

jandubois left a comment

jandubois Dec 28, 2021

jandubois Dec 28, 2021

jwehrlich commented Dec 28, 2021

jwehrlich commented Dec 28, 2021

jwehrlich left a comment

jandubois left a comment

AkihiroSuda left a comment

		if i, ok := location[mount.Location]; ok {
		mounts[i].SSHFS.FollowSymlinks = mount.SSHFS.FollowSymlinks

Enabled symbolic link following in mounts #495

Enabled symbolic link following in mounts #495

Conversation

jwehrlich commented Dec 26, 2021

jwehrlich commented Dec 26, 2021 • edited

jandubois left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 27, 2021

jandubois commented Dec 27, 2021 • edited

AkihiroSuda commented Dec 27, 2021

AkihiroSuda left a comment

Choose a reason for hiding this comment

jandubois commented Dec 27, 2021

AkihiroSuda commented Dec 27, 2021

jandubois commented Dec 27, 2021

jwehrlich commented Dec 27, 2021

jwehrlich commented Dec 27, 2021 • edited

jandubois Dec 27, 2021

Choose a reason for hiding this comment

jandubois commented Dec 27, 2021

jwehrlichskytap commented Dec 27, 2021 • edited

jwehrlichskytap commented Dec 27, 2021

jandubois commented Dec 27, 2021 • edited

jandubois Dec 27, 2021

Choose a reason for hiding this comment

jandubois commented Dec 27, 2021

jandubois Dec 27, 2021

Choose a reason for hiding this comment

jwehrlich commented Dec 28, 2021 • edited

AkihiroSuda Dec 28, 2021

Choose a reason for hiding this comment

jandubois commented Dec 28, 2021

jandubois left a comment

Choose a reason for hiding this comment

jandubois Dec 28, 2021

Choose a reason for hiding this comment

jandubois Dec 28, 2021

Choose a reason for hiding this comment

jwehrlich commented Dec 28, 2021

jwehrlich commented Dec 28, 2021

jwehrlich left a comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

jwehrlich commented Dec 26, 2021 •

edited

jandubois commented Dec 27, 2021 •

edited

jwehrlich commented Dec 27, 2021 •

edited

jwehrlichskytap commented Dec 27, 2021 •

edited

jandubois commented Dec 27, 2021 •

edited

jwehrlich commented Dec 28, 2021 •

edited