rbac: support mixing allow/deny policies with precedence

[yangminzhu]

Title: support mixing allow/deny policies with precedence

Description:
The current RBAC filter API has the following limitations:

• It binds a single action to a set of policies, in other words, we cannot mix the allow/deny policies in the same config
• It doesn't support policy precedence as all policies are specified in a map

Feature Request:

• Support action per-policy in the filter config
• Support policy with precedence

Use Cases:

• Some system (e.g. Istio) may have complicated layered access control design that uses both deny and allow policy at the same time with different priority.

Strawman design:

• Option 1: Incrementally change the current RBAC filter API

  • add the Action to the Policy message
  • add a new repeated Policy list_policies in the RBAC message and mark the current policies as deprecated

• Option 2: Introduce a new top-level config to simplify the RBAC filter API:

message Authorization {
  // The filter config is just a list of rules,
  // the first matched rule decide the action: ALLOW or DENY
  repeated Rule rules = 2;
}

message Rule {
  // Optional. A human readable string explaining the purpose of this rule.
  string description = 1;

  // A single match decide when to trigger the action
  Match match = 2;

  // action could be either ALLOW or DENY
  Action action = 3; 
}

message Match {
  // Optional. A human readable string explaining the purpose of this match.
  string description = 1;

  oneof type {
    MatchSet and = 2;
    MatchSet or = 3;
    Match not = 4;

    bool always = 5;
    HeaderMatcher header = 6;
    CidrRange source_ip = 7;
    CidrRange destination_ip = 8;
    uint32 destination_port = 9;
    StringMatcher requested_server_name = 10;
    Authenticated authenticated = 11;
    MetadataMatcher metadata = 12;
    string cel = 13;
  }
}

message MatchSet {
  repeated Match matches = 1;
}

Summary

• Option 1 tries to do everything incrementally, the change is less but I'm worried it's going to make the RBAC filter API more complicated in the long turn. Option 1 is fully backward-compatible and requires minimal changes in user code if they want to use the new features.
• Option 2 tries to do some some optimizations (most notably it has a much simpler and cleaner match/action pair), in the long term, we could rename the RBAC filter to just authz filter. Option 2 requires slight more but still reasonable changes in user code if they want to use the new feature, Envoy could also auto-translate the config if needed.

@mattklein123 @htuch @lizan would you mind to take a look and let me know what do you think about this? Thank you.

[htuch]

@yangminzhu I assume this will land next year in the v3 Envoy APIs. In that case, you will need to support both for 2020 in parallel, with both deprecated version alongside the new one for option 2. In v4 (2021) you will be able to drop the old one completely. I'm generally in favor of clean APIs, so I would recommend doing (2).

However, I think (2) should be aligned with the general move we want to make towards unifying matchers in v3. @mattklein123 has the most context on this, but basically we want to align RBAC, TAP, access log, routing and other matchers that all look very similar today.

[mattklein123]

However, I think (2) should be aligned with the general move we want to make towards unifying matchers in v3. @mattklein123 has the most context on this, but basically we want to align RBAC, TAP, access log, routing and other matchers that all look very similar today.

+1. I have some thoughts on this so let's chat when you are ready to start implementing. This keeps coming up so we need to have a central matching system, potentially provided by the HCM itself (as a module).

[yangminzhu]

@htuch Thanks for the information, is the general move tracked in #5569?

The proposal in the issue is to add the matcher to be part of the HttpConnectionManager and refer to it in filters using MatcherReference.

I have a few concerns of this approach:

1. This will create a dependency between the filter config and the HCM config. Currently the code responsible for the filter config could be completely separated from the HCM config code in control plane, and this allows the control plane to simplify the pipeline a lot. Using MatcherReference will couple the code for the core HCM and the optional filter config code path, and sometimes very hard to do in some environments.

2. Different filters have very different use cases of the matcher logic, for example, the RBAC filter may have some complicated matchers that are only meaningful in the access control context, it's not clear how much we can share from the central matcher in HCM

3. Similar to the above point, different filter may have its own unique matching conditions, for example, the RBAC filter support to use CEL expression, this means the matcher in HCM has to include everything needed in other filters.

4. It will make the debugging harder as now the matching configuration is separated in different places.

I will probably have the time to start the implementation next week or the week after next week, @mattklein123, can we proceed the changes in RBAC without blocking on the central matcher proposal?

