Skip to content
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

fix: add support for attrs in sshFxpOpenPacket for Server #567

Closed
wants to merge 6 commits into from

Conversation

mafredri
Copy link
Contributor

@mafredri mafredri commented Dec 12, 2023

Sending permissions (chmod) and atime/mtime is not uncommon for sftp clients via the sshFxpOpenPacket, however, flags/attrs seems to have been explicitly ignored even though they're part of the RFC (https://filezilla-project.org/specs/draft-ietf-secsh-filexfer-02.txt).

This leads to issues like mutagen-io/mutagen#459, where the client is relying on the attrs being applied.

This only adds support for the feature in the old Server implementation, which is what I used. Further work is most likely required to support it in the RequestServer as well (however, I have no experience with that part of the code base). I believe no backwards-incompatible changes were introduced since all we're doing is parsing more of the packet.

A bug was also fixed in sshFxpSetstatPacket and sshFxpFsetstatPacket where attrs were parsed in the wrong order.

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don’t think this has been done yet, just because it’s so particularly clunky.

And we kind of set out a plan for a new v2 in the internal/encoding/ssh/filexfer package that was supposed to clean up so much of this.

packet.go Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
server.go Outdated Show resolved Hide resolved
@mafredri mafredri force-pushed the fix-open-permissions branch 2 times, most recently from caeafbc to ed118bf Compare December 16, 2023 15:12
@mafredri
Copy link
Contributor Author

mafredri commented Dec 16, 2023

@puellanivis thanks for the thorough and timely review/feedback, as always. It's great to come back to this project every once in a while and see that it's still kicking. ☺️

I believe I've amended all of the feedback (at least what I felt confident/comfortable changing), including a little bit of cleanup of the old code.

@mafredri
Copy link
Contributor Author

mafredri commented Jan 5, 2024

@puellanivis hey, just wanted to check if there are any blockers/actions left for this PR? Cheers, and happy new year!

Copy link
Collaborator

@puellanivis puellanivis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in re-reviewing. Since the code itself is pretty much fine with only a simple concern, this time I’ve taken a closer look at the test.

b := p.Attrs.([]byte)
var err error
return statusFromError(p.ID, applyAttrsToFile(f.Name(), p.Flags, b))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dear. :( I’m unsure of how this subtle change from operating on a handle to operating on the filename would work. We’ve run into issues before where performing operations on what should be an open handle but done to the filename, or on a filename but through an open/close block.

I know Chtimes only exists for the filename, and not on a handle… but we could at least attempt to isolate this operation.

Also know that various systems will open a file, then delete it. This is a really weird corner case, but it seems like everyone runs into this weird pattern eventually and get tripped up assuming that a file will always be accessible from its filename while it is currently open.


tmppath := path.Join(os.TempDir(), "open_permissions")
pflags := flags(os.O_RDWR | os.O_CREATE | os.O_TRUNC)
ch := make(chan result, 2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This channel should never have more than one in flight. (The design of clientConn guarantees an “at most once” delivery on a channel given to dispatchRequest, and after the first dispatchRequest this goroutine blocks waiting for that response.)

Flags: sshFileXferAttrPermissions,
Attrs: []byte{0x0, 0x0, 0x1, 0xe5}, // 0o745 -- a slightly strange permission to test.
})
<-ch
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably test the error returned from the dispatchRequest channel return?

Why not actually just avoid channel tedium here entirely and use the slightly higher level sendPacket which will do the receive and extract the error from the result into a return value for you?

})
<-ch
stat, err := os.Stat(tmppath)
assert.NoError(t, err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be a require.NoError as if err != nil then stat == nil and the following line will panic.

server_test.go Show resolved Hide resolved
t.Logf("stat.Mode() = %v", stat.Mode())
}

os.Remove(tmppath)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this in a defer so that it gets performed even if a t.Fail is called.

})
<-ch
stat, err = os.Stat(tmppath)
assert.NoError(t, err, "mode should not have changed")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test failure message doesn’t make sense. We don’t know if the mode has changed or not yet. We just know that the Stat failed, and this could point to a deeper issue than just “mode should not have changed“.

Fix: just don’t put a log message here. Simply there being an error message popping up here is clue enough that an unexpected error occurred, and deeper review is necessary of what is happening.

@puellanivis
Copy link
Collaborator

Closing in deference to #574 which is pulling in the changes.

Thanks for your contribution though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants