filesystem: allow .fscrypt to be a symlink #150
Conversation
filesystem/filesystem.go
Outdated
| @@ -129,6 +133,23 @@ func (m *Mount) BaseDir() string { | |||
| return filepath.Join(m.Path, baseDirName) | |||
| } | |||
|
|
|||
| // baseDirReal is like BaseDir, except it evaluates the symlink if the BaseDir | |||
| // is a symlink. | |||
| func (m *Mount) baseDirReal() (string, error) { | |||
There was a problem hiding this comment.
So this approch will work, but it seems like it would be simpler to just have BaseDir call filepath.EvalSymlinks and just return that for all BaseDir operations. That would also help keep the symlink code size down and in one place.
// BaseDir returns the path of the base fscrypt directory on this filesystem after resolving symlinks.
func (m *Mount) BaseDir() string {
dir, err := filepath.EvalSymlinks(filepath.Join(m.Path, baseDirName))
if err != nil {
panic(err)
}
return dir
}There was a problem hiding this comment.
That doesn't work because EvalSymlinks returns an error if the symlink target doesn't exist. So I went with:
func (m *Mount) BaseDir() string {
rawBaseDir := filepath.Join(m.Path, baseDirName)
target, err := os.Readlink(rawBaseDir)
if err != nil {
return rawBaseDir // not a symlink
}
if filepath.IsAbs(target) {
return target
}
return filepath.Join(m.Path, target)
}One thing I don't quite like about either approach is that it causes BaseDir() (which is called several times on each mount) to do syscalls rather than just return a string. The single readlink() isn't too bad in comparison to the full path walk done by EvalSymlinks() though, so I suppose that for simplicity in the code we should just go with that for now.
There was a problem hiding this comment.
This is much easier for me to understand. If it turns out BaseDir() making syscalls is a bottleneck for the fscrypt commands, we can always just cache the result (so that readLink is only ever called once per mount).
filesystem/path.go
Outdated
| @@ -56,6 +56,16 @@ func loggedStat(name string) (os.FileInfo, error) { | |||
| return info, err | |||
| } | |||
|
|
|||
| // loggedLstat runs os.Lstat (doesn't dereference trailing symlink), but it logs | |||
There was a problem hiding this comment.
This file would then be unnecessary.
There was a problem hiding this comment.
It's still useful to have isSymlink() for the test, so I've kept it.
There was a problem hiding this comment.
As these are test only I'm just going to move them into the test file (just to make it clear what code is test-only).
Support the case where the user has a read-only root filesystem (e.g. with OSTree) and had previously created a symlink /.fscrypt pointing to a writable location, so that login protectors can be created there. Resolves #131
|
@josephlr, any more comments on this pull request? |
filesystem/path.go
Outdated
| @@ -56,6 +56,16 @@ func loggedStat(name string) (os.FileInfo, error) { | |||
| return info, err | |||
| } | |||
|
|
|||
| // loggedLstat runs os.Lstat (doesn't dereference trailing symlink), but it logs | |||
There was a problem hiding this comment.
As these are test only I'm just going to move them into the test file (just to make it clear what code is test-only).
This makes it easier to understand which code is actually invoked by the command-line tool.
Support the case where the user has a read-only root filesystem (e.g.
with OSTree) and had previously created a symlink /.fscrypt pointing to
a writable location, so that login protectors can be created there.
Resolves #131