_snapshot/backup methods flaw


(Meee!) #1

Hi, Simone,

we’ve stumbled over an issue when setting up backups on shared ES clusters.
Let’s assume you have a ROR configuration like this one:

...

  - username: xyz
    groups:
    - zyz_rw
    - xyz_ro
    proxy_auth: xyz
  - username: xyz_ro
    groups:
    - xyz_ro
    proxy_auth: xyz_ro
...
  - name: es-xyz reader
    x_forwarded_for:
    - es-xyz
    type: allow
    verbosity: error
    actions:
    - cluster:monitor/*
    - indices:admin/get
    - indices:admin/aliases/get
    - indices:admin/mappings/get
    - indices:admin/mappings/fields/get
    - indices:admin/types/exists
    - indices:admin/refresh*
    - indices:admin/validate/query
    - indices:admin/exists
    - indices:data/read/*
    - indices:monitor/stats
    - indices:monitor/recovery
    indices:
    - xyz-*
    - xyz_*
    - ".kibana_xyz"
    - ".kibana"
    groups:
    - xyz_ro
  - naxyz writer
    x_forwarded_for:
    - es-xyz
    type: allow
    verbosity: error
    indices:
    - xyz-*
    - xyz_*
    - ".kibana_xyz"
    - ".kibana"
    groups:
    - xyz_rw
...

With this configuration, xyz writer can do any _snapshot actions, including getting the snapshots of other users on the same cluster. It seems that no restrictions to the indices is applied. Specifically, any user can steal the indices of other users like this:

curl -XPOST -H 'x-forwarded-for: es-xyz' -H 'x-forwarded-user: xyz' http://localhost:9200/_snapshot/blabla/blabla-2018-01-17t15:20:26/_restore -d'{"rename_pattern": "acron_(.+)","rename_replacement": "xyz_blabla_$1"}'

This happily goes through, and xyz can now read the contents of index. The replacement name does not even have to match the index pattern of the user, it can be anything in fact.

We’ve worked around this by restricting the write actions by adding:

    actions:


- cluster:admin/snapshot/create <-
- cluster:admin/snapshot/status <-
- cluster:admin/tasks/*

This still allows users to see the indices other users have backed up but at least they cannot spy at them any longer. However, they cannot restore their own backups either any longer, and need the cluster admins for that.

Let us know if you need any additional information.

Cheers,
Ulrich


(Simone Scarduzio) #2

This bug is triggered because in Elasticsearch, RestoreSnapshotRequest does not implement the IndicesRequest interface and it’s not detected as indices-related by ROR. Therefore the indices rule won’t apply.

See the related bug report I opened in ES repo:

In the meanwhile, and to cover all the past ES releases, I explicitly took care of this case in ROR logic.

The fix is present in the current release 1.16.16 of ReadonlyREST for Elasticsearch. Please @schwicke have a look and see if there’s anything else we can do about this.


(Meee!) #3

Thanks a lot! We’re preparing to test the work around. I’ll let you know.


(Meee!) #4

We’ve done a brief testing of this. Users will still be able to query the snapshots of other users, simply because the snapshot names are arbitrary. They cannot steal foreign indices any longer with this patch.
Thanks a lot, that is good progress!

For the first issue I wonder if having another rule, similar to the “index: [list]” would do the job, like “snaphots: [list]”.


(Simone Scarduzio) #5

This is a great idea, now in the pipeline for the next release 1.16.17!


(Simone Scarduzio) #6

I implemented the “snapshots” rule that behaves like the actions rule (supports a list of snapshot names, with asterisks).

You can try it right now @schwicke, the only thing is that I didn’t yet implement the dynamic variables replacement. Do you need it?


(Meee!) #7

Coming back to this. Sorry for the delay, we’ve been rather busy with other stuff recently.

For backups we currently do not need dynamic variables.
While testing this, I wonder if the restriction is on the snapshot name or on the repository name.

If the restriction is on the actual snapshot name only, I wonder if that would still allow users to guess the repository name of others (eg. ‘backup’), and put their backups into foreign repositories ?


(Simone Scarduzio) #8

OK makes sense, what do you suggest to do?

  1. Harvest both repository and snapshot name in the “extractSnapshots” method and keep on using the snapshots rule?
  2. Or we can create a “repositories” rule that behaves like snapshots rule.

(Meee!) #9

Personally, I have a small preference for the latter because it looks a bit cleaner and quite flexible to me.