-
Notifications
You must be signed in to change notification settings - Fork 380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
request server: handle relative symlinks #526
Conversation
ac03a58
to
2b5a4f9
Compare
I don't have time to test this change now (I'll do it later today or this weekend), I think this change will allow symlinks outside the base directory. Applications enforcing a chroot such as SFTPGo should check the symlink target and normalize it themselves after this change. This is acceptable but maybe should be documented as a backward incompatible change. |
I just did a basic test with SFTPGo: Escaping the chroot (using cd or get) didn't seem to work - fine so far. SFTPGo is restricting the link target of the symlink command pretty strong - but since symlinks could exist from other sources (directly in filesystem using But yes, SFTPGo would need to change its implementation to handle relative symlinks correctly.
Symlink |
Just as a note: The last comment and tests were about the current SFTPGo implementation without the relative symlink patch applied to sftp. |
Yes, I noticed this morning, after your initial comment that relative symlinks are not handled correctly in SFTPGo. I'll give a look this weekend, thanks |
request-server_test.go
Outdated
@@ -590,6 +604,17 @@ func TestRequestSymlink(t *testing.T) { | |||
require.NoError(t, err) | |||
assert.Equal(t, []byte("hello"), content) | |||
|
|||
fi, err = r.lfetch("/link-to-non-existent-file") | |||
require.NoError(t, err) | |||
assert.True(t, fi.Mode()&os.ModeSymlink == os.ModeSymlink) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using assert.True
it can be very useful to put in some sort of explaining communication about what this is actually testing, otherwise the test just fails with expected: true \n got: false
which is pretty much not helpful at all.
However, I suggest instead, skipping the whole assert
library† and just write the test clearly:
if fi.Mode() & os.ModeSymlink != os.ModeSymlink {
t.Error("expected mode to be a symlink, but was: ", fi.Mode())
}
†: I recommend skiping these assert libraries entirely and always, but we’re already using it heavily here. There is a reason why Golang doesn’t have a standard library assert package, and it’s because it leads people to be careless about their tests, and obfuscates how the code should be used. Namely, test code should be as close to example code as possible. (Bonus: if writing real production-quality code in your tests is cumbersome, it’s a signal to fix your API, not wallpaper over the burden of writing the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was just a copy of the symlink test from the same testcase. I will rework this one and the others aswell
request-server_test.go
Outdated
assert.True(t, fi.Mode()&os.ModeSymlink == os.ModeSymlink) | ||
|
||
_, err = r.fetch("/link-to-non-existent-file") | ||
require.True(t, os.IsNotExist(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, after adding the explanatory details we’re not getting anything out of the require
package:
if !os.IsNotExist(err) {
t.Fatal("expected file to not exist, but it did")
}
I've done some testing and this patch looks good to me. I think we just need to document somewhere that Filepath can be relative for symbolic links now. @puellanivis do you have any suggestions other than the above for test cases? |
@georgmu please add some test cases for relative links from a dir different from
Thanks |
Nope, not really. Tried my hardest, and only the test code was something I could see anything at all in. |
I will add some more testcases, also the one mentioned in my comment about moving the parent directory. I will also try the testcases with 98b35dc reverted (just to check that it worked before). |
just to make sure what these commands do as checked with sftp CLI client and standard sftp server:
I would reverse the names so that the symlinks are named |
Yes, reverse the names, thanks |
As I understand sftp, |
Yes, you are right, you already included a test like this:
I added such tests to SFTPGo because I have to handle other things there |
One more thing here: From my understanding, a symlink is just a textfile whose content gets interpreted by the virtual file system. So if I write chroot'ing does not alter the content ("target") of the link, only the interpretation of the content.
This is how symlinks work and I think the library should do the same: not interpreting the link target. What a chroot-in-software (what SFTPGo does) should do sanity checks whenever reading a link (like interpreting absolute links as links from the chroot and check that relative links do not escape the chroot), but the library should return them as is. |
The library should pass the cleaned path for readlink requests, the request server implementations (SFTPGo or others) can handle the request according to their own logic, which could be the one you describe or a different one
SFTPGo resolves the link and if it is outside the chroot directory it will return an error like this:
|
Currently, it returns a relative path, even if an absolute path is generated.
I just checked the code: openssh sftp-server also just returns the content of |
The problem with Line 584 in 3aa4378
file.Name() only returns the base name of the file (as specified in docs for os.FileInfo), but this is wrong here. Solution would be to depend on something other than os.FileInfo here or return a custom Sys() result. |
ah ok, now I better understand what you mean. In SFTPGo I use this workaround https://github.com/drakkan/sftpgo/blob/main/internal/sftpd/handler.go#L263 I have custom fileinfo and for the Readlink case I set fullName to I agree we should fix this issue in a better way (I use the same hack also for FTP). |
As for the server side path normalization, the library always cleaned paths (I'm not the original author), the only expection is the request.Filepath for symlinks introduced in this PR. I suggest to discuss this in a separate issue/PR, thanks |
2b5a4f9
to
98d9618
Compare
I just uploaded a solution for both symlink and readlink stuff with extended test cases. The readlink fix is a bit hacky and requires some documentation and/or another type. |
I'm not sure about the readlink fix. You have to use a custom fileinfo implementing the new Readlink is currently broken, I think we can break backward compatibility for something that does not work or that works with hacks like the one I used in SFTPGo. @puellanivis what do you think about? (I already know you disagree 😄 ) |
@georgmu I'm now using your initial patch in SFTPGo drakkan/sftpgo@0e8c41b#diff-d48bed73cb243b5300108a318eea6494652d40093322e498e5fd5d098725ef33 thanks! |
We could do something like this:
@puellanivis, @georgmu do you have other/better suggestions? thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure on the other questions being asked. I’m a little sick right now, and finding it difficult to follow things well enough.
request.go
Outdated
if f, err := rl.ReadLink(); err != nil { | ||
return statusFromError(pkt.id(), err) | ||
} else { | ||
filename = f | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is better written as:
f, err := rl.ReadLink()
if err != nil {
return statusFromError(pkt.id(), err)
}
filename = f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do something like this:
diff --git a/request-interfaces.go b/request-interfaces.go index 2e5ee6b..ab5f5a1 100644 --- a/request-interfaces.go +++ b/request-interfaces.go @@ -94,7 +94,7 @@ type LstatFileLister interface { // // Up to v1.13.5 the signature for the RealPath method was: // -// RealPath(string) string +// # RealPath(string) string // // we have added a legacyRealPathFileLister that implements the old method // to ensure that your code does not break. @@ -104,6 +104,13 @@ type RealPathFileLister interface { RealPath(string) (string, error) } +// ReadlinkFileLister is a FileLister that implements the Readlink method. +// TODO: complete docs +type ReadlinkFileLister interface { + FileLister + Readlink(string) (string, error) +} + // This interface is here for backward compatibility only type legacyRealPathFileLister interface { FileLister diff --git a/request.go b/request.go index 6a7e6cf..4db3f54 100644 --- a/request.go +++ b/request.go @@ -295,7 +295,25 @@ func (r *Request) call(handlers Handlers, pkt requestPacket, alloc *allocator, o return filecmd(handlers.FileCmd, r, pkt) case "List": return filelist(handlers.FileList, r, pkt) - case "Stat", "Lstat", "Readlink": + case "Stat", "Lstat": + return filestat(handlers.FileList, r, pkt) + case "Readlink": + if readlinkFileLister, ok := handlers.FileList.(ReadlinkFileLister); ok { + resolved, err := readlinkFileLister.Readlink(r.Filepath) + if err != nil { + return statusFromError(pkt.id(), err) + } + return &sshFxpNamePacket{ + ID: pkt.id(), + NameAttrs: []*sshFxpNameAttr{ + { + Name: resolved, + LongName: resolved, + Attrs: emptyFileStat, + }, + }, + } + } return filestat(handlers.FileList, r, pkt) default: return statusFromError(pkt.id(), fmt.Errorf("unexpected method: %s", r.Method))
@puellanivis, @georgmu do you have other/better suggestions? thanks
I think this sounds like a good solution and is done in the same way as the LstatFileLister.
Another option would be a bigger rewrite but this would brake existing code: Make Handlers
an Interface (it is used way in most cases anyway where FileGet/FilePut/FileCmd/FileList all point to the same implementation) and make the now optional interfaces mandatory with a hint on what to return when support is not necessary. Go is lacking features like default implementations for interfaces like in rust...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be a bigger rewrite but this would brake existing code: Make Handlers an Interface (it is used way in most cases anyway where FileGet/FilePut/FileCmd/FileList all point to the same implementation) and make the now optional interfaces mandatory with a hint on what to return when support is not necessary. Go is lacking features like default implementations for interfaces like in rust...
This will require a jump to V2. But this is something that we really do need to get done. Default implementations can be done by allowing embedded types that implement the default. See protobuf and their “UnimplementedXYServer”.
We can also cover default implementations through interface extensions as we have been doing a few of the newer implementations.
I understand that it is hard to shift out of a familiar paradigm where rust provides for default implementations, but there exists an equivalent in Go. You just have to often think about things from a different perspective.
I try to summarize:
we'll break backward compatibility in v2. If we all agree with this approach we can add the |
@georgmu are you interested to add the |
Sure. I will prepare the commits. |
98d9618
to
ec65d7d
Compare
I prepared a commit for the |
f6172b6
to
40ee72a
Compare
Thanks! You are much better than me at writing docs 😄 The PR LGTM. Maybe you can also test returning an error from a readlink request. There is no coverage: |
40ee72a
to
40d3e29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This PR is ok for me. I'll wait a few more days to see if @puellanivis has any other suggestions before merging this patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That I am providing feedback only on documentation and string format verbs is a testament to the code being pretty great. 👍
request-interfaces.go
Outdated
@@ -74,6 +74,10 @@ type StatVFSFileCmder interface { | |||
// FileLister should return an object that fulfils the ListerAt interface | |||
// Note in cases of an error, the error text will be sent to the client. | |||
// Called for Methods: List, Stat, Readlink | |||
// For Readlink, Filelist is limited to os.FileInfo response. For a more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might recommend a paragraph break here. The added whitespace makes the separation of the purpose of the two comments more clear.
Also, I know the existing text looks like it’s using a 80 column break, each of the three existing lines is a distinct individual semantic statement. In Golang, you don’t want to be breaking up lines just because you’re fitting a column width, but rather because they’re natural breaks. So more https://sembr.org/
So:
// For Readlink, Filelist is limited to os.FileInfo response.
// For more sophisticated Readlink response, please implement ReadlinkFileLister.
// If ReadlinkFileLister is implemented, its Readlink method is called for method Readlink instead of calling Filelist.
I’d probably also suggest being more explicit about what “sophisticated” is supposed to mean. So maybe even better:
// Since Filelist returns an `os.FileInfo`, this can make it non-ideal for impelmenting Readlink.
// This is because the `Name` receiver method defined by that interface defines that it should only return the base name.
// However, `Readlink` is required to be capable of returning essentially any arbitrary valid path relative or absolute.
// In order to implement this more expressive requirement, implement `ReadlinkFileLister`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will just copy youre suggestion here
request-interfaces.go
Outdated
// ReadlinkFileLister is a FileLister that implements the Readlink method. | ||
// By implementing the Readlink method, it is possible to give a better | ||
// response than via the default FileLister, since Readlink can return | ||
// a better result than FileLister (which is limited to os.FileInfo, whose | ||
// Name() should only return the base name of a file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave this comment block as an exercise for the reader to improve with sembr. I think it’s probably fine and explicitly spelling out what is important.
40d3e29
to
6f60892
Compare
* add tests for sub directories * add tests for relative symlink targets * improve error messages
ReadlinkFileLister with its Readlink method allows returning paths without misusing the os.FileInfo interface, whose Name() method should only return the base name of a file. By implementing ReadlinkFileLister, it is possible to easily return symlinks of any kind (absolute, relative, multiple directory levels)
6f60892
to
9183e7f
Compare
The request server implementation does not handle symlinks correctly. If the link target is relative it is rewritten to an absolute path. The first commit fixes that.
The second commit is just adding a testcase for linking to non-existent files.