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

Document level security for ROR #326

Merged
merged 3 commits into from Mar 29, 2018

Conversation

Projects
None yet
6 participants
@rvibrac
Copy link
Contributor

commented Mar 23, 2018

Hi,

This pull request allows to set up document level security.
I kept it really simple for now, I added a new property to the rule named 'filter'.
You put a DLS query and it will wrap this query to the ES query if the rule is matched.
For example:

     access_control_rules:

    - name: Just certain indices, and read only
      actions: ["indices:data/read/*"]
      indices: ["product_catalogue-*"]
      filter: ""{\"bool\": {\"must\": [{\"term\": {\"name\": {\"value\": \"test\"}}}]}}""

So if the rule is matched, it will filter all the documents that have the 'name' equals to 'test'

Regards,

rvibrac added some commits Mar 6, 2018

@rvibrac

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

By the way, i'm working with @tcharlot-datasweet who made the following PR #308

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented Mar 23, 2018

Hey @rvibrac, thanks to you and all the lovely people from DataSweet.fr for this second, very significant contribution. This feature will get a lot of other people excited for sure.
By the way : so happy to have released this project as authentic Free & Open Source Software, without weird custom licenses, so this kind of things are free to happen.

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

return Base64.getEncoder().encodeToString(baos.toByteArray());
}

public static FilterTransient Deserialize(String userTransientEncoded) {

This comment has been minimized.

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

Repository owner deleted a comment from codacy-bot Mar 23, 2018

@fbaligand

This comment has been minimized.

Copy link

commented Mar 24, 2018

Thanks for this great contribution @rvibrac !

I have one question : does "filter" option support dynamic variable injection like this :
filter: "... owner:@{user} ..."

It would be very powerful, because it lets to authorize a user to only access his own documents.

@tcharlot-datasweet

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2018

Hi @fbaligand,

Not in this pull request. But we will release it soon.

@fbaligand

This comment has been minimized.

Copy link

commented Mar 24, 2018

Nice !

@rvibrac

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2018

Happy to help you all :)

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented Mar 29, 2018

Hi @rvibrac @tcharlot-datasweet am I right this PR opens the way to field level security too? BTW will finalize the review and merge today.

@sscarduzio
Copy link
Owner

left a comment

Lots of stuff here didn't get the 100% of some of the tricks you do. Overall looks good though, and all the tests are green.
The only thing I'd love to see in the future (after field level security and variables replacement) is move the "filter" away from being an attribute in Block, and make it a SyncRule.

@sscarduzio sscarduzio merged commit 27b3500 into sscarduzio:master Mar 29, 2018

2 of 3 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rvibrac

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2018

Hi @sscarduzio.
Yes this PR opens up for FLS. But we're not ready to do it now, we're a little busy at the moment.

Regarding the stuff you didn't get, could you enlight me with it ?

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented Apr 3, 2018

OK will try: why here it's important to disable cache? I mean, do we really always need to disable it even when the "filter" is not configured?

https://github.com/sscarduzio/elasticsearch-readonlyrest-plugin/pull/326/files#diff-9016a2dc553830aa8af5bed943d090d7R195

@rvibrac

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2018

In this simple configuration we could enable back the cache if the filter is not set. But keep in mind that we did it to avoid any side effects.
Here is one of the possible side effects : Two rules are configured, R1 and R2. Both with the same indices, but let's say they differ on users configured and only one is filtered (let's use R1 here), R2 is not filtered.
If you left the cache enabled, since the ES requests are the same, ES will request the cache instead. And you will get the answer from the cache. Meaning, that the rule without the filter (R2) will get the answer from the filtered request (R1). And we don't want that.

I know it's not optimal since we're lowering the performances, and I don't have another method to avoid this.
You could use a global parameter, allowing to enable or disable the filtering, and if it's disabled, use the cache. This way, only people who want to use the filters would be "impacted".

I hope it's more understable for you.

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented Apr 4, 2018

You could use a global parameter, allowing to enable or disable the filtering, and if it's disabled, use the cache. This way, only people who want to use the filters would be "impacted".

Yes I think this is very much necessary before we release. Thanks @rvibrac.

@seanziee

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

@sscarduzio As per @fbaligand 's question, is filter: "... owner:@{user} ..." allowed yet?

Just playing with this new version now, awesome work guys!

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented Apr 21, 2018

@seanziee it's not possible to use variables in the filter just yet.
Would be nice to hear your impressions about this feature, do you plan to test it in a Kibana scenario too?

@fbaligand

This comment has been minimized.

Copy link

commented Apr 21, 2018

I have a use case where data belongs to a specific user (with a user field). There is only one index for all users.
So I wish that all users access to a same kibana dashboard, but with only their own data displayed.

With filter and dynamic variable injection, it would be magic !

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented Apr 22, 2018

Yeah it's easy to do, it's coming. Watch the repo!

@fbaligand

This comment has been minimized.

Copy link

commented Apr 22, 2018

@sscarduzio
Great !

@seanziee

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

@sscarduzio

Great that you're working on this! My use case is that I have users that I'd like to filter the documents that they can see depending on their nginx sign in. So I have a field named username and each username has tons of activity attached to it and I only want that user to be able to see what his activity is on Kibana. By being able to use this feature, I could filter: "... Username:@{user} ..." so he can see only his data. Make sense?

@seanziee

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

I guess identical to @fbaligand use case 😅

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented May 4, 2018

This is already possible in the 20-pre3. Who needs a build? Say the ES version you need.

@seanziee

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

@fbaligand

This comment has been minimized.

Copy link

commented May 4, 2018

Great to see user injection is already available !

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented May 5, 2018

@fbaligand

This comment has been minimized.

Copy link

commented May 7, 2018

Hi @sscarduzio
I just tested this pre-release and it works great ! This is awesome !

Here's my test configuration :

    - name: "::USER::"
      proxy_auth: "*"
      indices: ["test-dls"]
      filter: '{"query_string":{"query":"user:@{user}"}}'

When I execute a search query with authenticated user "fbaligand", I get only data that owns to fbaligand !

Example curl query :
curl -H "x-forwarded-user: fbaligand" localhost:9200/_search?pretty

Little limit : it works only for read operations. If I try to index a document, I can index any document, even if user field is something else than "fbaligand".

@sscarduzio

This comment has been minimized.

Copy link
Owner

commented May 7, 2018

Thanks @fbaligand, thanks so much for the feedback, need to proof document level security is stable before going the next step: field level security.
And yeah I know, the "only for read" limitation is common also to x-pack and search guard, IIRC.
Apropos of this topic, do you think we should reject any non-read request if any "filter" is specified in the current block?

@fbaligand

This comment has been minimized.

Copy link

commented May 7, 2018

Well, it would be a powerful feature to support also "write" queries.
That said, I check on elasticsearch documentation, and you're right, ony read access is supported :
https://www.elastic.co/guide/en/x-pack/current/field-and-document-access-control.html#document-level-security
On searchguard, it is not clearly indicated, but they say : "Only documents matching this query will be visible". They don't speak about write queries.

So I think there is a good reason why both don't allow write access. Maybe too complex or too poor performance.

So given that X-Pack and SearchGuard - which have DLS feature since a long time - don't support write queries, I think it is "reasonable" to not implement it in readonlyrest.
Or maybe later in another release :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.