From c7b459b615b5862681985989ebecfadf8b301fa3 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Mon, 27 Mar 2023 19:19:32 +0200 Subject: [PATCH] Fix issue where ACL * would filter out returning connections (#1279) --- ...st-integration-v2-TestACLAllowStarDst.yaml | 57 ++++++ ...st-integration-v2-TestACLAllowUserDst.yaml | 57 ++++++ ...t-integration-v2-TestACLDenyAllPort80.yaml | 57 ++++++ CHANGELOG.md | 2 + acls.go | 2 + integration/acl_test.go | 163 +++++++++++++++++- machine.go | 6 + machine_test.go | 98 ++++++++++- 8 files changed, 437 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/test-integration-v2-TestACLAllowStarDst.yaml create mode 100644 .github/workflows/test-integration-v2-TestACLAllowUserDst.yaml create mode 100644 .github/workflows/test-integration-v2-TestACLDenyAllPort80.yaml diff --git a/.github/workflows/test-integration-v2-TestACLAllowStarDst.yaml b/.github/workflows/test-integration-v2-TestACLAllowStarDst.yaml new file mode 100644 index 00000000..47a1b60d --- /dev/null +++ b/.github/workflows/test-integration-v2-TestACLAllowStarDst.yaml @@ -0,0 +1,57 @@ +# DO NOT EDIT, generated with cmd/gh-action-integration-generator/main.go +# To regenerate, run "go generate" in cmd/gh-action-integration-generator/ + +name: Integration Test v2 - TestACLAllowStarDst + +on: [pull_request] + +concurrency: + group: ${{ github.workflow }}-$${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 2 + + - name: Get changed files + id: changed-files + uses: tj-actions/changed-files@v34 + with: + files: | + *.nix + go.* + **/*.go + integration_test/ + config-example.yaml + + - uses: cachix/install-nix-action@v18 + if: ${{ env.ACT }} || steps.changed-files.outputs.any_changed == 'true' + + - name: Run general integration tests + if: steps.changed-files.outputs.any_changed == 'true' + run: | + nix develop --command -- docker run \ + --tty --rm \ + --volume ~/.cache/hs-integration-go:/go \ + --name headscale-test-suite \ + --volume $PWD:$PWD -w $PWD/integration \ + --volume /var/run/docker.sock:/var/run/docker.sock \ + --volume $PWD/control_logs:/tmp/control \ + golang:1 \ + go test ./... \ + -tags ts2019 \ + -failfast \ + -timeout 120m \ + -parallel 1 \ + -run "^TestACLAllowStarDst$" + + - uses: actions/upload-artifact@v3 + if: always() && steps.changed-files.outputs.any_changed == 'true' + with: + name: logs + path: "control_logs/*.log" diff --git a/.github/workflows/test-integration-v2-TestACLAllowUserDst.yaml b/.github/workflows/test-integration-v2-TestACLAllowUserDst.yaml new file mode 100644 index 00000000..d745e871 --- /dev/null +++ b/.github/workflows/test-integration-v2-TestACLAllowUserDst.yaml @@ -0,0 +1,57 @@ +# DO NOT EDIT, generated with cmd/gh-action-integration-generator/main.go +# To regenerate, run "go generate" in cmd/gh-action-integration-generator/ + +name: Integration Test v2 - TestACLAllowUserDst + +on: [pull_request] + +concurrency: + group: ${{ github.workflow }}-$${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 2 + + - name: Get changed files + id: changed-files + uses: tj-actions/changed-files@v34 + with: + files: | + *.nix + go.* + **/*.go + integration_test/ + config-example.yaml + + - uses: cachix/install-nix-action@v18 + if: ${{ env.ACT }} || steps.changed-files.outputs.any_changed == 'true' + + - name: Run general integration tests + if: steps.changed-files.outputs.any_changed == 'true' + run: | + nix develop --command -- docker run \ + --tty --rm \ + --volume ~/.cache/hs-integration-go:/go \ + --name headscale-test-suite \ + --volume $PWD:$PWD -w $PWD/integration \ + --volume /var/run/docker.sock:/var/run/docker.sock \ + --volume $PWD/control_logs:/tmp/control \ + golang:1 \ + go test ./... \ + -tags ts2019 \ + -failfast \ + -timeout 120m \ + -parallel 1 \ + -run "^TestACLAllowUserDst$" + + - uses: actions/upload-artifact@v3 + if: always() && steps.changed-files.outputs.any_changed == 'true' + with: + name: logs + path: "control_logs/*.log" diff --git a/.github/workflows/test-integration-v2-TestACLDenyAllPort80.yaml b/.github/workflows/test-integration-v2-TestACLDenyAllPort80.yaml new file mode 100644 index 00000000..767aa213 --- /dev/null +++ b/.github/workflows/test-integration-v2-TestACLDenyAllPort80.yaml @@ -0,0 +1,57 @@ +# DO NOT EDIT, generated with cmd/gh-action-integration-generator/main.go +# To regenerate, run "go generate" in cmd/gh-action-integration-generator/ + +name: Integration Test v2 - TestACLDenyAllPort80 + +on: [pull_request] + +concurrency: + group: ${{ github.workflow }}-$${{ github.head_ref || github.run_id }} + cancel-in-progress: true + +jobs: + test: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + with: + fetch-depth: 2 + + - name: Get changed files + id: changed-files + uses: tj-actions/changed-files@v34 + with: + files: | + *.nix + go.* + **/*.go + integration_test/ + config-example.yaml + + - uses: cachix/install-nix-action@v18 + if: ${{ env.ACT }} || steps.changed-files.outputs.any_changed == 'true' + + - name: Run general integration tests + if: steps.changed-files.outputs.any_changed == 'true' + run: | + nix develop --command -- docker run \ + --tty --rm \ + --volume ~/.cache/hs-integration-go:/go \ + --name headscale-test-suite \ + --volume $PWD:$PWD -w $PWD/integration \ + --volume /var/run/docker.sock:/var/run/docker.sock \ + --volume $PWD/control_logs:/tmp/control \ + golang:1 \ + go test ./... \ + -tags ts2019 \ + -failfast \ + -timeout 120m \ + -parallel 1 \ + -run "^TestACLDenyAllPort80$" + + - uses: actions/upload-artifact@v3 + if: always() && steps.changed-files.outputs.any_changed == 'true' + with: + name: logs + path: "control_logs/*.log" diff --git a/CHANGELOG.md b/CHANGELOG.md index 34b36fc0..ba8b44f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Changes +- Fix longstanding bug that would prevent "\*" from working properly in ACLs (issue [#699](https://github.com/juanfont/headscale/issues/699)) [#1279](https://github.com/juanfont/headscale/pull/1279) + ## 0.21.0 (2023-03-20) ### Changes diff --git a/acls.go b/acls.go index 70295ead..9233cb43 100644 --- a/acls.go +++ b/acls.go @@ -179,6 +179,8 @@ func generateACLPeerCacheMap(rules []tailcfg.FilterRule) map[string]map[string]s } } + log.Trace().Interface("ACL Cache Map", aclCachePeerMap).Msg("ACL Peer Cache Map generated") + return aclCachePeerMap } diff --git a/integration/acl_test.go b/integration/acl_test.go index ed32a642..d7043249 100644 --- a/integration/acl_test.go +++ b/integration/acl_test.go @@ -2,6 +2,7 @@ package integration import ( "fmt" + "strings" "testing" "github.com/juanfont/headscale" @@ -32,7 +33,7 @@ func aclScenario(t *testing.T, policy headscale.ACLPolicy) *Scenario { tsic.WithDockerWorkdir("/"), }, hsic.WithACLPolicy(&policy), - hsic.WithTestName("acldenyallping"), + hsic.WithTestName("acl"), ) assert.NoError(t, err) @@ -278,3 +279,163 @@ func TestACLAllowUser80Dst(t *testing.T) { err = scenario.Shutdown() assert.NoError(t, err) } + +func TestACLDenyAllPort80(t *testing.T) { + IntegrationSkip(t) + + scenario := aclScenario(t, + headscale.ACLPolicy{ + Groups: map[string][]string{ + "group:integration-acl-test": {"user1", "user2"}, + }, + ACLs: []headscale.ACL{ + { + Action: "accept", + Sources: []string{"group:integration-acl-test"}, + Destinations: []string{"*:22"}, + }, + }, + }, + ) + + allClients, err := scenario.ListTailscaleClients() + assert.NoError(t, err) + + allHostnames, err := scenario.ListTailscaleClientsFQDNs() + assert.NoError(t, err) + + for _, client := range allClients { + for _, hostname := range allHostnames { + // We will always be allowed to check _self_ so shortcircuit + // the test here. + if strings.Contains(hostname, client.Hostname()) { + continue + } + + url := fmt.Sprintf("http://%s/etc/hostname", hostname) + t.Logf("url from %s to %s", client.Hostname(), url) + + result, err := client.Curl(url) + assert.Empty(t, result) + assert.Error(t, err) + } + } + + err = scenario.Shutdown() + assert.NoError(t, err) +} + +// Test to confirm that we can use user:* from one user. +// This ACL will not allow user1 access its own machines. +// Reported: https://github.com/juanfont/headscale/issues/699 +func TestACLAllowUserDst(t *testing.T) { + IntegrationSkip(t) + + scenario := aclScenario(t, + headscale.ACLPolicy{ + ACLs: []headscale.ACL{ + { + Action: "accept", + Sources: []string{"user1"}, + Destinations: []string{"user2:*"}, + }, + }, + }, + ) + + user1Clients, err := scenario.ListTailscaleClients("user1") + assert.NoError(t, err) + + user2Clients, err := scenario.ListTailscaleClients("user2") + assert.NoError(t, err) + + // Test that user1 can visit all user2 + for _, client := range user1Clients { + for _, peer := range user2Clients { + fqdn, err := peer.FQDN() + assert.NoError(t, err) + + url := fmt.Sprintf("http://%s/etc/hostname", fqdn) + t.Logf("url from %s to %s", client.Hostname(), url) + + result, err := client.Curl(url) + assert.Len(t, result, 13) + assert.NoError(t, err) + } + } + + // Test that user2 _cannot_ visit user1 + for _, client := range user2Clients { + for _, peer := range user1Clients { + fqdn, err := peer.FQDN() + assert.NoError(t, err) + + url := fmt.Sprintf("http://%s/etc/hostname", fqdn) + t.Logf("url from %s to %s", client.Hostname(), url) + + result, err := client.Curl(url) + assert.Empty(t, result) + assert.Error(t, err) + } + } + + err = scenario.Shutdown() + assert.NoError(t, err) +} + +// Test to confirm that we can use *:* from one user +// Reported: https://github.com/juanfont/headscale/issues/699 +func TestACLAllowStarDst(t *testing.T) { + IntegrationSkip(t) + + scenario := aclScenario(t, + headscale.ACLPolicy{ + ACLs: []headscale.ACL{ + { + Action: "accept", + Sources: []string{"user1"}, + Destinations: []string{"*:*"}, + }, + }, + }, + ) + + user1Clients, err := scenario.ListTailscaleClients("user1") + assert.NoError(t, err) + + user2Clients, err := scenario.ListTailscaleClients("user2") + assert.NoError(t, err) + + // Test that user1 can visit all user2 + for _, client := range user1Clients { + for _, peer := range user2Clients { + fqdn, err := peer.FQDN() + assert.NoError(t, err) + + url := fmt.Sprintf("http://%s/etc/hostname", fqdn) + t.Logf("url from %s to %s", client.Hostname(), url) + + result, err := client.Curl(url) + assert.Len(t, result, 13) + assert.NoError(t, err) + } + } + + // Test that user2 _cannot_ visit user1 + for _, client := range user2Clients { + for _, peer := range user1Clients { + fqdn, err := peer.FQDN() + assert.NoError(t, err) + + url := fmt.Sprintf("http://%s/etc/hostname", fqdn) + t.Logf("url from %s to %s", client.Hostname(), url) + + result, err := client.Curl(url) + assert.Empty(t, result) + assert.Error(t, err) + } + } + + err = scenario.Shutdown() + assert.NoError(t, err) +} diff --git a/machine.go b/machine.go index fd6e2ede..146bfcf7 100644 --- a/machine.go +++ b/machine.go @@ -243,6 +243,12 @@ func filterMachinesByACL( for _, peerIP := range peerIPs { if dstMap, ok := aclPeerCacheMap[peerIP]; ok { + // match source and all destination + if _, dstOk := dstMap["*"]; dstOk { + peers[peer.ID] = peer + + continue + } // match return path for _, machineIP := range machineIPs { if _, dstOk := dstMap[machineIP]; dstOk { diff --git a/machine_test.go b/machine_test.go index 86eb1913..c25f32da 100644 --- a/machine_test.go +++ b/machine_test.go @@ -282,10 +282,10 @@ func (s *Suite) TestGetACLFilteredPeers(c *check.C) { peersOfAdminMachine := app.filterMachinesByACL(adminMachine, machines) c.Log(peersOfTestMachine) - c.Assert(len(peersOfTestMachine), check.Equals, 4) - c.Assert(peersOfTestMachine[0].Hostname, check.Equals, "testmachine4") - c.Assert(peersOfTestMachine[1].Hostname, check.Equals, "testmachine6") - c.Assert(peersOfTestMachine[3].Hostname, check.Equals, "testmachine10") + c.Assert(len(peersOfTestMachine), check.Equals, 9) + c.Assert(peersOfTestMachine[0].Hostname, check.Equals, "testmachine1") + c.Assert(peersOfTestMachine[1].Hostname, check.Equals, "testmachine3") + c.Assert(peersOfTestMachine[3].Hostname, check.Equals, "testmachine5") c.Log(peersOfAdminMachine) c.Assert(len(peersOfAdminMachine), check.Equals, 9) @@ -950,6 +950,96 @@ func Test_getFilteredByACLPeers(t *testing.T) { }, want: Machines{}, }, + { + // Investigating 699 + // Found some machines: [ts-head-8w6paa ts-unstable-lys2ib ts-head-upcrmb ts-unstable-rlwpvr] machine=ts-head-8w6paa + // ACL rules generated ACL=[{"DstPorts":[{"Bits":null,"IP":"*","Ports":{"First":0,"Last":65535}}],"SrcIPs":["fd7a:115c:a1e0::3","100.64.0.3","fd7a:115c:a1e0::4","100.64.0.4"]}] + // ACL Cache Map={"100.64.0.3":{"*":{}},"100.64.0.4":{"*":{}},"fd7a:115c:a1e0::3":{"*":{}},"fd7a:115c:a1e0::4":{"*":{}}} + name: "issue-699-broken-star", + args: args{ + machines: Machines{ // + { + ID: 1, + Hostname: "ts-head-upcrmb", + IPAddresses: MachineAddresses{ + netip.MustParseAddr("100.64.0.3"), + netip.MustParseAddr("fd7a:115c:a1e0::3"), + }, + User: User{Name: "user1"}, + }, + { + ID: 2, + Hostname: "ts-unstable-rlwpvr", + IPAddresses: MachineAddresses{ + netip.MustParseAddr("100.64.0.4"), + netip.MustParseAddr("fd7a:115c:a1e0::4"), + }, + User: User{Name: "user1"}, + }, + { + ID: 3, + Hostname: "ts-head-8w6paa", + IPAddresses: MachineAddresses{ + netip.MustParseAddr("100.64.0.1"), + netip.MustParseAddr("fd7a:115c:a1e0::1"), + }, + User: User{Name: "user2"}, + }, + { + ID: 4, + Hostname: "ts-unstable-lys2ib", + IPAddresses: MachineAddresses{ + netip.MustParseAddr("100.64.0.2"), + netip.MustParseAddr("fd7a:115c:a1e0::2"), + }, + User: User{Name: "user2"}, + }, + }, + rules: []tailcfg.FilterRule{ // list of all ACLRules registered + { + DstPorts: []tailcfg.NetPortRange{ + { + IP: "*", + Ports: tailcfg.PortRange{First: 0, Last: 65535}, + }, + }, + SrcIPs: []string{ + "fd7a:115c:a1e0::3", "100.64.0.3", + "fd7a:115c:a1e0::4", "100.64.0.4", + }, + }, + }, + machine: &Machine{ // current machine + ID: 3, + Hostname: "ts-head-8w6paa", + IPAddresses: MachineAddresses{ + netip.MustParseAddr("100.64.0.1"), + netip.MustParseAddr("fd7a:115c:a1e0::1"), + }, + User: User{Name: "user2"}, + }, + }, + want: Machines{ + { + ID: 1, + Hostname: "ts-head-upcrmb", + IPAddresses: MachineAddresses{ + netip.MustParseAddr("100.64.0.3"), + netip.MustParseAddr("fd7a:115c:a1e0::3"), + }, + User: User{Name: "user1"}, + }, + { + ID: 2, + Hostname: "ts-unstable-rlwpvr", + IPAddresses: MachineAddresses{ + netip.MustParseAddr("100.64.0.4"), + netip.MustParseAddr("fd7a:115c:a1e0::4"), + }, + User: User{Name: "user1"}, + }, + }, + }, } var lock sync.RWMutex for _, tt := range tests {