Open didactic-drunk opened 5 years ago
They should not be in permissions. The conflation of flags and permissions is an unneccesary UNIXism. The design of File::Info
and File::Flags
is fine. You're just missing a way to set/copy File::Flags
on a file.
In fact, setting suid, sgid, or sticky "by accident" is possible with your API when copying permissions - and dangerous to security.
My preferred API would be File.chmod(path, permissions : Int)
which converts the 0o655
to a File::Permissions
then passes that to File.chmod(path, permissions : File::Permissions, flags : File::Flags = File::Flags::None)
.
So we don't have future naming collisions:
3 API's that avoid security problems while keeping setuid
with permissions:
#
File.info("foo").permissions => Permissions without setuid.
# Choose a name.
File.info("foo").permissions_with_name => Permissions with `setuid`.
File.info("foo").full_permissions => Permissions with `setuid`.
Advantages:
to_s
shows correct information on File::Info
.Disadvantages:
#
permissions = File.info("foo").permissions
permissions.privileged # Sets flag indicating setuid/setgid is allowed when setting permissions.
File.chmod("bar", permissions.privileged)
Advantages:
to_s
always shows correct information.#
permissions = File.info("foo").permissions
permissions.strip_setuid # Or another name.
File.chmod("bar", permissions.strip_setuid)
to_s
always shows correct information.You said my API creates security holes on files. That's addressed in the prior post.
The current API creates security holes on directories by not retaining setgid and sticky bits.
My preferred API would be
File.chmod(path, permissions : Int)
which converts the0o655
to aFile::Permissions
then passes that toFile.chmod(path, permissions : File::Permissions, flags : File::Flags = File::Flags::None)
.
My preferred API would be:
# Creates a setuid file because that's exactly what I specified.
File.chmod(path, 0o4755)
# Now we can argue about implementation since permissions possibly
# came from some other API.
File.chmod(path, permissions)
Sometimes I need to import or export permissions to or from other file formats or programs. The current API makes this impossible (using Permissions
). The proposed API makes it a PITA.
Whether or not it setuid was conflated or is inherent to permissions is besides the point. They're always together in every OS that supports them.
Perhaps you have a use case that illustrates where this is beneficial and generates fewer bugs or fewer lines of code. I documented one of my use cases under "Background" where the current API doesn't work. Another program where crystal failed is a tripwire like program I attempted to port a month or 2 ago but couldn't because readlink
among others was missing. The proposed API won't work for that either.
Splitting permissions is a hindrance in actual use. I've avoided the crystal API for this reason.
They're always together in every OS that supports them
that's only because there's only one (family of) OS which supports them
it's not about fewer lines of code, it's about reducing the amount of breaking changes (removing File::Flags
) needed to support what is an uncommon usecase (setting non-permission bits).
uncommon usecase (setting non-permission bits).
Or comparing, or exporting, or importing. I admit they are all uncommon but it's not a single case. Every case of attempting to use them is made harder by splitting flags from permissions.
Where is the tangible benefit?
By splitting them permissions.to_s
shows incorrect file permissions.
I see your point that maybe POSIX flags could be it's own thing with it's own API and designed differently but it's not. It's always part of the file mode
.
mode
?chmod
?mode
as u16
which also happens to be the representation of File::Permissions
but is incompatible for no discernible reason?Even if you come up with a solution it's:
If you were doing an OS abstraction maybe there would be a win but this is a POSIX only feature.
@didactic-drunk we still need to do OS abstraction, especially because it's a POSIX-only feature.
I'd like others to chip in, but I'm unlikely to change my mind.
How would you do an OS abstraction?
How would I do an OS abstraction?
Permissions
adds additional setuid/setgid/sticky
bits.setuid
raise NotImplemented
.You still haven't given a tangible benefit. Or plans to fix '.to_s'. The first thing I thought when I saw the output was "How did my files lose setuid?".
For a tangible benefit what is the interface and implementation and how is it more robust?
I gave three ways to handle the issue with less code and very robust with [POSIX, Windows, 3rd party library and existing programmer expectations] compatibility.
Problem
The current API for dealing with setuid/setgid/sticky bits is over abstracted and missing useful features. I should be able to:
Permissions.to_s
show accuratesetuid/setgid/sticky
information.setuid/setgid/sticky
bits.File::Info::Permissions
toFile.chmod
without alteration.For
.to_s
to work the information must be contained inFile::Permissions
, not a separate structure.Passing the info to chmod requires the same.
Getting the actual POSIX bits requires the same.
Setting the bits probably requires the same.
Implementation
Permissions
.File::Permissions.[setuid?, setgid?, sticky?]
File::Info.[setuid?, setgid?, sticky?]
topermissions
.File::Flags
.Background
A few weeks ago I was working with
setuid
files and found it utterly frustrating as the information ispermissions
.permissions.to_s
shows incorrect values by omittingsetuid/setgid/sticky
status.Try copying a
setgid
file or directory in crystal and you'll see what I mean. What was a 3 line file copy adds 3 if statements.I found it quicker to monkey patch
File::Info
and work with the permissions directly.File::Flags
is pretty abstraction but not useful in the application space that uses it.