From 9c425a1c08047393323bb2fb71d9071d5b285f28 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Thu, 8 Jun 2023 19:50:59 +0200 Subject: [PATCH] Finish SSH This commit allows SSH rules to be assigned to each relevant not and by doing that allow SSH to be rejected, completing the initial SSH support. This commit enables SSH by default and removes the experimental flag. Signed-off-by: Kristoffer Dalby --- CHANGELOG.md | 3 +- hscontrol/db/acls_test.go | 83 +------------ hscontrol/policy/acls.go | 38 ++++-- hscontrol/policy/acls_test.go | 182 ++++++++++++++++++++++++++++ hscontrol/policy/matcher/matcher.go | 15 ++- hscontrol/types/machine.go | 10 ++ integration/ssh_test.go | 40 +++--- 7 files changed, 254 insertions(+), 117 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1b1db050..ac8f323b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,8 @@ ### Changes -- Adjust the template for the OIDC callback login page [#1484](https://github.com/juanfont/headscale/pull/1484) +- Make the OIDC callback page better [#1484](https://github.com/juanfont/headscale/pull/1484) +- SSH is no longer experimental [#1487](https://github.com/juanfont/headscale/pull/1487) ## 0.22.3 (2023-05-12) diff --git a/hscontrol/db/acls_test.go b/hscontrol/db/acls_test.go index e7d09a5f..fc822a79 100644 --- a/hscontrol/db/acls_test.go +++ b/hscontrol/db/acls_test.go @@ -9,8 +9,6 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" "github.com/stretchr/testify/assert" - "gopkg.in/check.v1" - "tailscale.com/envknob" "tailscale.com/tailcfg" ) @@ -18,82 +16,6 @@ import ( // Convert these tests to being non-database dependent and table driven. They are // very verbose, and dont really need the database. -func (s *Suite) TestSshRules(c *check.C) { - envknob.Setenv("HEADSCALE_EXPERIMENTAL_FEATURE_SSH", "1") - - user, err := db.CreateUser("user1") - c.Assert(err, check.IsNil) - - pak, err := db.CreatePreAuthKey(user.Name, false, false, nil, nil) - c.Assert(err, check.IsNil) - - _, err = db.GetMachine("user1", "testmachine") - c.Assert(err, check.NotNil) - hostInfo := tailcfg.Hostinfo{ - OS: "centos", - Hostname: "testmachine", - RequestTags: []string{"tag:test"}, - } - - machine := types.Machine{ - ID: 0, - MachineKey: "foo", - NodeKey: "bar", - DiscoKey: "faa", - Hostname: "testmachine", - IPAddresses: types.MachineAddresses{netip.MustParseAddr("100.64.0.1")}, - UserID: user.ID, - RegisterMethod: util.RegisterMethodAuthKey, - AuthKeyID: uint(pak.ID), - HostInfo: types.HostInfo(hostInfo), - } - err = db.MachineSave(&machine) - c.Assert(err, check.IsNil) - - aclPolicy := &policy.ACLPolicy{ - Groups: policy.Groups{ - "group:test": []string{"user1"}, - }, - Hosts: policy.Hosts{ - "client": netip.PrefixFrom(netip.MustParseAddr("100.64.99.42"), 32), - }, - ACLs: []policy.ACL{ - { - Action: "accept", - Sources: []string{"*"}, - Destinations: []string{"*:*"}, - }, - }, - SSHs: []policy.SSH{ - { - Action: "accept", - Sources: []string{"group:test"}, - Destinations: []string{"client"}, - Users: []string{"autogroup:nonroot"}, - }, - { - Action: "accept", - Sources: []string{"*"}, - Destinations: []string{"client"}, - Users: []string{"autogroup:nonroot"}, - }, - }, - } - - _, sshPolicy, err := policy.GenerateFilterRules(aclPolicy, &machine, types.Machines{}, false) - - c.Assert(err, check.IsNil) - c.Assert(sshPolicy, check.NotNil) - c.Assert(sshPolicy.Rules, check.HasLen, 2) - c.Assert(sshPolicy.Rules[0].SSHUsers, check.HasLen, 1) - c.Assert(sshPolicy.Rules[0].Principals, check.HasLen, 1) - c.Assert(sshPolicy.Rules[0].Principals[0].UserLogin, check.Matches, "user1") - - c.Assert(sshPolicy.Rules[1].SSHUsers, check.HasLen, 1) - c.Assert(sshPolicy.Rules[1].Principals, check.HasLen, 1) - c.Assert(sshPolicy.Rules[1].Principals[0].NodeIP, check.Matches, "*") -} - // this test should validate that we can expand a group in a TagOWner section and // match properly the IP's of the related hosts. The owner is valid and the tag is also valid. // the tag is matched in the Sources section. @@ -376,7 +298,10 @@ func TestValidExpandTagOwnersInDestinations(t *testing.T) { } if diff := cmp.Diff(want, got); diff != "" { - t.Errorf("TestValidExpandTagOwnersInDestinations() unexpected result (-want +got):\n%s", diff) + t.Errorf( + "TestValidExpandTagOwnersInDestinations() unexpected result (-want +got):\n%s", + diff, + ) } } diff --git a/hscontrol/policy/acls.go b/hscontrol/policy/acls.go index deaf8a62..d3ad12fd 100644 --- a/hscontrol/policy/acls.go +++ b/hscontrol/policy/acls.go @@ -136,7 +136,7 @@ func GenerateFilterRules( log.Trace().Interface("ACL", rules).Msg("ACL rules generated") var sshPolicy *tailcfg.SSHPolicy - sshRules, err := generateSSHRules(policy, append(peers, *machine), stripEmailDomain) + sshRules, err := policy.generateSSHRules(machine, peers, stripEmailDomain) if err != nil { return []tailcfg.FilterRule{}, &tailcfg.SSHPolicy{}, err } @@ -215,17 +215,13 @@ func (pol *ACLPolicy) generateFilterRules( return rules, nil } -func generateSSHRules( - policy *ACLPolicy, - machines types.Machines, +func (pol *ACLPolicy) generateSSHRules( + machine *types.Machine, + peers types.Machines, stripEmailDomain bool, ) ([]*tailcfg.SSHRule, error) { rules := []*tailcfg.SSHRule{} - if policy == nil { - return nil, ErrEmptyPolicy - } - acceptAction := tailcfg.SSHAction{ Message: "", Reject: false, @@ -246,7 +242,25 @@ func generateSSHRules( AllowLocalPortForwarding: false, } - for index, sshACL := range policy.SSHs { + for index, sshACL := range pol.SSHs { + var dest netipx.IPSetBuilder + for _, src := range sshACL.Destinations { + expanded, err := pol.ExpandAlias(append(peers, *machine), src, stripEmailDomain) + if err != nil { + return nil, err + } + dest.AddSet(expanded) + } + + destSet, err := dest.IPSet() + if err != nil { + return nil, err + } + + if !machine.IPAddresses.InIPSet(destSet) { + continue + } + action := rejectAction switch sshACL.Action { case "accept": @@ -273,7 +287,7 @@ func generateSSHRules( Any: true, }) } else if isGroup(rawSrc) { - users, err := policy.getUsersInGroup(rawSrc, stripEmailDomain) + users, err := pol.getUsersInGroup(rawSrc, stripEmailDomain) if err != nil { log.Error(). Msgf("Error parsing SSH %d, Source %d", index, innerIndex) @@ -287,8 +301,8 @@ func generateSSHRules( }) } } else { - expandedSrcs, err := policy.ExpandAlias( - machines, + expandedSrcs, err := pol.ExpandAlias( + peers, rawSrc, stripEmailDomain, ) diff --git a/hscontrol/policy/acls_test.go b/hscontrol/policy/acls_test.go index e9352d2a..d7f5932a 100644 --- a/hscontrol/policy/acls_test.go +++ b/hscontrol/policy/acls_test.go @@ -10,6 +10,7 @@ import ( "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" "github.com/rs/zerolog/log" + "github.com/stretchr/testify/assert" "go4.org/netipx" "gopkg.in/check.v1" "tailscale.com/tailcfg" @@ -2413,3 +2414,184 @@ func Test_getFilteredByACLPeers(t *testing.T) { }) } } + +func TestSSHRules(t *testing.T) { + tests := []struct { + name string + machine types.Machine + peers types.Machines + pol ACLPolicy + want []*tailcfg.SSHRule + }{ + { + name: "peers-can-connect", + machine: types.Machine{ + Hostname: "testmachine", + IPAddresses: types.MachineAddresses{netip.MustParseAddr("100.64.99.42")}, + UserID: 0, + User: types.User{ + Name: "user1", + }, + }, + peers: types.Machines{ + types.Machine{ + Hostname: "testmachine2", + IPAddresses: types.MachineAddresses{netip.MustParseAddr("100.64.0.1")}, + UserID: 0, + User: types.User{ + Name: "user1", + }, + }, + }, + pol: ACLPolicy{ + Groups: Groups{ + "group:test": []string{"user1"}, + }, + Hosts: Hosts{ + "client": netip.PrefixFrom(netip.MustParseAddr("100.64.99.42"), 32), + }, + ACLs: []ACL{ + { + Action: "accept", + Sources: []string{"*"}, + Destinations: []string{"*:*"}, + }, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: []string{"group:test"}, + Destinations: []string{"client"}, + Users: []string{"autogroup:nonroot"}, + }, + { + Action: "accept", + Sources: []string{"*"}, + Destinations: []string{"client"}, + Users: []string{"autogroup:nonroot"}, + }, + { + Action: "accept", + Sources: []string{"group:test"}, + Destinations: []string{"100.64.99.42"}, + Users: []string{"autogroup:nonroot"}, + }, + { + Action: "accept", + Sources: []string{"*"}, + Destinations: []string{"100.64.99.42"}, + Users: []string{"autogroup:nonroot"}, + }, + }, + }, + want: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + { + UserLogin: "user1", + }, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{Accept: true, AllowLocalPortForwarding: true}, + }, + { + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Principals: []*tailcfg.SSHPrincipal{ + { + Any: true, + }, + }, + Action: &tailcfg.SSHAction{Accept: true, AllowLocalPortForwarding: true}, + }, + { + Principals: []*tailcfg.SSHPrincipal{ + { + UserLogin: "user1", + }, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{Accept: true, AllowLocalPortForwarding: true}, + }, + { + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Principals: []*tailcfg.SSHPrincipal{ + { + Any: true, + }, + }, + Action: &tailcfg.SSHAction{Accept: true, AllowLocalPortForwarding: true}, + }, + }, + }, + { + name: "peers-cannot-connect", + machine: types.Machine{ + Hostname: "testmachine", + IPAddresses: types.MachineAddresses{netip.MustParseAddr("100.64.0.1")}, + UserID: 0, + User: types.User{ + Name: "user1", + }, + }, + peers: types.Machines{ + types.Machine{ + Hostname: "testmachine2", + IPAddresses: types.MachineAddresses{netip.MustParseAddr("100.64.99.42")}, + UserID: 0, + User: types.User{ + Name: "user1", + }, + }, + }, + pol: ACLPolicy{ + Groups: Groups{ + "group:test": []string{"user1"}, + }, + Hosts: Hosts{ + "client": netip.PrefixFrom(netip.MustParseAddr("100.64.99.42"), 32), + }, + ACLs: []ACL{ + { + Action: "accept", + Sources: []string{"*"}, + Destinations: []string{"*:*"}, + }, + }, + SSHs: []SSH{ + { + Action: "accept", + Sources: []string{"group:test"}, + Destinations: []string{"100.64.99.42"}, + Users: []string{"autogroup:nonroot"}, + }, + { + Action: "accept", + Sources: []string{"*"}, + Destinations: []string{"100.64.99.42"}, + Users: []string{"autogroup:nonroot"}, + }, + }, + }, + want: []*tailcfg.SSHRule{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := tt.pol.generateSSHRules(&tt.machine, tt.peers, false) + assert.NoError(t, err) + + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("TestSSHRules() unexpected result (-want +got):\n%s", diff) + } + }) + } +} diff --git a/hscontrol/policy/matcher/matcher.go b/hscontrol/policy/matcher/matcher.go index 8458339c..1905dad2 100644 --- a/hscontrol/policy/matcher/matcher.go +++ b/hscontrol/policy/matcher/matcher.go @@ -14,17 +14,26 @@ type Match struct { } func MatchFromFilterRule(rule tailcfg.FilterRule) Match { + dests := []string{} + for _, dest := range rule.DstPorts { + dests = append(dests, dest.IP) + } + + return MatchFromStrings(rule.SrcIPs, dests) +} + +func MatchFromStrings(sources, destinations []string) Match { srcs := new(netipx.IPSetBuilder) dests := new(netipx.IPSetBuilder) - for _, srcIP := range rule.SrcIPs { + for _, srcIP := range sources { set, _ := util.ParseIPSet(srcIP, nil) srcs.AddSet(set) } - for _, dest := range rule.DstPorts { - set, _ := util.ParseIPSet(dest.IP, nil) + for _, dest := range destinations { + set, _ := util.ParseIPSet(dest, nil) dests.AddSet(set) } diff --git a/hscontrol/types/machine.go b/hscontrol/types/machine.go index 9a576ad7..9fedae73 100644 --- a/hscontrol/types/machine.go +++ b/hscontrol/types/machine.go @@ -92,6 +92,16 @@ func (ma MachineAddresses) Prefixes() []netip.Prefix { return addrs } +func (ma MachineAddresses) InIPSet(set *netipx.IPSet) bool { + for _, machineAddr := range ma { + if set.Contains(machineAddr) { + return true + } + } + + return false +} + // AppendToIPSet adds the individual ips in MachineAddresses to a // given netipx.IPSetBuilder. func (ma MachineAddresses) AppendToIPSet(build *netipx.IPSetBuilder) { diff --git a/integration/ssh_test.go b/integration/ssh_test.go index 006ac0cb..c8963b19 100644 --- a/integration/ssh_test.go +++ b/integration/ssh_test.go @@ -421,29 +421,25 @@ func TestSSUserOnlyIsolation(t *testing.T) { t.Errorf("failed to get FQDNs: %s", err) } - // TODO(kradalby,evenh): ACLs do currently not cover reject - // cases properly, and currently will accept all incomming connections - // as long as a rule is present. + for _, client := range ssh1Clients { + for _, peer := range ssh2Clients { + if client.Hostname() == peer.Hostname() { + continue + } - // for _, client := range ssh1Clients { - // for _, peer := range ssh2Clients { - // if client.Hostname() == peer.Hostname() { - // continue - // } - // - // assertSSHPermissionDenied(t, client, peer) - // } - // } - // - // for _, client := range ssh2Clients { - // for _, peer := range ssh1Clients { - // if client.Hostname() == peer.Hostname() { - // continue - // } - // - // assertSSHPermissionDenied(t, client, peer) - // } - // } + assertSSHPermissionDenied(t, client, peer) + } + } + + for _, client := range ssh2Clients { + for _, peer := range ssh1Clients { + if client.Hostname() == peer.Hostname() { + continue + } + + assertSSHPermissionDenied(t, client, peer) + } + } for _, client := range ssh1Clients { for _, peer := range ssh1Clients {