search

Azure / iot-identity-service

Source of the Azure IoT Identity Service and related services.
MIT License
37 stars 46 forks source link

feature/snapping: fix file permissions issues #520

Closed alexclewontin closed 1 year ago

alexclewontin commented 1 year ago

The aziotctl_common::config::write_file function first changes ownership of a file, then changes the permissions.

Changing the permissions on a file owned by another user requires CAP_FOWNER. While this is not normally a problem for root, the assumption that root ALWAYS has CAP_FOWNER is erroneous (snaps are a counter example).

This commit simply reorders the operation, so that the file is still owned by the creator when the permissions get changed.

NB: afaict this doesn't actually have any meaningful effects on the snapped identity service by itself (because everything runs as root, so no files ever actually change ownership.) It does impact iotedge, which leverages this code in a way that breaks in confinement.

arsing commented 1 year ago

It does impact iotedge, which leverages this code in a way that breaks in confinement.

Just to be clear, it breaks the iotedge snap, not regular non-snap iotedge, right? Because any uses of this code in non-snap iotedge would also happen as root.

alexclewontin commented 1 year ago

Just to be clear, it breaks the iotedge snap, not regular non-snap iotedge, right?

The way the code is currently only breaks in snap confinement, correct (or technically any other environment in which root user is subjected to normal DAC, and lacks CAP_FOWNER, but I don't know of any besides snaps). In snap confinement, the root user is sometimes subject to DAC in ways that it is not outside of snap confinement. This is an instance of that: root inside snap confinement does not have CAP_FOWNER, but root outside snap confinement does have CAP_FOWNER.

The proposed changes should work well either in or outside of confinement

arsing commented 1 year ago

One of the things that CI is complaining about is new in this PR, but the others have been there before, and I see commits being made to this feature branch despite the failures. I assume the intent is to fix them up when the branch is ready for review, and for now it's okay to just merge this manually, so doing that.

@veyalla @bishal41 FYI.