pathpol: ensure deserialized ACL has default rule

[fbuetler]

fixes #4504

[matzf]

This change is 

[matzf]

Thank you! I only have a few nitpicks below.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @fbuetler)

---

private/path/pathpol/acl.go line 68 at r1 (raw file):

		return ErrNoDefault
	}
	return err

This will shadow any serialization error; if the json is malformed, it will result in an empty list and we say ErrNoDefault.
Check for the serialization error first. (Same for the yaml below).

Suggestion:

	if err := json.Unmarshal(b, &a.Entries); err != nil {
		return err
    }
	if len(a.Entries) == 0 || !a.Entries[len(a.Entries)-1].Rule.matchesAll() {
		return ErrNoDefault
	}
	return nil

---

private/path/pathpol/acl.go line 68 at r1 (raw file):

		return ErrNoDefault
	}
	return err

It could make sense to add a shared function func validateACL(entries []*ACLEntry) error which does the check for the default entry. Then the above code snippet becomes:

    if err := json.Unmarshal(b, &a.Entries); err != nil {
		return err
    }
    return validateACL(a.Entries)

The validate function can perform exactly the check that we currently apply -- or it can be a good place to make this a bit more helpful; enforcing the last entry to be a default match seems unnecessarily restrictive. The evaluation just needs any entry that is a default match. Sure, the subsequent entries are "dead code"; if we want to forbid this (to be "helpful"), we could have a separate error for this so the user can more easily understand the problem.

---

private/path/pathpol/acl_test.go line 70 at r1 (raw file):

		"No entry": {
			Input:       []byte{},
			ExpectedErr: ErrNoDefault,

This should be a json serialization error (see my comment on the "shadowing" of the error).
Perhaps you meant to check [] instead?

---

private/path/pathpol/acl_test.go line 73 at r1 (raw file):

		},
		"No default entry": {
			Input:       []byte(`"+ 42"`),

You probably meant ["+ 42"]

Perhaps change the type of Input to string and just apply the cast to []byte when invoking the unmarshal function below; then this becomes slightly more readable here.

---

private/path/pathpol/acl_test.go line 89 at r1 (raw file):

			assert.ErrorIs(t, err, test.ExpectedErr)
			if test.ExpectedErr == nil {
				assert.NotNil(t, acl)

This does not check anything, acl cannot be nil (it's a struct, not a pointer to a struct).
Maybe assert.NotEmpty(t, acl.Entries) is a good check?

---

private/path/pathpol/acl_test.go line 92 at r1 (raw file):

			}
		})
	}

We can use the exact same test inputs for also checking the YAML unmarshal (all the inputs are valid YAML too).

[fbuetler]

Thanks a lot for taking the time to review this PR and very good catches!

I applied most of your suggestions. However, I don't quite understand how the UnmarshalYAML function works. Where is the input coming from? Is it hidden within the supplied function? Is there a reason to not use yaml.Unmarshal analogously to UnmarshalJSON?

[oncilla]

Yes. The yaml unmarshler interface is a bit of a mind bender.

The provided function should be used to unmarshal into a variable.
See https://github.com/search?q=repo%3Ascionproto%2Fscion%20unmarshalyaml&type=code for some examples

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @fbuetler and @matzf)

[matzf]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @fbuetler)

---

private/path/pathpol/acl.go line 32 at r3 (raw file):

	// ErrObsoleteEntries indicates that there is a default acl entry posisioned in
	// the middle, making the following acl entries obsolete.
	ErrObsoleteEntries = errors.New("ACL has a default entry posisioned in the middle, making the following entries obsolete")

Nit. Typo "posisioned" -> "positioned".
Maybe "obsolete" is not the ideal term here (it means outmoded, out of date), I'd suggest "unused" or "extra": ErrExtraEntries = .. "ACL has unused extra entries after a default entry".

---

private/path/pathpol/acl_test.go line 94 at r3 (raw file):

		t.Run(name, func(t *testing.T) {
			var acl ACL
			err := acl.UnmarshalJSON([]byte(test.Input))

Note, perhaps this can help with your question regarding using UnmarshalJSON/YAML.

Here, calling acl.UnmarshalJSON(buffer) works and is perfectly fine, but the more "generic" invocation would be json.Unmarshal(buffer, &acl). This variant works for all "unmarshallable" types, not only those types that directly implement a custom UnmarshalJSON -- for example, json.Unmarshal() would also let you unmarshal strings, numbers, structs with default unmarshaller, or a list of ACL by var v []ACL; json.Unmarshal(buffer, &v).

The yaml interface is similar, use yaml.Unmarshal(buffer, &v). Here, the customized UnmarshalYAML cannot be invoked directly so easily as with the JSON counterpart.

[fbuetler]

So the idea is to do something like this?

			var acl ACL
			err := acl.UnmarshalYAML(func(entries interface{}) error {
				return yaml.Unmarshal([]byte(test.Input), entries)
			})
			assert.ErrorIs(t, err, test.ExpectedErr)

[matzf]

No, just err := yaml.Unmarshal([]byte(test.Input), &acl).

(see also the note in my previous comment in acl_test.go a few moments ago)

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @fbuetler)

[matzf]

Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @fbuetler)

a discussion (no related file):
There is a failing test now, pathpol/policy_test.go TestPathJsonConversion.
This fails because the policy contains a default deny entry after a test entry that is "accidentally" a default entry (predicate 0-0#0 is a match all). Please change the test there; perhaps it would make sense to create the ACL with NewACL to ensure that the test does not create a bogus object in the first place.

---

[fbuetler]

Ah, this works because of the Unmarshaler interface. This was the missing puzzle piece for me.

[matzf]

Reviewed 1 of 2 files at r5, 2 of 2 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @fbuetler)

---

private/path/pathpol/policy_test.go line 592 at r8 (raw file):

		{Action: Allow, Rule: mustHopPredicate(t, "42-0#0")},
		denyEntry,
	}...)

Nit: no real problem it works, but it's more clumsy than necessary. The slice and then unpacking with ellipsis can be left away:

Suggestion:

	acl, err := NewACL(
		&ACLEntry{Action: Allow, Rule: mustHopPredicate(t, "42-0#0")},
		denyEntry,
	)

[matzf]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fbuetler)

[matzf]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @fbuetler)