Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build fails due to setting xattrs that affect SELinux, "permission denied" #8601

Closed
decathorpe opened this issue Oct 11, 2022 · 3 comments
Closed
Labels
build Issues caused by or requiring changes to the build system (scripts or Docker image) frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Milestone

Comments

@decathorpe
Copy link

I discovered this when running tests for syncthing v1.22, which fail on a system with SELinux enabled with this error message:

--- FAIL: TestXattr (0.00s)
    basicfs_test.go:597: set xattrs /tmp/TestXattr2645321982/001/test: Removexattr "security.selinux": permission denied
FAIL
FAIL	github.com/syncthing/syncthing/lib/fs	23.380s
FAIL

I assume that removing the security.selinux xattr from a file is not allowed unless the user has acquired superuser privileges first, so syncthing should probably leave attributes that are related to SELinux alone (as far as I can see, that's only the security.selinux attribute).


syncthing version: v1.22.0
operating system: tested on Fedora Linux 36 ,37, and Rawhide (development branch)
architecture: x86_64

@decathorpe decathorpe added bug A problem with current functionality, as opposed to missing functionality (enhancement) needs-triage New issues needed to be validated labels Oct 11, 2022
@calmh
Copy link
Member

calmh commented Oct 12, 2022

Yes, lots of attributes will require elevated permissions, as noted in the docs. What isn't noted in the docs yet is that you can filter out attributes you don't want to sync;

<folder id="...">
  ...
  <syncXattrs>true</syncXattrs>
  <xattrFilter>
    <entry match="security.*" permit="false"></entry>
    <entry match="*" permit="true"></entry>
  </xattrFilter>
</folder>

(the xattrfilter being a somewhat complicated config value means it's not currently supported in the config editor...)

@decathorpe
Copy link
Author

So .. is the test failure harmless because this will not occur in real-world usage, or does the failure indicate that the feature is not ready for systems where SELinux is enabled?

@calmh
Copy link
Member

calmh commented Oct 12, 2022

The test failure is because the test doesn't expect there to be attributes on the newly created test file that it can't touch, because I didn't expect that when I wrote the test. In real world usage this will pop up when the user checks sync xattrs and isn't running as a privileged user and isn't filtering xattrs. I think that's expected. However it would be nice if it were possible to build on an selinux-enabled system as well...

@calmh calmh added build Issues caused by or requiring changes to the build system (scripts or Docker image) and removed bug A problem with current functionality, as opposed to missing functionality (enhancement) needs-triage New issues needed to be validated labels Oct 12, 2022
@calmh calmh changed the title v1.22: setting xattrs that affect SELinux fails with "permission denied" Build fails due to setting xattrs that affect SELinux, "permission denied" Oct 30, 2022
@calmh calmh closed this as completed in bf1e418 Nov 3, 2022
calmh added a commit to imsodin/syncthing that referenced this issue Nov 8, 2022
* main: (36 commits)
  lib/protocol: Ignore inode time when xattr&ownership is ignored (fixes syncthing#8654) (syncthing#8655)
  lib/fs: Try to remove read only Windows files (fixes syncthing#3744) (syncthing#8650)
  gui: Add copy to clipboard, share by email, and share by SMS buttons to device IDs (fixes syncthing#2771, ref syncthing#3868) (syncthing#7984)
  gui, man, authors: Update docs, translations, and contributors
  build: Add GitHub actions build for Windows (syncthing#8627)
  gui: Fix connection type icon width (fixes syncthing#8592) (syncthing#8644)
  gui: Adjust connection type icon size scaling and alignment (syncthing#8645)
  docker: Use healthcheck endpoint (syncthing#8640)
  lib/connections: Use adaptive write size for rate limited connections (fixes syncthing#8630) (syncthing#8631)
  gui: Mark devices that haven't connected for a long time (fixes syncthing#7703) (syncthing#8530)
  gui: Fix rescan interval when add encrypted folder with watch for changes enabled (fixes syncthing#8570) (syncthing#8571)
  gui: Always show Out of Sync Items for remote devices (syncthing#8632)
  lib/fs: Let xattr test avoid non-test attributes (fixes syncthing#8601) (syncthing#8628)
  build: Add GitHub actions build for Windows
  gui, man, authors: Update docs, translations, and contributors
  gui: Display folder and device count number (syncthing#8615)
  gui, man, authors: Update docs, translations, and contributors
  lib/model, lib/protocol: Fix file comparisons (fixes syncthing#8594) (syncthing#8603)
  lib/scanner: More sensible debug output (syncthing#8596)
  gui: Allow automatic device ID selection on WebKit browsers (ref syncthing#8544) (syncthing#8597)
  ...
@calmh calmh added this to the v1.22.2 milestone Nov 9, 2022
@st-review st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Nov 3, 2023
@syncthing syncthing locked and limited conversation to collaborators Nov 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Issues caused by or requiring changes to the build system (scripts or Docker image) frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion
Projects
None yet
Development

No branches or pull requests

3 participants