diff --git a/.github/workflows/test-integration.yaml b/.github/workflows/test-integration.yaml index 6203e51b..aa220261 100644 --- a/.github/workflows/test-integration.yaml +++ b/.github/workflows/test-integration.yaml @@ -37,6 +37,7 @@ jobs: - TestNodeRenameCommand - TestNodeMoveCommand - TestPolicyCommand + - TestPolicyBrokenConfigCommand - TestResolveMagicDNS - TestValidateResolvConf - TestDERPServerScenario diff --git a/hscontrol/app.go b/hscontrol/app.go index b66e939b..087d2f2a 100644 --- a/hscontrol/app.go +++ b/hscontrol/app.go @@ -1001,6 +1001,32 @@ func (h *Headscale) loadACLPolicy() error { if err != nil { return fmt.Errorf("failed to load ACL policy from file: %w", err) } + + // Validate and reject configuration that would error when applied + // when creating a map response. This requires nodes, so there is still + // a scenario where they might be allowed if the server has no nodes + // yet, but it should help for the general case and for hot reloading + // configurations. + // Note that this check is only done for file-based policies in this function + // as the database-based policies are checked in the gRPC API where it is not + // allowed to be written to the database. + nodes, err := h.db.ListNodes() + if err != nil { + return fmt.Errorf("loading nodes from database to validate policy: %w", err) + } + + _, err = pol.CompileFilterRules(nodes) + if err != nil { + return fmt.Errorf("verifying policy rules: %w", err) + } + + if len(nodes) > 0 { + _, err = pol.CompileSSHPolicy(nodes[0], nodes) + if err != nil { + return fmt.Errorf("verifying SSH rules: %w", err) + } + } + case types.PolicyModeDB: p, err := h.db.GetPolicy() if err != nil { diff --git a/hscontrol/grpcv1.go b/hscontrol/grpcv1.go index d4e10849..83048bec 100644 --- a/hscontrol/grpcv1.go +++ b/hscontrol/grpcv1.go @@ -4,6 +4,7 @@ package hscontrol import ( "context" "errors" + "fmt" "io" "os" "sort" @@ -721,9 +722,31 @@ func (api headscaleV1APIServer) SetPolicy( p := request.GetPolicy() - valid, err := policy.LoadACLPolicyFromBytes([]byte(p)) + pol, err := policy.LoadACLPolicyFromBytes([]byte(p)) if err != nil { - return nil, err + return nil, fmt.Errorf("loading ACL policy file: %w", err) + } + + // Validate and reject configuration that would error when applied + // when creating a map response. This requires nodes, so there is still + // a scenario where they might be allowed if the server has no nodes + // yet, but it should help for the general case and for hot reloading + // configurations. + nodes, err := api.h.db.ListNodes() + if err != nil { + return nil, fmt.Errorf("loading nodes from database to validate policy: %w", err) + } + + _, err = pol.CompileFilterRules(nodes) + if err != nil { + return nil, fmt.Errorf("verifying policy rules: %w", err) + } + + if len(nodes) > 0 { + _, err = pol.CompileSSHPolicy(nodes[0], nodes) + if err != nil { + return nil, fmt.Errorf("verifying SSH rules: %w", err) + } } updated, err := api.h.db.SetPolicy(p) @@ -731,7 +754,7 @@ func (api headscaleV1APIServer) SetPolicy( return nil, err } - api.h.ACLPolicy = valid + api.h.ACLPolicy = pol ctx := types.NotifyCtx(context.Background(), "acl-update", "na") api.h.nodeNotifier.NotifyAll(ctx, types.StateUpdate{ diff --git a/integration/cli_test.go b/integration/cli_test.go index 088db786..9e7d179f 100644 --- a/integration/cli_test.go +++ b/integration/cli_test.go @@ -1676,3 +1676,77 @@ func TestPolicyCommand(t *testing.T) { assert.Len(t, output.ACLs, 1) assert.Equal(t, output.TagOwners["tag:exists"], []string{"policy-user"}) } + +func TestPolicyBrokenConfigCommand(t *testing.T) { + IntegrationSkip(t) + t.Parallel() + + scenario, err := NewScenario(dockertestMaxWait()) + assertNoErr(t, err) + defer scenario.Shutdown() + + spec := map[string]int{ + "policy-user": 1, + } + + err = scenario.CreateHeadscaleEnv( + spec, + []tsic.Option{}, + hsic.WithTestName("clins"), + hsic.WithConfigEnv(map[string]string{ + "HEADSCALE_POLICY_MODE": "database", + }), + ) + assertNoErr(t, err) + + headscale, err := scenario.Headscale() + assertNoErr(t, err) + + p := policy.ACLPolicy{ + ACLs: []policy.ACL{ + { + // This is an unknown action, so it will return an error + // and the config will not be applied. + Action: "acccept", + Sources: []string{"*"}, + Destinations: []string{"*:*"}, + }, + }, + TagOwners: map[string][]string{ + "tag:exists": {"policy-user"}, + }, + } + + pBytes, _ := json.Marshal(p) + + policyFilePath := "/etc/headscale/policy.json" + + err = headscale.WriteFile(policyFilePath, pBytes) + assertNoErr(t, err) + + // No policy is present at this time. + // Add a new policy from a file. + _, err = headscale.Execute( + []string{ + "headscale", + "policy", + "set", + "-f", + policyFilePath, + }, + ) + assert.ErrorContains(t, err, "verifying policy rules: invalid action") + + // The new policy was invalid, the old one should still be in place, which + // is none. + _, err = headscale.Execute( + []string{ + "headscale", + "policy", + "get", + "--output", + "json", + }, + ) + assert.ErrorContains(t, err, "acl policy not found") +} diff --git a/integration/dockertestutil/execute.go b/integration/dockertestutil/execute.go index 5a8e92b3..1b41e324 100644 --- a/integration/dockertestutil/execute.go +++ b/integration/dockertestutil/execute.go @@ -62,7 +62,7 @@ func ExecuteCommand( exitCode, err := resource.Exec( cmd, dockertest.ExecOptions{ - Env: append(env, "HEADSCALE_LOG_LEVEL=disabled"), + Env: append(env, "HEADSCALE_LOG_LEVEL=info"), StdOut: &stdout, StdErr: &stderr, }, diff --git a/integration/hsic/hsic.go b/integration/hsic/hsic.go index 0b5a6be3..bef05818 100644 --- a/integration/hsic/hsic.go +++ b/integration/hsic/hsic.go @@ -551,7 +551,7 @@ func (t *HeadscaleInContainer) Execute( log.Printf("command stdout: %s\n", stdout) } - return "", err + return stdout, fmt.Errorf("executing command in docker: %w, stderr: %s", err, stderr) } return stdout, nil