UnixFS default mode perm does not match current behaviour

[kalmi]

I have come across this while working on ipfs/kubo#6920.

ipfs get currently writes files with mode perm 666, but thanks to the most common default umask, this usually results in 644, which is the default fallback specified by the UnixFS spec.

Changing ipfs get to use 644 as specified could in theory be breaking change for some with unusual umasks.

Is this something to be concerned about? Should I just use the default as specified? Or should the spec be changed? Or is there some alternative solution I am not seeing?

[ribasushi]

My 2c would be that the original of 666 was a dangerously-incorrect default, and we should switch to 644 in the course of implementing this.

The unlikely case you mentioned is almost certainly going to exist somewhere, but hopefully beta testing will uncover it quickly enough for us to re-assess.

For the time being I vote +1 to go with 644 and 755 as default modes on get

[achingbrain]

I think what we previously decided was to only set a mode if the UnixFS entry in question had a mode set on it.

If no mode is present in the UnixFS metadata, we were to continue doing what we did before.

[kalmi]

If we are to continue doing what we were doing when no UnixFS metadata is present, then the spec should be changed to match the actual current behaviour.

https://github.com/ipfs/specs/blob/master/UNIXFS.md currently specifies that if the optional mode field is unspecified, than it defaults to:
- 0755 for directories/HAMT shards
- 0644 for all other types where applicable

But reality is the following:

kalmi@kb ~ % ipfs get /ipfs/QmfGBRT6BbWJd7yUc2uYdaUZJBbnEFvTqehPFoSMQ6wgdr/readme       
kalmi@kb ~ % stat -f '%A %N' readme
644 readme
kalmi@kb ~ % umask 0000
kalmi@kb ~ % rm readme 
kalmi@kb ~ % ipfs get /ipfs/QmfGBRT6BbWJd7yUc2uYdaUZJBbnEFvTqehPFoSMQ6wgdr/readme
kalmi@kb ~ % stat -f '%A %N' readme
666 readme

It defaults to 666 currently for non-directories, and the most common default umask turns that into 644 for most users, and that's why it works for most users as described.

If the spec specifies 644 as the default, it would be reasonable to expect it to create files with mode perm 644 even if umask is set to 0000.

Maybe the solution is to remove the default perms from spec, and allow mode to be unspecified with no default just like mtime, and just let umask take care of it when mode is unspecified.

[ribasushi]

I think what we previously decided was to only set a mode if the UnixFS entry in question had a mode set on it.

I have the same reading of the spec as @kalmi. Additionally doing 666 by default is very dangerous, we really shouldn't be doing it unless absolutely requested by the user ( via undesirable metadata in-block )

[achingbrain]

My understanding of the spec is that it is also intended to cater for when you're only doing manipulations within IPFS, for example your mfs so I think the defaults should stay.

That said, I wouldn't be averse to changing the permissions of files written to disk by ipfs get to match the UnixFS defaults, my concern was more that we historically have tried not to change existing behaviour wherever possible.

[Stebalien]

My 2c would be that the original of 666 was a dangerously-incorrect default, and we should switch to 644 in the course of implementing this.

The current default is definitely correct for what it does: it obeys the umask. It would actually be nice to continue doing this when the mode is unspecified.

However, this is tricky from a UX & deduplication perspective:

1. We want to be able to display modes in ipfs ls, and the user's umask shouldn't affect it. We definitely don't want to display one set of permissions in ipfs ls but get a different set when we create the files.
2. If the mode is the "default" for the file type, we'd like to be able to omit it in IPFS to deduplicate with content that doesn't keep modes.

For now, I think the right solution is to set the mode to the unixfs default AND obey the umask. This should "just work" in most cases. This isn't perfect because a more restrictive umask (e.g., 0700) will zero out the group/other permissions, but if that's what the user set, that's their problem.

[Stebalien]

my concern was more that we historically have tried not to change existing behaviour wherever possible.

Note: In the go contributor guidelines, we say "don't break the ecosystem" to try to capture some of the nuances here. Small breaking changes are fine in many cases as long as they have minimal impact and can easily be worked around. In this case, changing the default to 0644 would only affect users with modes set to 0664 or 0666 (both of which are not recommended).

[kalmi]

Thanks for all the discussion. I agree with the ~umask&default approach.

(I decided to have a look at how tar handles it, and I was horrified at first. By default, tar applies the umask when the user is a regular user, and by default ignores it when the user is root, and there are cli options to control this behaviour. After some thought I think it's actually a good default behaviour for tar's use cases, but it's a surprising behaviour.)

[ribasushi]

@kalmi you bet! Please close the ticket if there are no further questions on this subject for the time being. You can always reopen it later if the need arises.