Remove the standard JSON enforcer and the JSON policy parsing in the Rego enforcer #2539
Conversation
micromaomao
force-pushed
the
tingmao_github/remove-json-policies
branch
from
c8be83c to
64da836
Compare
| // Return an user-friendly error message if we get a JSON policy. | ||
| // Previously such policy was supported, but we currently only | ||
| // support Rego. | ||
| return nil, fmt.Errorf("JSON policy is not supported.") |
There was a problem hiding this comment.
We don't need this. JSON policies only existed for dev purposes. We never shipped a product that supported them. There was a small overlap of time after the first release when the confcom toolling could produce them and they would have loaded.
| } | ||
| // createAllowAllEnforcer creates and returns OpenDoorSecurityPolicyEnforcer | ||
| // instance. The provided base64EncodedPolicy must be either empty or contain | ||
| // exactly the field "allow_all" set to true. |
There was a problem hiding this comment.
"exactly the field "allow_all" set to true" We don't want that anymore. They can use a long hand allow all policy.
There was a problem hiding this comment.
this and the above comment addressed
| // this is either a Rego policy or malformed JSON | ||
| code = string(rawPolicy) | ||
| } | ||
|
|
There was a problem hiding this comment.
Did you run the tests with this change? This change is needed and is used by tests and in enforcements. The handles are created based on osAwareMarshalRego
There was a problem hiding this comment.
Running go test -tags functional,rego ./pkg/securitypolicy/ on Linux, all tests pass. Not tried Windows yet but I think the security policy tests are mostly platform agnostic?
I also don't think any of the test should try and give the rego enforcer a JSON policy - if they need to turn some JSON into Rego, the test ought to call osAwareMarshalRego or MarshalPolicy themselves (which I think is already how it works?), then pass the resulting Rego to the Rego enforcer.
I also can't find any invocation of CreateSecurityPolicyEnforcer or createRegoEnforcer in the tests.
There was a problem hiding this comment.
but I think the security policy tests are mostly platform agnostic?
Just realized that I missed the fact that there is now regopolicy_linux/regopolicy_windows_test.go
(Although I think technically, unless the tests are generating actual CIM hashes, it should in theory be able to run on Linux too?)
Tried running it on Windows, all pass as well
micromaomao
force-pushed
the
tingmao_github/remove-json-policies
branch
from
64da836 to
165fb45
Compare
micromaomao
force-pushed
the
tingmao_github/remove-json-policies
branch
3 times, most recently
from
217484d to
cb41b99
Compare
| // UVM. At policy generation time, it's impossible to tell what the sandboxID | ||
| // will be, so the prefix substitution needs to happen during runtime. | ||
| // | ||
| //nolint:unused // linting is wrong, this is used in rego_utils_test.go |
There was a problem hiding this comment.
I'm not sure if this is the right fix though. I'd look into why lint is complaining now and not before this PR.
There was a problem hiding this comment.
So it looks like before this PR, this function was used in securitypolicyenforcer.go, but now it's only used in rego_utils_test.go. I wonder if the lint is because this is now a function only used by tests...?
| if slice1[i] != slice2[i] { | ||
| return false | ||
| } | ||
| // createAllowAllEnforcer creates and returns OpenDoorSecurityPolicyEnforcer |
There was a problem hiding this comment.
nit: match the name of the function.
There was a problem hiding this comment.
Oops not sure how that happened - I ended up removing the name altogether. Can you approve again and merge?
This commit removes the long deprecated standard JSON enforcer - all
confidential containers now has to either use Rego (the default), or the
open_door enforcer if provided with an empty policy or a policy that is
`{"allow_all": true}` (both case checked against host data).
Note that the host can still choose either rego or open_door, and this is not
measured into host data, but the policy check in createOpenDoorEnforcer ensures
that if the policy is a rego policy, trying to use an open_door enforcer will
error, leaving the enforcer at the default (which for confidential is a
deny-everything ClosedDoorSecurityPolicyEnforcer).
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This removes the ability to send in a standard JSON policy to the Rego enforcer. Previously it would translate it into the equivalent Rego policy (the format is different from the `containers := [...]` definition in Rego), but we need to stop supporting this. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
For the open door enforcer to be used, policy must be empty (i.e. the case in non-confidential containers). Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Suggested-by: Ken Gordon <kegordo@microsoft.com> Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
This function is now only used by tests, and having it in non _test files causses unused function lints. Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
micromaomao
force-pushed
the
tingmao_github/remove-json-policies
branch
from
1c223e4 to
922b7f8
Compare
|
@micromaomao , can you |
Signed-off-by: Tingmao Wang <tingmaowang@microsoft.com>
micromaomao
force-pushed
the
tingmao_github/remove-json-policies
branch
from
922b7f8 to
7f09812
Compare
Oops, forgot, just did now |
4 participants
This removes two kinds of JSON policies, both of which have never been supported for customer usage:
containersdeclaration, but in another custom format) to the Rego enforcer.Note that this in itself is not a security vulnerability against any deployments using normal Rego policy, as the host data in the attestation report generated form a container with such a policy would be the hash of a JSON structure, thus such a report would not e.g. pass existing key release policy enforcement.