admin: authorize remote requests matching any permission entry (#8317)

* admin: authorize remote requests matching any permission entry

enforceAccessControls rejected a request as soon as one permission entry
did not match its method or path, so only the first entry was usable.
Allow the request if any entry permits both, and deny only after all fail.

* admin: check every access entry that holds the client key

A key listed in several access control entries was only checked
against the first one; deny only after all of them are tried.

admin.go

@@ -680,6 +680,10 @@ func (ctx Context) IdentityCredentials(logger *zap.Logger) ([]tls.Certificate, e
 // trust, and then looks for a matching, authorized public key that is allowed to access
 // the defined path(s) using the defined method(s).
 func (remote RemoteAdmin) enforceAccessControls(r *http.Request) error {
+	// the same public key may appear in more than one access control entry;
+	// the request is allowed if any entry containing the key permits it
+	var keyRecognized, pathAllowedSomewhere bool
+
 	for _, chain := range r.TLS.VerifiedChains {
 		for _, peerCert := range chain {
 			for _, adminAccess := range remote.AccessControl {
@@ -692,18 +696,15 @@ func (remote RemoteAdmin) enforceAccessControls(r *http.Request) error {
 					if !ok || !comparer.Equal(allowedKey) {
 						continue
 					}
+					keyRecognized = true
 
 					// key recognized; make sure its HTTP request is permitted
+					// by at least one of the permission entries (if none are
+					// configured, all permissions are granted)
+					if len(adminAccess.Permissions) == 0 {
+						return nil
+					}
 					for _, accessPerm := range adminAccess.Permissions {
-						// verify method
-						methodFound := accessPerm.Methods == nil || slices.Contains(accessPerm.Methods, r.Method)
-						if !methodFound {
-							return APIError{
-								HTTPStatus: http.StatusForbidden,
-								Message:    "not authorized to use this method",
-							}
-						}
-
 						// verify path
 						pathFound := accessPerm.Paths == nil
 						for _, allowedPath := range accessPerm.Paths {
@@ -713,20 +714,40 @@ func (remote RemoteAdmin) enforceAccessControls(r *http.Request) error {
 							}
 						}
 						if !pathFound {
-							return APIError{
-								HTTPStatus: http.StatusForbidden,
-								Message:    "not authorized to access this path",
-							}
+							continue
+						}
+						pathAllowedSomewhere = true
+
+						// verify method
+						if accessPerm.Methods == nil || slices.Contains(accessPerm.Methods, r.Method) {
+							// public key authorized, method and path allowed
+							return nil
 						}
 					}
 
-					// public key authorized, method and path allowed
-					return nil
+					// this entry does not permit the request; other entries
+					// containing the same key may
+					break
 				}
 			}
 		}
 	}
 
+	if keyRecognized {
+		// no entry allows this request; if the path is allowed by
+		// some entry, the method is what's missing
+		if pathAllowedSomewhere {
+			return APIError{
+				HTTPStatus: http.StatusForbidden,
+				Message:    "not authorized to use this method",
+			}
+		}
+		return APIError{
+			HTTPStatus: http.StatusForbidden,
+			Message:    "not authorized to access this path",
+		}
+	}
+
 	// in theory, this should never happen; with an unverified chain, the TLS server
 	// should not accept the connection in the first place, and the acceptable cert
 	// pool is configured using the same list of public keys we verify against

admin_test.go

@@ -805,6 +805,141 @@ func TestRemoteAdminAccessControlPathSegmentMatching(t *testing.T) {
 	}
 }
 
+func TestRemoteAdminAccessControlMultiplePermissions(t *testing.T) {
+	const authorizedKey testAdminPublicKey = "authorized"
+	peerCert := &x509.Certificate{PublicKey: authorizedKey}
+
+	tests := []struct {
+		name        string
+		permissions []AdminPermissions
+		// extraAccess adds further access control entries holding the same key
+		extraAccess [][]AdminPermissions
+		method      string
+		requestPath string
+		wantErr     string
+	}{
+		{
+			name:        "no permissions grants all",
+			permissions: nil,
+			method:      http.MethodDelete,
+			requestPath: "/config/",
+		},
+		{
+			name: "first entry matches",
+			permissions: []AdminPermissions{
+				{Methods: []string{http.MethodGet}, Paths: []string{"/config/"}},
+				{Methods: []string{http.MethodPost}, Paths: []string{"/load"}},
+			},
+			method:      http.MethodGet,
+			requestPath: "/config/",
+		},
+		{
+			name: "second entry matches",
+			permissions: []AdminPermissions{
+				{Methods: []string{http.MethodGet}, Paths: []string{"/config/"}},
+				{Methods: []string{http.MethodPost}, Paths: []string{"/load"}},
+			},
+			method:      http.MethodPost,
+			requestPath: "/load",
+		},
+		{
+			name: "same path different methods across entries",
+			permissions: []AdminPermissions{
+				{Methods: []string{http.MethodGet}, Paths: []string{"/config/"}},
+				{Methods: []string{http.MethodPatch}, Paths: []string{"/config/"}},
+			},
+			method:      http.MethodPatch,
+			requestPath: "/config/",
+		},
+		{
+			name: "method and path from different entries",
+			permissions: []AdminPermissions{
+				{Methods: []string{http.MethodGet}, Paths: []string{"/config/"}},
+				{Methods: []string{http.MethodPost}, Paths: []string{"/load"}},
+			},
+			method:      http.MethodPost,
+			requestPath: "/config/",
+			wantErr:     "not authorized to use this method",
+		},
+		{
+			name: "path allowed by no entry",
+			permissions: []AdminPermissions{
+				{Methods: []string{http.MethodGet}, Paths: []string{"/config/"}},
+				{Methods: []string{http.MethodPost}, Paths: []string{"/load"}},
+			},
+			method:      http.MethodGet,
+			requestPath: "/stop",
+			wantErr:     "not authorized to access this path",
+		},
+		{
+			name: "later access entry with the same key matches",
+			permissions: []AdminPermissions{
+				{Methods: []string{http.MethodGet}, Paths: []string{"/config/"}},
+			},
+			extraAccess: [][]AdminPermissions{
+				{{Methods: []string{http.MethodPost}, Paths: []string{"/load"}}},
+			},
+			method:      http.MethodPost,
+			requestPath: "/load",
+		},
+		{
+			name: "no access entry with the same key matches",
+			permissions: []AdminPermissions{
+				{Methods: []string{http.MethodGet}, Paths: []string{"/config/"}},
+			},
+			extraAccess: [][]AdminPermissions{
+				{{Methods: []string{http.MethodGet}, Paths: []string{"/load"}}},
+			},
+			method:      http.MethodPost,
+			requestPath: "/load",
+			wantErr:     "not authorized to use this method",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			remote := RemoteAdmin{
+				AccessControl: []*AdminAccess{
+					{
+						Permissions: test.permissions,
+						publicKeys:  []crypto.PublicKey{authorizedKey},
+					},
+				},
+			}
+			for _, permissions := range test.extraAccess {
+				remote.AccessControl = append(remote.AccessControl, &AdminAccess{
+					Permissions: permissions,
+					publicKeys:  []crypto.PublicKey{authorizedKey},
+				})
+			}
+
+			req := httptest.NewRequest(test.method, "https://localhost:2021"+test.requestPath, nil)
+			req.TLS = &tls.ConnectionState{
+				VerifiedChains: [][]*x509.Certificate{{peerCert}},
+			}
+
+			err := remote.enforceAccessControls(req)
+			if test.wantErr == "" {
+				if err != nil {
+					t.Errorf("%s %s: expected no error, got %v", test.method, test.requestPath, err)
+				}
+				return
+			}
+
+			var apiErr APIError
+			if !errors.As(err, &apiErr) {
+				t.Fatalf("%s %s: expected APIError, got %T: %v", test.method, test.requestPath, err, err)
+			}
+			if apiErr.HTTPStatus != http.StatusForbidden {
+				t.Errorf("%s %s: expected HTTP status %d, got %d", test.method, test.requestPath, http.StatusForbidden, apiErr.HTTPStatus)
+			}
+			if apiErr.Message != test.wantErr {
+				t.Errorf("%s %s: expected message %q, got %q", test.method, test.requestPath, test.wantErr, apiErr.Message)
+			}
+		})
+	}
+}
+
 func TestReplaceRemoteAdminServer(t *testing.T) {
 	const testCert = `MIIDCTCCAfGgAwIBAgIUXsqJ1mY8pKlHQtI3HJ23x2eZPqwwDQYJKoZIhvcNAQEL
 BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTIzMDEwMTAwMDAwMFoXDTI0MDEw