Add restic_mode variable #42

jkirk · 2018-12-20T14:58:05Z

It should be possible to allow all users to run the restic binary.
For that the variable restic_mode should be set to 0755 to set the
permissions of the file accordingly.

paulfantom · 2018-12-20T15:07:41Z

What would be the purpose of that?

jkirk · 2018-12-20T17:48:52Z

I sometimes want to allow all my users on the system to use the restic binary (mostly Linux Desktop systems). I then need restic to have executable right for other. (Like rsync for example).

paulfantom · 2018-12-20T23:24:11Z

Then what is the purpose of using ansible for that? This role is aimed primarily at server usage where you automate your backup configuration (that's why we have those cron file templates) and not at only providing restic binary.

jkirk · 2019-01-21T11:52:38Z

(Sorry for the delay, somehow missed your answer).

I think I get your point, but in our scenario we (the admins) would like to be able to run restic snapshots and the like without sudo permissions.

Or do you think that allowing restic to be run as "ordinary" user is somehow risky? (Changing the mode of the binary is completely optional after all).

paulfantom · 2019-01-21T15:35:36Z

I don't see any risk in running restic binary as an ordinary user and changing binary permissions to 0755 seems ok to me. @TheLastProject what do you think?

TheLastProject · 2019-01-21T16:35:43Z

cap_dac_read_search+ep is set currently, so that would give ANY user read access to ANY file. So we definitely shouldn't unless we make sure that's not set

jkirk · 2019-01-21T17:12:20Z

@TheLastProject Sorry, you lost me here. Why / Where are these capabilities set? Is this somehow specific to restic itself (shall we file a security bug there)?

TheLastProject · 2019-01-21T17:50:59Z

It's set here:

ansible-restic/tasks/install.yml

Lines 77 to 82 in 8fc1da0

    
           - name: Set proper capabilities for restic binary 
        
             capabilities: 
        
               path: '{{ restic_install_path }}/restic' 
        
               capability: cap_dac_read_search+ep 
        
               state: present 
        
             when: not ansible_check_mode

Normally, it improves security, because restic could run as another user than root but still back up every file, see https://github.com/paulfantom/ansible-restic#security. However, if you give everyone execute permission, this becomes a serious security concern. I'm all for supporting a way to give everyone execute permission, but we need to make sure to unset it then.

It should be possible to allow all users to run the restic binary. For that the variable restic_mode should be set to 0755 to set the permissions of the file accordingly.

@TheLastProject

…ute permission In PR paulfantom#42 we talked about that it should be possible to to allow all users to run the restic binary. It was objected by @TheLastProject that the capability cap_dac_read_search is set and that would give ANY user read access to ANY file. To prevent that, the capability should only be set if "other" users have no execute permission on the restic binary. But on the the other hand, if a restic_group other than 'root' is set, we need the capability, so setting it in that case.

jkirk · 2019-05-21T23:29:46Z

Hey @paulfantom, @TheLastProject,

sorry for the very long delay.

What about only setting the capability when the "other" execute permission is not set? (I updated my PR (also rebased it on latest master)).

TheLastProject · 2019-05-22T05:13:08Z

Well, you'd also want to unset it if that bit is set, otherwise doing a rollout and then deciding "oh nvm I actually want to allow everyone to use it" will still open you to the same security issue.

jkirk · 2019-05-22T22:30:09Z

Oh, yes... Good point! I'll update my PR...

jkirk

I've made some remarks on the code. Please have a look, so I can update my PR. Thx!

jkirk · 2019-05-22T22:44:30Z

tasks/install.yml

@@ -80,6 +85,7 @@
  when:
    - ansible_os_family | lower == "debian"
    - restic_user != 'root'
+    - restic_group != 'root'


@TheLastProject I think this line is wrong too, right? libcap2-bin should be installed if restic_user OR restic_group are not 'root', like this:

- restic_user != 'root' or restic_group != 'root'

I think this is @paulfantom's code, but it seems to me that that indeed should be changed.

jkirk · 2019-05-22T22:44:55Z

tasks/install.yml

@@ -88,4 +94,6 @@
    state: present
  when:
    - restic_user != 'root'
+    - restic_group != 'root'


Same as above, right?

Same as above.

jkirk · 2019-05-22T22:48:00Z

tasks/install.yml

@@ -88,4 +94,6 @@
    state: present
  when:
    - restic_user != 'root'
+    - restic_group != 'root'
    - not ansible_check_mode


@TheLastProject This does not come from my PR, but still: what is this line for? The tasks are always run when not ansible_check_mode, no? Did I make a mistake in my thinking?

.. and skipped when in Check/Test-Mode.

I think it's just to not fail if the capabilities are not set

jkirk · 2019-05-22T23:06:19Z

And: shouldn't we also unset the capability when owner and group are 'root' (again)?

(I so sorry for this comment flood... )

TheLastProject · 2019-05-23T10:48:44Z

And: shouldn't we also unset the capability when owner and group are 'root' (again)?

(I so sorry for this comment flood... )

Well, yup, we talked about that:

Well, you'd also want to unset it if that bit is set, otherwise doing a rollout and then deciding "oh nvm I actually want to allow everyone to use it" will still open you to the same security issue.

EDIT: Actually I misread. When owner and group are root there are read rights for anything anyway, but it couldn't hurt just for the sake of consistency I suppose.

jkirk added 2 commits May 22, 2019 00:56

Add restic_mode variable

ec419b7

It should be possible to allow all users to run the restic binary. For that the variable restic_mode should be set to 0755 to set the permissions of the file accordingly.

jkirk force-pushed the restic-mode branch from 8fc1da0 to ab82f5c Compare May 21, 2019 23:25

jkirk commented May 22, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restic_mode variable #42

Add restic_mode variable #42

jkirk commented Dec 20, 2018

paulfantom commented Dec 20, 2018

jkirk commented Dec 20, 2018

paulfantom commented Dec 20, 2018

jkirk commented Jan 21, 2019

paulfantom commented Jan 21, 2019

TheLastProject commented Jan 21, 2019

jkirk commented Jan 21, 2019

TheLastProject commented Jan 21, 2019

jkirk commented May 21, 2019

TheLastProject commented May 22, 2019

jkirk commented May 22, 2019

jkirk left a comment

jkirk May 22, 2019

TheLastProject May 23, 2019

jkirk May 22, 2019

TheLastProject May 23, 2019

jkirk May 22, 2019

jkirk May 22, 2019

TheLastProject May 23, 2019

jkirk commented May 22, 2019

TheLastProject commented May 23, 2019 •

edited

Loading

Add restic_mode variable #42

Are you sure you want to change the base?

Add restic_mode variable #42

Conversation

jkirk commented Dec 20, 2018

paulfantom commented Dec 20, 2018

jkirk commented Dec 20, 2018

paulfantom commented Dec 20, 2018

jkirk commented Jan 21, 2019

paulfantom commented Jan 21, 2019

TheLastProject commented Jan 21, 2019

jkirk commented Jan 21, 2019

TheLastProject commented Jan 21, 2019

jkirk commented May 21, 2019

TheLastProject commented May 22, 2019

jkirk commented May 22, 2019

jkirk left a comment

Choose a reason for hiding this comment

jkirk May 22, 2019

Choose a reason for hiding this comment

TheLastProject May 23, 2019

Choose a reason for hiding this comment

jkirk May 22, 2019

Choose a reason for hiding this comment

TheLastProject May 23, 2019

Choose a reason for hiding this comment

jkirk May 22, 2019

Choose a reason for hiding this comment

jkirk May 22, 2019

Choose a reason for hiding this comment

TheLastProject May 23, 2019

Choose a reason for hiding this comment

jkirk commented May 22, 2019

TheLastProject commented May 23, 2019 • edited Loading

TheLastProject commented May 23, 2019 •

edited

Loading