pathpol: ensure deserialized ACL has default rule (scionproto#4505)

An ACL without default rule may panic during evaluation of the filter,
which is why the constructor NewACL checks that such a default rule
exists.

Add the same check also to the deserialization from JSON/YAML.

Fixes scionproto#4504

private/path/pathpol/BUILD.bazel

@@ -45,5 +45,6 @@ go_test(
  "@com_github_golang_mock//gomock:go_default_library",
  "@com_github_stretchr_testify//assert:go_default_library",
  "@com_github_stretchr_testify//require:go_default_library",
+ "@in_gopkg_yaml_v2//:go_default_library",
  ],
 )

private/path/pathpol/acl.go

@@ -27,6 +27,8 @@ import (
 var (
  // ErrNoDefault indicates that there is no default acl entry.
  ErrNoDefault = errors.New("ACL does not have a default")
+ // ErrExtraEntries indicates that there extra entries after the default entry.
+ ErrExtraEntries = errors.New("ACL has unused extra entries after a default entry")
 )
 
 type ACL struct {
@@ -35,8 +37,8 @@ type ACL struct {
 
 // NewACL creates a new entry and checks for the presence of a default action
 func NewACL(entries ...*ACLEntry) (*ACL, error) {
- if len(entries) == 0 || !entries[len(entries)-1].Rule.matchesAll() {
- return nil, ErrNoDefault
+ if err := validateACL(entries); err != nil {
+ return nil, err
  }
  return &ACL{Entries: entries}, nil
 }
@@ -61,15 +63,21 @@ func (a *ACL) MarshalJSON() ([]byte, error) {
 }
 
 func (a *ACL) UnmarshalJSON(b []byte) error {
- return json.Unmarshal(b, &a.Entries)
+ if err := json.Unmarshal(b, &a.Entries); err != nil {
+ return err
+ }
+ return validateACL(a.Entries)
 }
 
 func (a *ACL) MarshalYAML() (interface{}, error) {
  return a.Entries, nil
 }
 
 func (a *ACL) UnmarshalYAML(unmarshal func(interface{}) error) error {
- return unmarshal(&a.Entries)
+ if err := unmarshal(&a.Entries); err != nil {
+ return err
+ }
+ return validateACL(a.Entries)
 }
 
 func (a *ACL) evalPath(pm *snet.PathMetadata) ACLAction {
@@ -90,6 +98,30 @@ func (a *ACL) evalInterface(iface snet.PathInterface, ingress bool) ACLAction {
  panic("Default ACL action missing")
 }
 
+func validateACL(entries []*ACLEntry) error {
+ if len(entries) == 0 {
+ return ErrNoDefault
+ }
+
+ foundAt := -1
+ for i, e := range entries {
+ if e.Rule.matchesAll() {
+ foundAt = i
+ break
+ }
+ }
+
+ if foundAt < 0 {
+ return ErrNoDefault
+ }
+
+ if foundAt != len(entries)-1 {
+ return ErrExtraEntries
+ }
+
+ return nil
+}
+
 type ACLEntry struct {
  Action ACLAction
  Rule *HopPredicate

private/path/pathpol/acl_test.go

@@ -15,10 +15,12 @@
 package pathpol
 
 import (
+ "encoding/json"
  "testing"
 
  "github.com/golang/mock/gomock"
  "github.com/stretchr/testify/assert"
+ "gopkg.in/yaml.v2"
 
  "github.com/scionproto/scion/pkg/addr"
  "github.com/scionproto/scion/pkg/private/common"
@@ -60,6 +62,48 @@ func TestNewACL(t *testing.T) {
  }
 }
 
+func TestUnmarshal(t *testing.T) {
+ tests := map[string]struct {
+ Input string
+ ExpectedErr error
+ }{
+ "No entry": {
+ Input: `[]`,
+ ExpectedErr: ErrNoDefault,
+ },
+ "No default entry": {
+ Input: `["+ 42"]`,
+ ExpectedErr: ErrNoDefault,
+ },
+ "Entry without rule": {
+ Input: `["+"]`,
+ },
+ "Entry with hop predicates": {
+ Input: `["+ 42", "-"]`,
+ },
+ "Extra entries (first)": {
+ Input: `["-", "+ 27"]`,
+ ExpectedErr: ErrExtraEntries,
+ },
+ "Extra entries (in the middle)": {
+ Input: `["+ 42", "-", "+ 27", "- 30"]`,
+ ExpectedErr: ErrExtraEntries,
+ },
+ }
+ for name, test := range tests {
+ t.Run("json: "+name, func(t *testing.T) {
+ var acl ACL
+ err := json.Unmarshal([]byte(test.Input), &acl)
+ assert.ErrorIs(t, err, test.ExpectedErr)
+ })
+ t.Run("yaml: "+name, func(t *testing.T) {
+ var acl ACL
+ err := yaml.Unmarshal([]byte(test.Input), &acl)
+ assert.ErrorIs(t, err, test.ExpectedErr)
+ })
+ }
+}
+
 func TestACLEntryLoadFromString(t *testing.T) {
  tests := map[string]struct {
  String string

private/path/pathpol/policy_test.go

@@ -586,16 +586,17 @@ func TestFilterOpt(t *testing.T) {
 }
 
 func TestPolicyJsonConversion(t *testing.T) {
+ acl, err := NewACL(
+ &ACLEntry{Action: Allow, Rule: mustHopPredicate(t, "42-0#0")},
+ denyEntry,
+ )
+ require.NoError(t, err)
+
  policy := NewPolicy("", nil, nil, []Option{
  {
  Policy: &ExtPolicy{
  Policy: &Policy{
- ACL: &ACL{
- Entries: []*ACLEntry{
- {Action: Allow, Rule: mustHopPredicate(t, "0-0#0")},
- denyEntry,
- },
- },
+ ACL: acl,
  LocalISDAS: &LocalISDAS{
  AllowedIAs: []addr.IA{
  xtest.MustParseIA("64-123"),