Note, the first point of the above concerns is the most challenging one, is it possible to introduce the central matching system as a library instead of putting it in the HCM?

[mattklein123]

Sorry for the multi-month late response here, but I agree with some of your concerns around centralized though I think we should try to work through them, or at minimum figure out how to share a lot more code than we do now. I have some ideas though it might be better to chat in a meeting. Let me know if you want to set something up.

[yangminzhu]

@mattklein123 Thank you for the reply, what do you think about having the chat in the Envoy community meeting? If it's too late for tomorrow's meeting (02/25), we can do it in the next one (03/10). If you prefer a separate chat, I can set it up and invite other stakeholders (cc @mergeconflict ) to the meeting.

[mattklein123]

Sure happy to discuss on the call today or some other time as needed.

[yangminzhu]

Sorry I can't make it today, will attend the meeting next time. Thank you.

…

On Tue, Feb 25, 2020 at 08:05 Matt Klein ***@***.***> wrote: Sure happy to discuss on the call today or some other time as needed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9376?email_source=notifications&email_token=AAQWO2EPHCT3J44GPDOIBL3REU6ULA5CNFSM4J3VD3OKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEM4RKCY#issuecomment-590943499>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQWO2BRP6YVKXYRRRWH5P3REU6ULANCNFSM4J3VD3OA> .

[markdroth]

@jiangtaoli2016 may want to review this proposal as well.

[jiangtaoli2016]

We need to be careful when ALLOW rule mixed with DENY rule in the same policy. If a policy has multiple rules and there is an explicit match in ALLOW rule first and an explicit match in DENY rule later, shall we allow or deny? If deny, we need to evaluate all the rules and make decision carefully.

In the current RBAC, each RBAC has only one action, it is semantically clear and unambiguous.

[yangminzhu]

We need to be careful when ALLOW rule mixed with DENY rule in the same policy. If a policy has multiple rules and there is an explicit match in ALLOW rule first and an explicit match in DENY rule later, shall we allow or deny? If deny, we need to evaluate all the rules and make decision carefully.

In the current RBAC, each RBAC has only one action, it is semantically clear and unambiguous.

@jiangtaoli2016

a policy will have only one action, the current RBAC filter puts the action in the filter level instead of policy level meaning all policies will have the same action, the order of policies doesn't matter.

By moving the action to policy level, we change the evaluation to use the action of the first matched policy, the order of policies now matter if you have both allow and deny actions.

For example, assume you have the config of [policy A - ALLOW, policy B - DENY], if a request is matched with policy A, then it's allowed immediately (it could still match to policy B but the order matters here).

This essentially make the RBAC filter more consistent with normal firewall evaluation process where each firewall rule is assigned with a priority and the first matched firewall rule is used.

The new semantic is still simple and straightforward enough (action of first-matched policy), and can easily support any existing users (they can just assign the same action to all its policies and insert a default ALLOW or DENY to the end of the list).

[jiangtaoli2016]

Having one action per rule is fine. However, making authorization decision based on the first matched rule is error-prone and is bad security practice. It conflicts with internal authorization and will cause more problems in the future. I recommend the following:

• If any DENY rule matches, permission is denied.
• Otherwise, if any ALLOW rule matches, permission is granted.
• Otherwise, if no rule applies, permission is denied.

[gitcos]

What is the status of this? It's still marked open, but I see no updates for a few years.

We very much need the ability to express, in a network RBAC filter:

1. Always allow these networks - list A
2. If the request was not from a network on list A, then deny if it's from one of these networks - list B
3. If it didn't match 1 or 2, then allow

In other words, we need a default-allow configuration where we can block any requests "list B" but create a "list A" of exceptions that MUST NOT be blocked. From reading Envoy's documentation, there seems to be no way to do that, which is pretty terrible - it means we need to write code to do a bunch of logic to create a combined set of lists A and B to feed to Envoy, and that's really error prone and hard to maintain.

BTW, if the suggestion from jiangtaoli2016 above were taken, then it would still be impossible to do what I'm describing even if this feature is added, so I oppose that.

Anyway, what's the status, anyone know?

[github-actions]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

[gitcos]

This is a real issue, it's just being ignored. That is not a reason to close it.

[gitcos]

@alyssawilk Why did you enable the stalebot on this issue when it had been disabled?

[alyssawilk]

It's a feature request from 2019 - if no one has picked it up by now I suspect no one is likely to regardless of if it's open or not?