bcpierce00 / unison

Unison file synchronizer
GNU General Public License v3.0
4.01k stars 227 forks source link

Fix permissions of new files when dontchmod=true #864

Closed tleedjarv closed 1 year ago

tleedjarv commented 1 year ago

As discussed on unison-users.

Props.fileDefault is based on umask.

Instead of just having Props.fileDefault in place of 0o600, I added a condition on the 'dontchmod' preference. That's because if chmod is allowed then before doing the final chmod the temporary file used during the sync may become world-readable (which is likely, given common defaults for umask). Similar code for mkdir omits this condition and always uses the default based on umask; this code has been in place since before the initial commit on GH (well before 2005). (An alternative to this condition could be to OR the expected final perms with 0o600 for the temporary file, but that code would be even more complex.)

Not tested on a cifs/smbfs mount.

gdt commented 1 year ago

So

To me the real issue is why is dontchmod sensible, and why do we think that if we can't call chmod that we can control permissions at all. It really feels like either we are syncing permissions or we aren't, and then either the system is deficient or it isn't. Supporting a request not to call a particular syscall seems a bit bizarre, and it seems further odd to manage permissions in that environment.

I can see just using umask if the set of permissions bits to sync is empty, and then not calling chmod because there is no need. That would bring us back to letting the user express semantics and then respecting that.

I think we also need to be clear on "user doesn't want to sync permissions" and "we are syncing to a filesystem that can't handle permissions". Maybe we allow -perms 0 and if that isn't given and there are errors, so be it?

tleedjarv commented 1 year ago

So

  • the standard approach is to create temporary files 0600, because we don't know what they are ending up as
  • if dontchmod is true, we try to create them with umask?

Correct. Additionally, when perms=0 and dontchmod=false then files end up with permissions based on umask.

This PR is only about restoring consistency between perms=0 and dontchmod=true. The inconsistency is quite clearly a bug; I think it is never noticed because almost nobody uses dontchmod with a filesystem that really supports chmod.

What follows below is a discussion separate from this PR, with more details on unison-users. I'll just add a few remarks to your comments.

To me the real issue is why is dontchmod sensible, and why do we think that if we can't call chmod that we can control permissions at all.

That's the point, we don't. (And I agree, I don't think either that it is sensible.)

It really feels like either we are syncing permissions or we aren't, and then either the system is deficient or it isn't. Supporting a request not to call a particular syscall seems a bit bizarre, and it seems further odd to manage permissions in that environment.

Moreover, syncing permissions while at the same time not syncing owners/groups may also produce completely different results.

I can see just using umask if the set of permissions bits to sync is empty, and then not calling chmod because there is no need. That would bring us back to letting the user express semantics and then respecting that.

I think we also need to be clear on "user doesn't want to sync permissions" and "we are syncing to a filesystem that can't handle permissions". Maybe we allow -perms 0 and if that isn't given and there are errors, so be it?

I agree, these two cases should have clear semantics. But there is a lot of flexibility that users either leverage to their advantage or can shoot themselves in the foot with. For example, what if I run the server under different user? What if I dont syn'c owner/groups? What if I use ACLs (that override basic permissons) but don't sync those (or the target fs doesn't support them)? I don't think we should nor could control all those cases; we only need to get the defaults good for most users.

gdt commented 1 year ago

SInce this is a bugfix for a scheme that may or may not make sense, I'm going to merge it and we can talk about dontchmod separately.