What's the design around rule ordering


(Diego Nieto Cid) #1

Hello

Today I was debugging ROR plugin and noticed that the rules in one of the blocks I’ve configured were not executed in the order I expected. Then I realised there’s is no user defined order as a block is just a YAML map and well maps are usually not ordered :stuck_out_tongue:

So, I was curious about what was the design decision here. Most of the rules don’t actually care as they work independently but indices_rewrite and indices could have ordering implications.

Like filtering based on user provided index name or the rewritten one or even both if rules were allowed to be repeated.

Cheers


BTW, to give some context, I’m using ROR to secure a plain ElasticSearch cluster where the user can only access its own index where I’m playing with indices_rewrite and indices to make it happen.

Details below if you are interested.

[details=Summary]All the configuration is currently the block below:

    - name: "Restrict access to user's index only"
      type: allow
      proxy_auth: "*"
      indices_rewrite: ["^(client-index).*$", "[email protected]"]
      indices: ["client-index*"]

You can figure my expectations were, check auth then rewrite finally check rewritten indices.

Anyway, after making reads work, I had some trouble with update operations, but in version 1.14.1-pre4 that config seems to be working ok.

I don’t really have much time available so I had to use what’s already in the plugin. But a simpler way of implementing the rule above would be if indices supported the @user placeholder; except for the user visible index name differences.

The only change I had to apply is switching a HashSet to a LinkedHashSet to make the order of iteration predictable (currently the order of addition in Block::initSyncConditions).

