diff --git a/cmd/headscale/cli/policy.go b/cmd/headscale/cli/policy.go index 94f9e84b..1c6cb3dd 100644 --- a/cmd/headscale/cli/policy.go +++ b/cmd/headscale/cli/policy.go @@ -1,7 +1,6 @@ package cli import ( - "bytes" "errors" "fmt" "os" @@ -18,10 +17,7 @@ const ( bypassFlag = "bypass-grpc-and-access-database-directly" //nolint:gosec // not a credential ) -var ( - errAborted = errors.New("command aborted by user") - errTestsRequireBypass = errors.New("policy contains a tests block; rerun with --" + bypassFlag + " to evaluate it (headscale must not be running)") -) +var errAborted = errors.New("command aborted by user") // bypassDatabase loads the server config and opens the database directly, // bypassing the gRPC server. The caller is responsible for closing the @@ -176,9 +172,10 @@ var checkPolicy = &cobra.Command{ Use: "check", Short: "Check the Policy file for errors", Long: ` - Check parses the policy and validates its structure. If the policy contains a - "tests" block, those tests are evaluated against the live users and nodes: - pass --` + bypassFlag + ` to open the database directly when headscale is not running.`, + Check validates the policy against the server's live users and nodes, + running any "tests" block. By default the command is a thin frontend + for a gRPC call to a running headscale; pass --` + bypassFlag + ` to + open the database directly when headscale is not running.`, RunE: func(cmd *cobra.Command, args []string) error { policyPath, _ := cmd.Flags().GetString("file") @@ -187,56 +184,54 @@ var checkPolicy = &cobra.Command{ return fmt.Errorf("reading policy file: %w", err) } - bypass, _ := cmd.Flags().GetBool(bypassFlag) + if bypass, _ := cmd.Flags().GetBool(bypassFlag); bypass { + if !confirmAction(cmd, "DO NOT run this command if an instance of headscale is running, are you sure headscale is not running?") { + return errAborted + } - // Without bypass we don't have users or nodes to resolve user@ - // tokens or test aliases against. Validate structure and warn - // explicitly if tests are present, rather than running them - // with empty data and reporting spurious failures. - if !bypass { - _, err = policy.NewPolicyManager(policyBytes, nil, views.Slice[types.NodeView]{}) + d, err := bypassDatabase() + if err != nil { + return err + } + defer d.Close() + + users, err := d.ListUsers() + if err != nil { + return fmt.Errorf("loading users: %w", err) + } + + nodes, err := d.ListNodes() + if err != nil { + return fmt.Errorf("loading nodes: %w", err) + } + + // NewPolicyManager validates structure and user references + // but intentionally skips test evaluation (boot path). + // SetPolicy is the user-write boundary and is what runs the + // tests block. + pm, err := policy.NewPolicyManager(policyBytes, users, nodes.ViewSlice()) if err != nil { return fmt.Errorf("parsing policy file: %w", err) } - if policyHasTestsBlock(policyBytes) { - return errTestsRequireBypass + _, err = pm.SetPolicy(policyBytes) + if err != nil { + return err } - fmt.Println("Policy syntax is valid (run with --" + bypassFlag + " to also validate user references against the database)") + fmt.Println("Policy is valid") return nil } - if !confirmAction(cmd, "DO NOT run this command if an instance of headscale is running, are you sure headscale is not running?") { - return errAborted - } - - d, err := bypassDatabase() + ctx, client, conn, cancel, err := newHeadscaleCLIWithConfig() if err != nil { - return err + return fmt.Errorf("connecting to headscale: %w", err) } - defer d.Close() + defer cancel() + defer conn.Close() - users, err := d.ListUsers() - if err != nil { - return fmt.Errorf("loading users: %w", err) - } - - nodes, err := d.ListNodes() - if err != nil { - return fmt.Errorf("loading nodes: %w", err) - } - - // NewPolicyManager validates structure and user references but - // intentionally skips test evaluation (boot path). SetPolicy is - // the user-write boundary and is what runs the tests block. - pm, err := policy.NewPolicyManager(policyBytes, users, nodes.ViewSlice()) - if err != nil { - return fmt.Errorf("parsing policy file: %w", err) - } - - _, err = pm.SetPolicy(policyBytes) + _, err = client.CheckPolicy(ctx, &v1.CheckPolicyRequest{Policy: string(policyBytes)}) if err != nil { return err } @@ -246,11 +241,3 @@ var checkPolicy = &cobra.Command{ return nil }, } - -// policyHasTestsBlock is a cheap textual probe for `"tests"` so callers -// can warn the user when they need to rerun with bypass. False positives -// (e.g. a comment containing the word) are harmless: the user is told to -// pass a flag they could have passed anyway. -func policyHasTestsBlock(b []byte) bool { - return bytes.Contains(b, []byte(`"tests"`)) -} diff --git a/cmd/headscale/cli/root.go b/cmd/headscale/cli/root.go index a8a9bf69..fa7ded2d 100644 --- a/cmd/headscale/cli/root.go +++ b/cmd/headscale/cli/root.go @@ -3,7 +3,6 @@ package cli import ( "os" "runtime" - "slices" "strings" "github.com/juanfont/headscale/hscontrol/types" @@ -22,11 +21,6 @@ func init() { return } - if slices.Contains(os.Args, "policy") && slices.Contains(os.Args, "check") { - zerolog.SetGlobalLevel(zerolog.Disabled) - return - } - cobra.OnInitialize(initConfig) rootCmd.PersistentFlags(). StringVarP(&cfgFile, "config", "c", "", "config file (default is /etc/headscale/config.yaml)") diff --git a/hscontrol/grpcv1.go b/hscontrol/grpcv1.go index 48d6e2d1..953ae0d7 100644 --- a/hscontrol/grpcv1.go +++ b/hscontrol/grpcv1.go @@ -26,6 +26,7 @@ import ( "tailscale.com/types/views" v1 "github.com/juanfont/headscale/gen/go/headscale/v1" + policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" "github.com/juanfont/headscale/hscontrol/state" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" @@ -781,6 +782,35 @@ func (api headscaleV1APIServer) SetPolicy( return response, nil } +// CheckPolicy validates the given policy against the server's live users +// and nodes, running its `tests` block as a sandbox. Nothing is persisted +// and the live PolicyManager is not touched. Works regardless of +// policy.mode so operators can validate a policy file before storing it. +func (api headscaleV1APIServer) CheckPolicy( + _ context.Context, + request *v1.CheckPolicyRequest, +) (*v1.CheckPolicyResponse, error) { + polB := []byte(request.GetPolicy()) + + users, err := api.h.state.ListAllUsers() + if err != nil { + return nil, status.Errorf(codes.Internal, "loading users: %s", err) + } + + nodes := api.h.state.ListNodes() + + pm, err := policyv2.NewPolicyManager(polB, users, nodes) + if err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + if _, err := pm.SetPolicy(polB); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + return &v1.CheckPolicyResponse{}, nil +} + // The following service calls are for testing and debugging func (api headscaleV1APIServer) DebugCreateNode( ctx context.Context, diff --git a/integration/cli_policy_test.go b/integration/cli_policy_test.go new file mode 100644 index 00000000..2f8a0c11 --- /dev/null +++ b/integration/cli_policy_test.go @@ -0,0 +1,166 @@ +package integration + +import ( + "encoding/json" + "testing" + + policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2" + "github.com/juanfont/headscale/integration/hsic" + "github.com/juanfont/headscale/integration/tsic" + "github.com/stretchr/testify/require" + "tailscale.com/tailcfg" +) + +// TestPolicyCheckCommand exercises `headscale policy check` across the +// matrix that nblock asked about on PR #3229: +// +// - policyMode: server runs with policy_mode=file vs policy_mode=database. +// `check` reads from `--file`, so the server-side mode should not +// change the outcome; running both proves that. +// - fixture: ACL only, ACL with passing tests, ACL with failing tests. +// - bypass: no-bypass talks to the server over gRPC; bypass opens the +// database directly. +// +// Each row spins up its own scenario because policy_mode is fixed at boot +// via `HEADSCALE_POLICY_MODE`. The two users + two nodes give the tests +// block real `user@` aliases to resolve against. +func TestPolicyCheckCommand(t *testing.T) { + IntegrationSkip(t) + + type fixture struct { + name string + policy policyv2.Policy + } + + const ( + user1 = "user1@" + user2 = "user2@" + ) + + aclOnly := policyv2.Policy{ + ACLs: []policyv2.ACL{ + { + Action: policyv2.ActionAccept, + Protocol: "tcp", //nolint:goconst // protocol literal, used inline once + Sources: []policyv2.Alias{usernamep(user1)}, + Destinations: []policyv2.AliasWithPorts{ + aliasWithPorts(usernamep(user2), tailcfg.PortRange{First: 22, Last: 22}), + }, + }, + }, + } + + aclPlusPassingTests := aclOnly + aclPlusPassingTests.Tests = []policyv2.PolicyTest{ + { + Src: user1, + Accept: []string{user2 + ":22"}, + }, + } + + aclPlusFailingTests := aclOnly + aclPlusFailingTests.Tests = []policyv2.PolicyTest{ + { + // Reverse direction is not allowed by the ACL; the test + // asserts ALLOWED, so it must fail. + Src: user2, + Accept: []string{user1 + ":22"}, + }, + } + + fixtures := []fixture{ + {name: "acl-only", policy: aclOnly}, + {name: "acl-plus-passing-tests", policy: aclPlusPassingTests}, + {name: "acl-plus-failing-tests", policy: aclPlusFailingTests}, + } + + type row struct { + name string + policyMode string + fixture fixture + bypass bool + wantErr string + wantStdout string + } + + modes := []string{"file", "database"} //nolint:goconst // axis labels match HEADSCALE_POLICY_MODE values + bypasses := []bool{false, true} + rows := make([]row, 0, len(modes)*len(fixtures)*len(bypasses)) + + for _, mode := range modes { + for _, f := range fixtures { + for _, bypass := range bypasses { + suffix := "no-bypass" + if bypass { + suffix = "bypass" + } + + r := row{ + name: mode + "-" + f.name + "-" + suffix, + policyMode: mode, + fixture: f, + bypass: bypass, + wantStdout: "Policy is valid", + } + if f.name == "acl-plus-failing-tests" { + r.wantErr = "test(s) failed" + r.wantStdout = "" + } + + rows = append(rows, r) + } + } + } + + for _, tt := range rows { + t.Run(tt.name, func(t *testing.T) { + spec := ScenarioSpec{ + NodesPerUser: 1, + Users: []string{"user1", "user2"}, //nolint:goconst // matches usernamep("user1@")/("user2@") above + } + + scenario, err := NewScenario(spec) + require.NoError(t, err) + + defer scenario.ShutdownAssertNoPanics(t) + + err = scenario.CreateHeadscaleEnv( + []tsic.Option{}, + hsic.WithTestName("cli-policycheck"), + hsic.WithConfigEnv(map[string]string{ + "HEADSCALE_POLICY_MODE": tt.policyMode, //nolint:goconst // env var name from hscontrol/types/config.go + }), + ) + require.NoError(t, err) + + headscale, err := scenario.Headscale() + require.NoError(t, err) + + pBytes, err := json.Marshal(tt.fixture.policy) + require.NoError(t, err) + + policyFilePath := "/etc/headscale/policy.json" //nolint:goconst // standard headscale policy path + err = headscale.WriteFile(policyFilePath, pBytes) + require.NoError(t, err) + + cmd := []string{"headscale", "policy", "check", "-f", policyFilePath} //nolint:goconst // CLI invocation + if tt.bypass { + // --force suppresses the "is the server running?" + // confirmation prompt so the command can run + // non-interactively under the test harness. + cmd = append(cmd, "--bypass-grpc-and-access-database-directly", "--force") + } + + stdout, err := headscale.Execute(cmd) + + if tt.wantErr != "" { + require.ErrorContains(t, err, tt.wantErr) + + return + } + + require.NoError(t, err) + require.Contains(t, stdout, tt.wantStdout) + }) + } +}