VAULT-23533: Audit - distinct audit devices

audit/backend.go

@@ -6,6 +6,7 @@ package audit
 import (
 	"context"
 	"fmt"
+	"strings"
 	"sync"
 	"sync/atomic"
 
@@ -265,3 +266,50 @@ func hasEnterpriseAuditOptions(options map[string]string) bool {
 
 	return false
 }
+
+// backendComparer describes the methods required for a backend to be comparable.
+type backendComparer interface {
+	Options(key string) string
+	Path() string
+	Type() string
+}
+
+// IsDistinct checks some basic properties to determine if the proposed backend is
+// distinct amongst another (existing) backend, initially by the backend's path.
+// Depending on the type of audit backend, it will examine a specific option to
+// ensure devices with different paths cannot use the same sink.
+func IsDistinct(proposed backendComparer, existing backendComparer) (bool, error) {
+	// Examples:
+	// existing: 'sql/mysql/', new: 'sql/mysql/' or
+	// existing: 'sql/mysql/', new: 'sql/' or
+	// existing: 'sql/', new: 'sql/mysql/'
+	if strings.EqualFold(proposed.Path(), existing.Path()) ||
+		strings.HasPrefix(proposed.Path(), existing.Path()) ||
+		strings.HasPrefix(existing.Path(), proposed.Path()) {
+		return false, fmt.Errorf("path %q already in use: %w", existing.Path(), ErrExternalOptions)
+	}
+
+	// Depending on which audit device type we have, check the relevant option
+	// to ensure it doesn't already exist.
+	var opt string
+	switch proposed.Type() {
+	case TypeFile:
+		opt = optionFilePath
+	case TypeSocket:
+		opt = optionAddress
+	case TypeSyslog:
+		opt = optionFacility
+	default:
+		// Not an audit entry
+		return false, fmt.Errorf("unexpected 'type' for audit: %q: %w", proposed.Type(), ErrInvalidParameter)
+	}
+
+	opt1 := proposed.Options(opt)
+	opt2 := existing.Options(opt)
+
+	if isMatch := strings.EqualFold(opt1, opt2); isMatch {
+		return false, fmt.Errorf("%q %q already in use on device: %q: %w", opt, opt1, existing.Path(), ErrExternalOptions)
+	}
+
+	return true, nil
+}

audit/backend_test.go

@@ -12,6 +12,32 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// testAuditEntryMinimal represents a minimal version of a MountEntry which would be
+// required by the audit system in order to compare two entries and establish if
+// they are distinct. It is declared here in testing, because 'auditEntryMinimal'
+// is not exported, and 'MountEntry' exists in the 'vault' package and would cause
+// an import cycle.
+type testAuditEntryMinimal struct {
+	path       string
+	deviceType string
+	options    map[string]string
+}
+
+// Type returns the device type of the entry.
+func (m *testAuditEntryMinimal) Type() string {
+	return m.deviceType
+}
+
+// Path returns the device mount path of the entry.
+func (m *testAuditEntryMinimal) Path() string {
+	return m.path
+}
+
+// Options returns the specified option given a key.
+func (m *testAuditEntryMinimal) Options(key string) string {
+	return m.options[key]
+}
+
 // TestBackend_newFormatterConfig ensures that all the configuration values are
 // parsed correctly when trying to create a new formatterConfig via newFormatterConfig.
 func TestBackend_newFormatterConfig(t *testing.T) {
@@ -252,3 +278,120 @@ func TestBackend_hasInvalidAuditOptions(t *testing.T) {
 		})
 	}
 }
