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 {