Fix issue where ACL * would filter out returning connections (#1279)
This commit is contained in:
Kristoffer Dalby
GitHub
parent
56a7b1e349
commit
c7b459b615
8 changed files with 437 additions and 5 deletions
57
.github/workflows/test-integration-v2-TestACLAllowStarDst.yaml
vendored
Normal file
57
.github/workflows/test-integration-v2-TestACLAllowStarDst.yaml
vendored
Normal file
|
|
@ -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"
|
||||
57
.github/workflows/test-integration-v2-TestACLAllowUserDst.yaml
vendored
Normal file
57
.github/workflows/test-integration-v2-TestACLAllowUserDst.yaml
vendored
Normal file
|
|
@ -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"
|
||||
57
.github/workflows/test-integration-v2-TestACLDenyAllPort80.yaml
vendored
Normal file
57
.github/workflows/test-integration-v2-TestACLDenyAllPort80.yaml
vendored
Normal file
|
|
@ -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"
|
||||
|
|
@ -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
|
||||
|
|
|
|||
2
acls.go
2
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
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
Loading…
Reference in a new issue