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 <kristoffer@tailscale.com>

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)
 

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,
+		)
 	}
 }
 

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,
 				)

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)
+			}
+		})
+	}
+}

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)
 	}

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) {

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 {