+
+// TestBackend_IsDistinct tests a variety of scenarios for deciding whether a proposed
+// audit backend is distinct from an existing one (basically just comparing some
+// properties of two backends).
+func TestBackend_IsDistinct(t *testing.T) {
+	t.Parallel()
+
+	tests := map[string]struct {
+		isDistinctExpected   bool
+		expectedErrorMessage string
+		proposed             backendComparer
+		existing             backendComparer
+	}{
+		"different-path-and-file-path": {
+			isDistinctExpected: true,
+			proposed: &testAuditEntryMinimal{
+				path:       "foo1/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit.log"},
+			},
+			existing: &testAuditEntryMinimal{
+				path:       "foo2/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit2.log"},
+			},
+		},
+		"different-path-same-file-path": {
+			isDistinctExpected:   false,
+			expectedErrorMessage: "\"file_path\" \"/var/vault/audit.log\" already in use on device: \"foo2/\": invalid configuration",
+			proposed: &testAuditEntryMinimal{
+				path:       "foo1/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit.log"},
+			},
+			existing: &testAuditEntryMinimal{
+				path:       "foo2/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit.log"},
+			},
+		},
+		"same-path-different-file-path": {
+			isDistinctExpected:   false,
+			expectedErrorMessage: "path \"foo1/\" already in use: invalid configuration",
+			proposed: &testAuditEntryMinimal{
+				path:       "foo1/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit2.log"},
+			},
+			existing: &testAuditEntryMinimal{
+				path:       "foo1/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit.log"},
+			},
+		},
+		"bad-type": {
+			isDistinctExpected:   false,
+			expectedErrorMessage: "unexpected 'type' for audit: \"juan\": invalid internal parameter",
+			proposed: &testAuditEntryMinimal{
+				path:       "foo2/",
+				deviceType: "juan",
+				options:    map[string]string{"file_path": "/var/vault/audit2.log"},
+			},
+			existing: &testAuditEntryMinimal{
+				path:       "foo1/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit.log"},
+			},
+		},
+		// These tests feel odd, but it's what the code was doing before any move
+		// to eventlogger:
+		// https://github.com/hashicorp/vault/blob/release/1.14.x/vault/audit.go#L81-L86
+		"proposed-has-prefix-of-existing": {
+			isDistinctExpected:   false,
+			expectedErrorMessage: "path \"foo1/\" already in use: invalid configuration",
+			proposed: &testAuditEntryMinimal{
+				path:       "foo1/bar/",
+				deviceType: "juan",
+				options:    map[string]string{"file_path": "/var/vault/audit2.log"},
+			},
+			existing: &testAuditEntryMinimal{
+				path:       "foo1/",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit.log"},
+			},
+		},
+		"existing-has-prefix-of-proposed": {
+			isDistinctExpected:   false,
+			expectedErrorMessage: "path \"foo1/bar\" already in use: invalid configuration",
+			proposed: &testAuditEntryMinimal{
+				path:       "foo1/",
+				deviceType: "juan",
+				options:    map[string]string{"file_path": "/var/vault/audit2.log"},
+			},
+			existing: &testAuditEntryMinimal{
+				path:       "foo1/bar",
+				deviceType: "file",
+				options:    map[string]string{"file_path": "/var/vault/audit.log"},
+			},
+		},
+	}
+
+	for name, tc := range tests {
+		name := name
+		tc := tc
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			ok, err := IsDistinct(tc.proposed, tc.existing)
+			require.Equal(t, tc.isDistinctExpected, ok)
+			if tc.isDistinctExpected {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				require.EqualError(t, err, tc.expectedErrorMessage)
+			}
+		})
+	}
+}

vault/audit.go

@@ -105,15 +105,12 @@ func (c *Core) enableAudit(ctx context.Context, entry *MountEntry, updateStorage
 	c.auditLock.Lock()
 	defer c.auditLock.Unlock()
 
-	// Look for matching name
-	for _, ent := range c.audit.Entries {
-		switch {
-		// Existing is sql/mysql/ new is sql/ or
-		// existing is sql/ and new is sql/mysql/
-		case strings.HasPrefix(ent.Path, entry.Path):
-			fallthrough
-		case strings.HasPrefix(entry.Path, ent.Path):
-			return fmt.Errorf("path already in use: %w", audit.ErrExternalOptions)
+	// Ensure distinctness.
+	proposed := entry.toAuditEntryMinimal()
+	for _, existing := range c.audit.Entries {
+		existing := existing.toAuditEntryMinimal()
+		if ok, err := audit.IsDistinct(proposed, existing); !ok {
+			return err
 		}
 	}
 
@@ -628,3 +625,36 @@ func (g genericAuditor) AuditResponse(ctx context.Context, input *logical.LogInp
 	logInput.Type = g.mountType + "-response"
 	return g.c.auditBroker.LogResponse(ctx, &logInput)
 }
+
+// auditEntryMinimal represents a minimal version of a MountEntry which would be
+// required by the audit system in order to compare two entries and establish if
+// they are distinct.
+type auditEntryMinimal struct {
+	path       string
+	deviceType string
+	options    map[string]string
+}
+
+// Type returns the device type of the entry.
+func (m *auditEntryMinimal) Type() string {
+	return m.deviceType
+}
+
+// Path returns the device mount path of the entry.
+func (m *auditEntryMinimal) Path() string {
+	return m.path
+}
+
+// Options returns the specified option given a key.
+func (m *auditEntryMinimal) Options(key string) string {
+	return m.options[key]
+}
+
+// toAuditEntryMinimal converts a MountEntry to an auditEntryMinimal.
+func (e *MountEntry) toAuditEntryMinimal() *auditEntryMinimal {
+	return &auditEntryMinimal{
+		options:    e.Options,
+		deviceType: e.Type,
+		path:       e.Path,
+	}
+}