diff --git a/src/main/java/org/elasticsearch/plugin/readonlyrest/acl/blocks/Block.java b/src/main/java/org/elasticsearch/plugin/readonlyrest/acl/blocks/Block.java
index 55041ec..4523aff 100644
--- a/src/main/java/org/elasticsearch/plugin/readonlyrest/acl/blocks/Block.java
+++ b/src/main/java/org/elasticsearch/plugin/readonlyrest/acl/blocks/Block.java
@@ -70,7 +70,7 @@ public class Block {
   private final Logger logger;
   private final ConfigurationHelper conf;
   private final Client client;
-  private final Set<SyncRule> syncConditionsToCheck = Sets.newHashSet();
+  private final Set<SyncRule> syncConditionsToCheck = Sets.newLinkedHashSet();
   private final Set<AsyncRule> asyncConditionsToCheck = Sets.newHashSet();
   private boolean authHeaderAccepted = false;
   public Block(Settings s, List<User> userList, List<LdapConfig> ldapList, Logger logger,
@@ -246,6 +246,10 @@ public class Block {
     } catch (RuleNotConfiguredException ignored) {
     }
     try {
+        syncConditionsToCheck.add(new IndicesRewriteSyncRule(s));
+    } catch (RuleNotConfiguredException ignored) {
+    }
+    try {
       syncConditionsToCheck.add(new IndicesSyncRule(s));
     } catch (RuleNotConfiguredException ignored) {
     }
@@ -258,10 +262,6 @@ public class Block {
     } catch (RuleNotConfiguredException ignored) {
     }
     try {
-      syncConditionsToCheck.add(new IndicesRewriteSyncRule(s));
-    } catch (RuleNotConfiguredException ignored) {
-    }
-    try {
       syncConditionsToCheck.add(new KibanaHideAppsSyncRule(s));
     } catch (RuleNotConfiguredException ignored) {
     }

[/details]


[DONE] External auth feature
(Simone Scarduzio) #2

Hello @diegonc, what you said makes a ton of sense to me too.

Now that rules have side effects on the request context (i.e. messing with indices and HTTP headers), not only blocks execution order matters, but also the order of rules execution within a block.

Curiously enough, I’d imagined that people would like first to intercept the intent of the requester using indices and then “route” the request to a custom-pre/postfixed index name using indices_rewrite.
I guess there’s no right or wrong here, but this strengthens your point about rules ordering does matter.

To your proposed change, first of all: yes please, go play with the code and throw PR at me because it feels like christmas :tada::gift:.

To the specific proposed change: it’s cool, but it satisfies your precise use case. Needs to be more generic and reflect the actual rules ordering found in the block.

How about we refactor Block constructor as follows:

  • iterate orderly through this block’s Settings keys
  • Spot the rule by key name
  • Instantiate that rule
  • Add it to syncConditionsToCheck.

BTW, good call on changing it to LinkedHashSet to make the order matter!

Could you please, please, pretty please start a PR on this? :angel:

PS: @diegonc thanks for taking the time to describe the use case at hand, I encourage everyone else to do so, it’s a very important indicator for future development.
BTW yours is a great cookbook example, as it shows off of how TINY configuration ReadonlyREST needs to do insane stuff like on-the-fly index rewrite based on credentials. :rocket::clap:


(Diego Nieto Cid) #3

Oh, that’s an interesting alternative. I’ll make sure to try it :slight_smile:

Awesome.

Sounds good. Looking at the Settings class, it seems it’s backed by a map.
Is it ok to also change the configuration syntax to use an array?

 - name: "Block name"
   type: allow
   acl:
    - key1: <conf>
    - key2: <conf>
    - key3: <conf>

(Simone Scarduzio) #4

I’d love to keep the backwards compatibility of the configuration, the Settings class is backed by a LinkedHashMap so it’s possible to preserve the ordering.

See the way it’s done here:

https://github.com/sscarduzio/elasticsearch-readonlyrest-plugin/blob/master/src/main/java/org/elasticsearch/plugin/readonlyrest/acl/ACL.java#L61


(Diego Nieto Cid) #5

I think it’s not possible to remain backward compatible and get the rules in the same order as in the configuration file.

The settings builder collects all the flattened values in a TreeMap which gives alphabetically ordered keys.

The reason you can get the blocks in order is because access_control_rules is a YAML array which gets flattened to keys using the item index and you are not relying in the map key order (which would yield [0, 1, 10, 11 … 2 …] instead of the expected order).

For instance, the snippet of my configuration related to ROR gets translated to a map with the following keys:

  readonlyrest.access_control_rules.0.name
  readonlyrest.access_control_rules.0.type
  readonlyrest.access_control_rules.0.x_forwarded_for.0
  readonlyrest.access_control_rules.0.x_forwarded_for.1
  readonlyrest.access_control_rules.0.x_forwarded_for.2
  readonlyrest.access_control_rules.1.indices.0
  readonlyrest.access_control_rules.1.indices_rewrite.0
  readonlyrest.access_control_rules.1.indices_rewrite.1
  readonlyrest.access_control_rules.1.name
  readonlyrest.access_control_rules.1.proxy_auth
  readonlyrest.access_control_rules.1.type
  readonlyrest.enable
  readonlyrest.response_if_req_forbidden

And when I get the sub-settings passed to Block constructor using getGroups the key remain in that order.

  indices.0
  indices_rewrite.0
  indices_rewrite.1
  name
  proxy_auth
  type

I used the following test-case to get the keys:

Summary
public void testCheckSettingsRetainTextualOrder() throws IOException {
	String rorSettings = ""
		+ "\nreadonlyrest:"
		+ "\n  enable: true"
		+ "\n  response_if_req_forbidden: Sorry, your request is forbidden"
		+ "\n  access_control_rules:"
		+ "\n    - name: \"Local access is not restricted\""
		+ "\n      type: allow"
		+ "\n      x_forwarded_for: [\"127.0.0.0/8\", \"192.168.123.0/24\", \"192.168.56.101\"]"
		+ "\n"
		+ "\n    - name: \"Restrict access to user's index only\""
		+ "\n      type: allow"
		+ "\n      proxy_auth: \"*\""
		+ "\n      indices_rewrite: [\"^(client-index).*$\", \"[email protected]\"]"
		+ "\n      indices: [\"client-index*\"]";
	InputStream is = new ByteArrayInputStream(rorSettings.getBytes());
	Settings settings = Settings.builder().loadFromStream("a.yml", is).build();

	System.out.println("Settings key order:");
	for (Map.Entry<String, String> entry: settings.getAsMap().entrySet()) {
		System.out.println("  " + entry.getKey());
	}
		
	Settings rule1 = settings.getGroups("readonlyrest.access_control_rules").get("1");
	System.out.println("Rule key order:");
	for (Map.Entry<String, String> entry : rule1.getAsMap().entrySet()) {
		System.out.println("  " + entry.getKey());
	}
}

(Simone Scarduzio) #6

Hey @diegonc! Sorry for the delayed response.

Yep, I see there’s a challenge here.
I’ve been thinking about this. How about we just hard code the rule evaluation as follows:

  1. All authentications rules (populate the @user variable)
  2. All rules that don’t mutate the request (can use the @user variable)
  3. All rules that mutate the request (rewrite_indices)

This means that we could introduce the @user substitution in the indices rule, so - to answer your original query - you could have:

 - name: "Restrict access to user's index only"
      type: allow
      proxy_auth: "*"
      indices_rewrite: ["^(client-index).*$", "[email protected]"]
      indices: ["client-index*@user"]

#7

Why not simply add an optional numeric order field to rules ?


(Simone Scarduzio) #8

By optional numeric order field you mean declaring rules in an array?
Because it’s more cognitive load on the user, and it forks the configuration model. And it means more documentation too: more cognitive load for users (2).


(Diego Nieto Cid) #9

I submitted a pull request for what you described. Looks like that’s the best we can do for now.

This is only implemented for synchronous rules.
Async is another issue. Currently, all sync rules are executed first and then async ones. But it could make sense to intersperse them.


(Mateusz Kołodziejczyk) #10

Yes, definitely, async & sync rules order should not be determined separately. Eg. my case: sync proxy authentication and async role based authorization (this is ok now, because sync rules are executed before async ones). Simplest solution is adapter with async interface for sync rules. Then we could treat all rules as they were all asynchronous.


(Simone Scarduzio) #11

The concern for async rules being evaluated not in order is valid, I agree it’s a problem. The adapter sounds good, as long as it’s the kind of adapter that does not force all current sync Rule implementors to return a Future.

Moving all the currently super-simple sync code to an async interface would mean we are downgrading the whole code base to the lowest common usability denominator. Plus rewriting tests would be a pain in the ass.

How about in Block we wrap the sync rules in some AsyncAdapter and then sequence all the resulting real and fake futures? Does this match your proposal @coutoPL ?


(Mateusz Kołodziejczyk) #12

We are talking about the same solution. AsyncAdapter will implement async rule interface and will delegate matching to underlying sync rule and wrap its result into completable future. From block perspective all rules will be async. It is clean, simple and require no tests or rules refactor.