honor executable flag in Create file request

[stapelberg]

The following C program illustrates the problem:

int main() {
  open("/mnt/sshfs/executable", O_CREAT | O_EXCL | O_WRONLY, 0755);
}

Before this change, the file would be created using mode 0644, despite the caller requesting mode 0755.

[stapelberg]

1. We are not supporting this version of the filexfer standard linked to. We are only supporting https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-02 And the File Attributes object between the two standards are explicitly incompatible.

Turns out that’s actually what I need? At least sshfs(1) correctly sends the permission attribute.

If it’s easy enough to explain, how come there are more recent standards, but software implements an older version?

2. This code also fails to properly enact the standard linked to. This specific flag is set on the acl-flags uint32 field in the ACL field of the File Attributes struct: https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-13#section-7 not on the pflags field of the SSH_FXP_OPEN packet, where this flag would conflict with the already defined, and used sshFxfExcl = 0x00000020 flag.

Oops! Indeed, my testing was insufficient. For testing the 0644 file, I was using touch(1), which happens to not set O_EXCL.

In order to properly support this, we would need to be retrieving the attrs struct properly (it is currently being straight ignored), and then use the given permissions field if set. Such an undertaking only really makes sense within the context of moving to the new filexfer package, which already unmarshals the attrs struct, but this is still a fair bit of an undertaking and not a simple process.

Thanks for the pointer to the filexfer package. I updated the code to use the filexfer package for unmarshaling, locally in sshFxpOpenPacket.UnmarshalBinary for now until the whole package is updated to switch to filexfer.

Please take another look. Thanks!

[puellanivis]

While I appreciate your enthusiasm, the method by which you are trying to retrofit in the filexfer package into the current design is extremely hacky, and it is probably not going to end up doing what you want.

I wanted to avoid just closing down this PR without a resolution, but I really don’t think resolving this feature should be a priority right now.

[puellanivis]

If it’s easy enough to explain, how come there are more recent standards, but software implements an older version?

This version is basically what every sftp client and server start at, and then they renegotiate up to a later compatible version. Thus filexfer-02 is the minimal layer of support, and the most widely adopted version, and it’s basically the same version that everyone is using. Considering that openssh pushed out a bunch of extensions to work around bugs that were addressed in later versions (e.g. hardlink, and fsync) it’s pretty clear that many of the later versions are significantly more complicated than they really need to be to just get file transfer done.

Basically, think of it like filexfer-02 meets 99% of use cases, and then covers 0.99% more through extensions. Oh and also, openssh either implemented the SSH_FXP_SYMLINK backwards, but since it is the central and canonical implementation, everyone follows the bug, rather than the specification.

It’s really kind of interesting history, but more than I really want to spend a ton of time on right now.

[stapelberg]

the method by which you are trying to retrofit in the filexfer package into the current design is extremely hacky, and it is probably not going to end up doing what you want.

I agree it might not be the cleanest design. But aside from that, what’s wrong with the code? It worked fine in my test.

I really don’t think resolving this feature should be a priority right now.

That’s too bad. I’d like to use sshfs(1) and this SFTP package to improve the developer experience of working with my https://gokrazy.org/ application platform by making the go tool install binaries directly onto the remote file system. These binaries currently end up without the executable bit, so a workaround is required :-/

This PR seems like a small-enough fix to me. If you’re concerned about breakage (?), maybe we can restrict permission handling to the executable bit? That’d be a strict improvement over the status quo and good enough for my use-case.

[puellanivis]

So, I absolutely understand your frustration here, this is a missing part of the standard that we really should be supporting, but I also don’t want to make things worse in between trying to make them better.

As is, the PR entirely duplicates decoding the SSH_FXP_OPEN packet; newpkt.Attrs.Permissions > 0 is not the way to be checking to see if the Permissions field is valid; the newpkt.Attrs.Permissions field is not able to be directly cast into an os.FileMode type, as the bit fields are different between the two; by adding an os.FileMode field into the sshFxpOpenPacket it implies that this will symmetrically encode into the marshaled packet, though nothing in the PR changes the way the packet is marshalled.

The PR seems small enough, sure, but there are pretty big issues with not just the implementation, but the design. As another example, ”maybe we can restrict permission handling to the executable bit” this is not actually possible. The only way to send this permission detail in the packets requires implementing this properly. There is no where we can just stuff this executable permissions bit into the packet other than where it is supposed to be.

I would really love to just bang out some code and get this feature rolled out for you, but this PR is not the way to get the feature done, I’m sad to say.

[puellanivis]

Consider this PR https://github.com/pkg/sftp/pull/384/files where I wrote in support to handle the PFlags field more correctly, I almost had to rewrite the whole in-memory filesystem implementation, and sadly, I see this ending up in a similar sort of situation. This is a place where we went with an expedient implementation over strictly standards compliant, and 😢 that means we need to overhaul a lot of things to get it done right.

[stapelberg]

As is, the PR entirely duplicates decoding the SSH_FXP_OPEN packet; newpkt.Attrs.Permissions > 0 is not the way to be checking to see if the Permissions field is valid; the newpkt.Attrs.Permissions field is not able to be directly cast into an os.FileMode type, as the bit fields are different between the two; by adding an os.FileMode field into the sshFxpOpenPacket it implies that this will symmetrically encode into the marshaled packet, though nothing in the PR changes the way the packet is marshalled.

Thanks for elaborating, I fixed all of the above.

The PR seems small enough, sure, but there are pretty big issues with not just the implementation, but the design.

I still don’t quite see the design issues. What you explained so far I would consider minor issues that were quick to address.

but this PR is not the way to get the feature done, I’m sad to say.

Would you prefer if we abandoned this PR? If so, please close it.

[puellanivis]

No, you didn’t address them all. You’re still casting sshfx.FileMode directly between os.FileMode. These values are not bit compatible.

Newly:

• The sshFxpOpenPacket.MarshalBinary is not implemented correctly, because it will never encode the Perms field.
• The sshFxpOpenPacket.UnmarshalBinary should only ever correctly and exactly reflect the actual values of the packet received. Interpreting what the default permissions should be is a piece of business logic, not part of the packet-encode/decode logic.
• The implementation here ignores the request server implementation. (Since the Request.Flags field is overloaded between Setstat and Open that means implementing this is going to be non-trivial.)

And I will reformulate my primary concern: I don’t want to piecemeal roll out filexfer implementations, and I definitely do not want to roll it out by stuffing the filexfer mashaling/unmarshaling into the principle package’s packet structs. We should be striving towards simplifying code, rather than raising the abstraction layers, and producing emergent bug behaviors.

[puellanivis]

I want to tag this into https://github.com/pkg/sftp/issues/335 where we can better discuss how to move forward with a vision toward a long-term solution and box ourselves into something by being in a rush.