Skip setting security policy when it's empty

[anmaxvl]

Due to an error ignored when calling to json.Unmarshal the call
to SetSecurityPolicy with an empty or invalid string policy results
in a policy with no Containers and AllowAll set to false. No
container can be run as a result.

Fix the behavior by not sending modification request for SetSecurityPolicy
when policy string is empty (which is the default) and checking the
error result from json.Unmarshal call

Signed-off-by: Maksim An maksiman@microsoft.com

[anmaxvl]

@SeanTAllen fyi

[katiewasnothere]

Is this enough? When I was testing other changes, I got errors from EnforceOverlayMountPolicy and EnforcePmemMountPolicy even when I commented out setting the security policy here.

[anmaxvl]

Is this enough? When I was testing other changes, I got errors from EnforceOverlayMountPolicy and EnforcePmemMountPolicy even when I commented out setting the security policy here.

yeah, this was enough for me.

[SeanTAllen]

Odd. I was able to run without any annotation and no errors without that being needed.

[SeanTAllen]

I've been thinking about how this could have been missed. There must have been a change during review that seemed inconsequential where I didn't end up retesting without the annotation later (definite oversight on my part). I suspect that on the guest side, it was filtering out setting the policy from the default before.

@anmaxvl I personally think the "" check should go inside the SetSecurityPolicy as it would be possible to call it incorrectly in the future and cause an issue. We could also had a check for "that security policy doesnt look right" on the guest side and error out if an empty one.

What's concerning to me is this code in uvm.go:

	// json unmarshall the decoded to a SecurityPolicy
	securityPolicy := &securitypolicy.SecurityPolicy{}
	json.Unmarshal(jsonPolicy, securityPolicy)

	p, err := securitypolicy.NewSecurityPolicyEnforcer(securityPolicy)
	if err != nil {
		return err
	}

So, the Unmarshall... at some point we lost an error check but either way, from this, without an annotation we somehow ended up with security policy that wasn't nil. That's concerning. It shouldn't turn an empty string into a security policy.

Before this change goes in @anmaxvl, we should fix the Unmarshal not checking the error return value and understand if it errors when no annotation was used.

[SeanTAllen]

@anmaxvl I tested the Unmarshall change. It would have caught this issue and given a more helpful error message about not being able to unmarshall the policy. I think it would be reasonable to add that change to this PR. If you think it should be separate, let me know and I'll open a PR for it.

[anmaxvl]

@anmaxvl I tested the Unmarshall change. It would have caught this issue and given a more helpful error message about not being able to unmarshall the policy. I think it would be reasonable to add that change to this PR. If you think it should be separate, let me know and I'll open a PR for it.

thanks for looking into it more. It makes sense now, we initialize an empty security policy, ignore the unmarshal error and end up in this state. I'll add the check.

[katiewasnothere]

lgtm