Policy auth precedence fix v1.14 #26813
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[ upstream commit 9f52abb ] Export DenyPreferredInsertWithChanges and make it revertible by taking a map of old values as a new optional argument. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 8aa89ef ] Use DenyPreferredInsert instead of directly manipulating policy map state to make sure deny entries are not overridden by new proxy redirect entries. Prior to this fix it was possible for a proxy redirect to be pushed onto the policy map when it should have been overridden by a deny at least in these cases: - L3-only deny with L3/L4 redirect: No redirect should be added as the L3 is denied - L3-only deny with L4-only redirect: L4-only redirect should be added and an L3/L4 deny should also be added, but the L3/L4 deny is only added by deny preferred insert, and is missed when the map is manipulated directly. A new test case verifies this. It is clear that in the latter case the addition of the redirect can not be completely blocked, so we can't fix this by making AllowsL4 more restrictive. But also in the former case it is possible that the deny rule only covers a subset of security identities, while the redirect rule covers some of the same security identities, but also some more that should not be blocked. Hence the correct fix here is to leave AllowsL4 to be L3-independent, and cover these cases with deny preferred insert instead of adding redirect entries to the map directly. This commit also contains a related change that allows a redirect entry to be updated, maybe with a changed proxy port. I've not seen evidence that this is currently fixing a bug, but it feels like a real possibility. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 2d8fbce ] Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 569074d ] Users of ToMapState() combine the results from a single-filter ToMapState call to a policy-wide MapState using DenyPreferredInsert, which causes the heavy logic needed for giving preference to deny entries to be computed at least twice per entry, once within ToMapState, and then again in the caller. Since DenyPreferredInsert may add new entries, the 2nd round may actually involve more entries than the first. Change the ToMapState to match its current use by: - take the final map where entries are to be added as a parameter to avoid the need for an intermediate return MapState - take in a filter function so that the caller may update an entry without iterating over the mapstate again (e.g., replace temporary proxy port with the actual proxy port), and the caller has the option to skip adding the entry by returning 'false'. - take Keys arguments for 'adds' and 'deletes' that are passed to DenyPreferredInsertWithChanges(). This change reduces the computational cost of policy updates, and makes it easier to reason about the MapState processed with DenyPreferredInsertWithChanges, now that it is done in a single pass, rather than multiple passes like before. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 4ee2124 ] Add L4DirectionPolicy containting the L4PolicyMap, so that additional fields can be added in later commits. This should be a pure refactor without functional changes. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 1b577eb ] Add EndpointPolicy.UpdateRedirects() to keep more of the policy internals within the policy package. Rename GetLabels to GetLabelsLocked and note in the implementations that the lock of the implementing object is held when it is called. Make ToMapState non-exported, and simplify callers now that they are all in the policy package. Rename computeDesiredL4PolicyMapEntries and computeDirectionL4PolicyMapEntries to 'toMapState' for their respective receivers, as they do the same function and eventually call the L4Filter.toMapState. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 33bc745 ] Track policy features in use across all PortRules in a given traffic direction, and skip policy computations not needed when specific features are not in use. This is needed as processing deny rules involves scanning all MapState entries for each new entry (O(N^2)), which can be skipped when there are no deny rules in the policy. This will also be used for Authentication policy processing for the same reason, so that if Authentication is not used, there is no additional CPU overhead. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 0d188c2 ] Export MergeSorted and make it more efficient by adding entries to the receiver as needed. Use MergeSorted in policy where it is known that the LabelArrayLists are in sorted order already. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 807919b ] Keep track of all contributing rule's labels in merged map state entries. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit c018ac8 ] Restructure addKeyWithChanges to make it easier to understand: - if an old entry exists, the new one is merged to it, honoring the precedence rules for deny, proxy redirection, and auth type. - new entries get their own containers for owners, dependents, and DerivedFromRules so that each entry in MapState has separate containers. This was already the case previously, but the code was had to reason about. - when storing old values for reverting, clone containers so any futher changes in the values in the map will be change the stored old values. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 8be4aef ] The receiver of an autogenerated DeepEqual function can not be nil, must check for it explicitly. Generated api.Authentication.DeepEqual returns false whenever 'other' is nil, even if the receiver is also nil. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 59e7981 ] Make GetAuthType return both if the auth type is explicitly present, and the auth type itself. This becomes necessary when considering if the auth type of a more generic entiry should override the one in a more specific entry in future. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 8e3cf49 ] When considering if an l3l4 rule can be skipped due to a l4-only (wildcard L3) rule on the same filter we must also consider the auth types of both rules. More specifically, the l3l4 rule must be poshed to the datapath if it has an authentication policy while the l4-only rule does not, or if both have different auth policies. This opens up a possiblity that due to this auth type discrepency the redirect status of the l4-only rule must be explicitly applied to the l3l4 rule as the l3l4 rule can no longer be skipped in all cases. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
… ones [ upstream commit b201821 ] Apply authentication policy from more general rules to more specific rules, unless explicitly specified. Restructure addKeyWithChanges to make it easier to understand: - if an old entry exists, the new one is merged to it, honoring the precedence rules for deny, proxy redirection, and auth type. - new entries get their own containers for owners, dependents, and DerivedFromRules so that each entry in MapState has separate containers. This was already the case previously, but the code was had to reason about. redirectPreferredInsertWithChanges() is no longer needed as a separate function, as addKeyWithChanges now follows the same logic, but instead of simply overriding the old entry with the new one considers the precedence rules for deny, proxy redirect and auth type. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 7877f35 ] There is no reason selectorPolicy.L4Policy needs to be a pointer as we can initialize an empty L4Policy for it. Refactor by inlining L4Policy instead of pointing to it, as this simplifies code by removing the need for pointer nil checks. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit cb50a88 ] Use addKeysWithChanges in AddVisibilityKeys instead of directly manipulating the maps to keep track of changes in a uniform fashion. Also record changes when adding dependents to existing keys. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 82203d8 ] Simplify code by grouping change tracking arguments to a single "changes" argument. Change location of "identities" argument in mapstate functions to be more consistent with 'toMapState'. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 78b91bc ] Merging of deeply equal map state entries is not necessary, and it is clearer to skip the merge in this case. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 4547c0a ] Comment corrections for readability and camel-casing a local variable. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
jrajahalme
added
kind/backports
1 task
nathanjsweet
approved these changes
Jul 14, 2023
|
/test-backport-1.14 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Once this PR is merged, you can update the PR labels via:
or with