From 2b5e52b08b1e36944c3b101b8a365e66c638b3e5 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 30 Aug 2024 16:58:29 +0200 Subject: [PATCH] validate policy against nodes, error if not valid (#2089) * validate policy against nodes, error if not valid this commit aims to improve the feedback of "runtime" policy errors which would only manifest when the rules are compiled to filter rules with nodes. this change will in; file-based mode load the nodes from the db and try to compile the rules on start up and return an error if they would not work as intended. database-based mode prevent a new ACL being written to the database if it does not compile with the current set of node. Fixes #2073 Fixes #2044 Signed-off-by: Kristoffer Dalby * ensure stderr can be used in err checks Signed-off-by: Kristoffer Dalby * test policy set validation Signed-off-by: Kristoffer Dalby * add new integration test to ghaction Signed-off-by: Kristoffer Dalby * add back defer for cli tst Signed-off-by: Kristoffer Dalby --------- Signed-off-by: Kristoffer Dalby --- .github/workflows/test-integration.yaml | 1 + hscontrol/app.go | 26 +++++++++ hscontrol/grpcv1.go | 29 +++++++++- integration/cli_test.go | 74 +++++++++++++++++++++++++ integration/dockertestutil/execute.go | 2 +- integration/hsic/hsic.go | 2 +- 6 files changed, 129 insertions(+), 5 deletions(-) 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