Skip to content

Reject policies that contain rules with more than one L3 match in a single rule#3015

Merged
tgraf merged 4 commits intocilium:masterfrom
joestringer:submit/policy-api-validation
Mar 9, 2018
Merged

Reject policies that contain rules with more than one L3 match in a single rule#3015
tgraf merged 4 commits intocilium:masterfrom
joestringer:submit/policy-api-validation

Conversation

@joestringer
Copy link
Copy Markdown
Member

@joestringer joestringer commented Mar 5, 2018 •
edited
Loading

This PR rejects policies which are not supported as per the assessment in #3004 .

The combination of fromCIDR and fromEndpoints was previously allowed, but conflicted with the standard rules for members in an IngressRule defined below, so this PR disallows it.

cilium/pkg/policy/api/rule.go

Lines 99 to 102 in ca6e1cb

// - If multiple members are set, all of them need to match in order for
// the rule to take effect. The exception to this rule is FromRequires field;
// the effects of any Requires field in any rule will apply to all other
// rules as well.

Note that there are certain combinations, like the above, which we have previously accepted policies for and treated as OR (eg, FromCIDR: x with FromEndpoints: y in the same rule). These policies must be split into separate rules, one for each L3 after this PR. Examples can be seen in the unit test changes in this PR.

@joestringer joestringer added pending-review release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Mar 5, 2018
@joestringer joestringer requested review from a team as code owners March 5, 2018 22:19
@joestringer joestringer requested a review from a team March 5, 2018 22:19
@joestringer joestringer changed the title Reject policy import where more than one L3 match exists in a single rule Reject policies that contain rules with more than one L3 match in a single rule Mar 5, 2018
manalibhutiyani
manalibhutiyani reviewed Mar 5, 2018
View reviewed changes
Comment thread pkg/policy/api/rule_validation.go Outdated
Copy link
Copy Markdown

@manalibhutiyani manalibhutiyani Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the !l3DependentL4Support[member] check in egress, since all values are false?

Copy link
Copy Markdown
Member Author

@joestringer joestringer Mar 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly, no, but egress label-dependent l4 is pretty close so it seems easy enough to just include this here. It'll require a minor rebase and update for the egress work.

@joestringer joestringer force-pushed the submit/policy-api-validation branch from f64dd16 to a3b27d8 Compare March 5, 2018 23:23
@joestringer joestringer requested a review from a team as a code owner March 5, 2018 23:23
@joestringer joestringer requested a review from a team March 5, 2018 23:23
@joestringer joestringer mentioned this pull request Mar 5, 2018
Closed
rlenglet
rlenglet approved these changes Mar 5, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@rlenglet rlenglet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this. Thanks!

@joestringer joestringer mentioned this pull request Mar 5, 2018
Merged
@joestringer joestringer force-pushed the submit/policy-api-validation branch 2 times, most recently from c7d48fa to b22db04 Compare March 6, 2018 01:21
@joestringer
Copy link
Copy Markdown
Member Author

joestringer commented Mar 6, 2018

test-me-please

nebril
nebril previously requested changes Mar 8, 2018
View reviewed changes
Comment thread pkg/policy/api/rule_validation.go Outdated
Copy link
Copy Markdown
Member

@nebril nebril Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will cause ToServices to break, as ToServices rule is being translated into ToCIDRSet rule. This code needs a bit more logic that handles that case.

Copy link
Copy Markdown
Member Author

@joestringer joestringer Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Rather than opening a hole for configuration of things that violate the general rule semantics, I've attempted to reshuffle the sanitization logic so that internally we can represent things in whichever way makes sense, but externally we should only advertise support for particular combinations. This approach is represented by the new patch "daemon: Shift policy sanitize up to handle".

@joestringer joestringer force-pushed the submit/policy-api-validation branch from b22db04 to 740a97e Compare March 8, 2018 18:05
@joestringer
Copy link
Copy Markdown
Member Author

joestringer commented Mar 8, 2018 •
edited
Loading

It looks like the reason it fails after that translation is that we have yet another sanitize() as part of adding policy into the repository, which re-triggers the logic added in this patch. I'm planning to look at whether that sanitize can be moved further up the chain for the non-k8s API case so that all rules should be sanitized by the time that they are inserted into the repository. I expect that this should address the issue.

@joestringer joestringer force-pushed the submit/policy-api-validation branch from 740a97e to d4370ad Compare March 8, 2018 19:01
@joestringer joestringer requested a review from a team March 8, 2018 19:01
@rlenglet
Copy link
Copy Markdown
Contributor

rlenglet commented Mar 8, 2018

Nice!

@joestringer joestringer force-pushed the submit/policy-api-validation branch from d4370ad to 7512be8 Compare March 8, 2018 21:55
@joestringer joestringer mentioned this pull request Mar 8, 2018
Merged
@joestringer
policy: Reuse repo.AddList() from repo.Add()
69f00d7 
Share more code between these functions.

Signed-off-by: Joe Stringer <joe@covalent.io>
@joestringer
daemon: Shift policy sanitize up to handle
60d7803 
Rules should be sanitized relatively early, and in the k8s case they
already were sanitized before reaching the repository. Do likewise for
the regular API case so that k8s doesn't double-sanitize the rules.

This avoids issues when performing translation of k8s rules internally
after we've already sanitized and validated rule combinations, where the
translated rules no longer satisfy the same constraints as what is
rejected at the API - for example, translating headless services policy
into CIDR policies.

Signed-off-by: Joe Stringer <joe@covalent.io>
@joestringer joestringer force-pushed the submit/policy-api-validation branch from 7512be8 to 0b2a7ee Compare March 9, 2018 00:08
@joestringer
policy: Reject all unsupported rule combinations
5125193 
Signed-off-by: Joe Stringer <joe@covalent.io>
@joestringer
test: Fix formatting in headless policy
7cd0d29 
Signed-off-by: Joe Stringer <joe@covalent.io>
@joestringer joestringer force-pushed the submit/policy-api-validation branch from 0b2a7ee to 7cd0d29 Compare March 9, 2018 00:09
@joestringer
Copy link
Copy Markdown
Member Author

joestringer commented Mar 9, 2018

test-me-please

@joestringer joestringer mentioned this pull request Mar 9, 2018
Closed
4 tasks
@joestringer joestringer dismissed nebril’s stale review March 9, 2018 02:57

Addressed the issue, please re-review

tgraf
tgraf approved these changes Mar 9, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@tgraf tgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% happy with the validation only happening via the API but this is a great step forward so I'm fine with this for now.

@tgraf tgraf merged commit f5c5a5a into cilium:master Mar 9, 2018
@joestringer joestringer deleted the submit/policy-api-validation branch March 9, 2018 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nebril nebril nebril left review comments

+4 more reviewers

@rlenglet rlenglet rlenglet approved these changes

@tgraf tgraf tgraf approved these changes

@ianvernon ianvernon ianvernon approved these changes

@manalibhutiyani manalibhutiyani manalibhutiyani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joestringer @rlenglet @nebril @tgraf @ianvernon @manalibhutiyani