diff --git a/CHANGELOG.md b/CHANGELOG.md index d4589652..48d11080 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,6 +62,20 @@ new policy code passes all of our tests. `@` should be appended at the end. For example, if your user is `john`, it must be written as `john@` in the policy. +**SSH** + +The SSH policy has been reworked to be more consistent with the rest of the +policy. In addition, several inconsistencies between our implementation and +Tailscale's upstream has been closed and this might be a breaking change for +some users. Please refer to the +[upstream documentation](https://tailscale.com/kb/1337/acl-syntax#tailscale-ssh) +for more information on which types are allowed in `src`, `dst` and `users`. + +There is one large inconsistency left, we allow `*` as a destination as we +currently do not support `autogroup:self`, `autogroup:member` and +`autogroup:tagged`. The support for `*` will be removed when we have support for +the autogroups. + **Current state** The new policy is passing all tests, both integration and unit tests. This does @@ -70,8 +84,6 @@ working in v1 and not tested might be broken in v2 (and vice versa). **We do need help testing this code** - - #### Other breaking changes - Disallow `server_url` and `base_domain` to be equal diff --git a/flake.lock b/flake.lock index 11421972..5011e131 100644 --- a/flake.lock +++ b/flake.lock @@ -20,11 +20,11 @@ }, "nixpkgs": { "locked": { - "lastModified": 1746152631, - "narHash": "sha256-zBuvmL6+CUsk2J8GINpyy8Hs1Zp4PP6iBWSmZ4SCQ/s=", + "lastModified": 1746300365, + "narHash": "sha256-thYTdWqCRipwPRxWiTiH1vusLuAy0okjOyzRx4hLWh4=", "owner": "NixOS", "repo": "nixpkgs", - "rev": "032bc6539bd5f14e9d0c51bd79cfe9a055b094c3", + "rev": "f21e4546e3ede7ae34d12a84602a22246b31f7e0", "type": "github" }, "original": { diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index 671ed829..5b3814a2 100644 --- a/hscontrol/policy/policy_test.go +++ b/hscontrol/policy/policy_test.go @@ -4,6 +4,7 @@ import ( "fmt" "net/netip" "testing" + "time" "github.com/juanfont/headscale/hscontrol/policy/matcher" @@ -1540,3 +1541,408 @@ func TestFilterNodesByACL(t *testing.T) { }) } } + +func TestSSHPolicyRules(t *testing.T) { + users := []types.User{ + {Name: "user1", Model: gorm.Model{ID: 1}}, + {Name: "user2", Model: gorm.Model{ID: 2}}, + {Name: "user3", Model: gorm.Model{ID: 3}}, + } + + // Create standard node setups used across tests + nodeUser1 := types.Node{ + Hostname: "user1-device", + IPv4: ap("100.64.0.1"), + UserID: 1, + User: users[0], + } + nodeUser2 := types.Node{ + Hostname: "user2-device", + IPv4: ap("100.64.0.2"), + UserID: 2, + User: users[1], + } + taggedServer := types.Node{ + Hostname: "tagged-server", + IPv4: ap("100.64.0.3"), + UserID: 3, + User: users[2], + ForcedTags: []string{"tag:server"}, + } + taggedClient := types.Node{ + Hostname: "tagged-client", + IPv4: ap("100.64.0.4"), + UserID: 2, + User: users[1], + ForcedTags: []string{"tag:client"}, + } + + tests := []struct { + name string + targetNode types.Node + peers types.Nodes + policy string + wantSSH *tailcfg.SSHPolicy + expectErr bool + errorMessage string + + // There are some tests that will not pass on V1 since we do not + // have the same kind of error handling as V2, so we skip them. + skipV1 bool + }{ + { + name: "group-to-user", + targetNode: nodeUser1, + peers: types.Nodes{&nodeUser2}, + policy: `{ + "groups": { + "group:admins": ["user2@"] + }, + "ssh": [ + { + "action": "accept", + "src": ["group:admins"], + "dst": ["user1@"], + "users": ["autogroup:nonroot"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.2"}, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + }, + }, + }}, + + // It looks like the group implementation in v1 is broken, so + // we skip this test for v1 and not let it hold up v2 replacing it. + skipV1: true, + }, + { + name: "group-to-tag", + targetNode: taggedServer, + peers: types.Nodes{&nodeUser1, &nodeUser2}, + policy: `{ + "groups": { + "group:users": ["user1@", "user2@"] + }, + "ssh": [ + { + "action": "accept", + "src": ["group:users"], + "dst": ["tag:server"], + "users": ["autogroup:nonroot"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.1"}, + {NodeIP: "100.64.0.2"}, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + }, + }, + }}, + + // It looks like the group implementation in v1 is broken, so + // we skip this test for v1 and not let it hold up v2 replacing it. + skipV1: true, + }, + { + name: "tag-to-user", + targetNode: nodeUser1, + peers: types.Nodes{&taggedClient}, + policy: `{ + "ssh": [ + { + "action": "accept", + "src": ["tag:client"], + "dst": ["user1@"], + "users": ["autogroup:nonroot"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.4"}, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + }, + }, + }}, + }, + { + name: "tag-to-tag", + targetNode: taggedServer, + peers: types.Nodes{&taggedClient}, + policy: `{ + "ssh": [ + { + "action": "accept", + "src": ["tag:client"], + "dst": ["tag:server"], + "users": ["autogroup:nonroot"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.4"}, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + }, + }, + }}, + }, + { + name: "group-to-wildcard", + targetNode: nodeUser1, + peers: types.Nodes{&nodeUser2, &taggedClient}, + policy: `{ + "groups": { + "group:admins": ["user2@"] + }, + "ssh": [ + { + "action": "accept", + "src": ["group:admins"], + "dst": ["*"], + "users": ["autogroup:nonroot"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.2"}, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + }, + }, + }}, + + // It looks like the group implementation in v1 is broken, so + // we skip this test for v1 and not let it hold up v2 replacing it. + skipV1: true, + }, + { + name: "invalid-source-user-not-allowed", + targetNode: nodeUser1, + peers: types.Nodes{&nodeUser2}, + policy: `{ + "ssh": [ + { + "action": "accept", + "src": ["user2@"], + "dst": ["user1@"], + "users": ["autogroup:nonroot"] + } + ] + }`, + expectErr: true, + errorMessage: "not supported", + skipV1: true, + }, + { + name: "check-period-specified", + targetNode: nodeUser1, + peers: types.Nodes{&taggedClient}, + policy: `{ + "ssh": [ + { + "action": "check", + "checkPeriod": "24h", + "src": ["tag:client"], + "dst": ["user1@"], + "users": ["autogroup:nonroot"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.4"}, + }, + SSHUsers: map[string]string{ + "autogroup:nonroot": "=", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + SessionDuration: 24 * time.Hour, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + }, + }, + }}, + }, + { + name: "no-matching-rules", + targetNode: nodeUser2, + peers: types.Nodes{&nodeUser1}, + policy: `{ + "ssh": [ + { + "action": "accept", + "src": ["tag:client"], + "dst": ["user1@"], + "users": ["autogroup:nonroot"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: nil}, + }, + { + name: "invalid-action", + targetNode: nodeUser1, + peers: types.Nodes{&nodeUser2}, + policy: `{ + "ssh": [ + { + "action": "invalid", + "src": ["group:admins"], + "dst": ["user1@"], + "users": ["autogroup:nonroot"] + } + ] + }`, + expectErr: true, + errorMessage: `SSH action "invalid" is not valid, must be accept or check`, + skipV1: true, + }, + { + name: "invalid-check-period", + targetNode: nodeUser1, + peers: types.Nodes{&nodeUser2}, + policy: `{ + "ssh": [ + { + "action": "check", + "checkPeriod": "invalid", + "src": ["group:admins"], + "dst": ["user1@"], + "users": ["autogroup:nonroot"] + } + ] + }`, + expectErr: true, + errorMessage: "not a valid duration string", + skipV1: true, + }, + { + name: "multiple-ssh-users-with-autogroup", + targetNode: nodeUser1, + peers: types.Nodes{&taggedClient}, + policy: `{ + "ssh": [ + { + "action": "accept", + "src": ["tag:client"], + "dst": ["user1@"], + "users": ["alice", "bob"] + } + ] + }`, + wantSSH: &tailcfg.SSHPolicy{Rules: []*tailcfg.SSHRule{ + { + Principals: []*tailcfg.SSHPrincipal{ + {NodeIP: "100.64.0.4"}, + }, + SSHUsers: map[string]string{ + "alice": "=", + "bob": "=", + }, + Action: &tailcfg.SSHAction{ + Accept: true, + AllowAgentForwarding: true, + AllowLocalPortForwarding: true, + }, + }, + }}, + }, + { + name: "unsupported-autogroup", + targetNode: nodeUser1, + peers: types.Nodes{&taggedClient}, + policy: `{ + "ssh": [ + { + "action": "accept", + "src": ["tag:client"], + "dst": ["user1@"], + "users": ["autogroup:invalid"] + } + ] + }`, + expectErr: true, + errorMessage: "autogroup \"autogroup:invalid\" is not supported", + skipV1: true, + }, + } + + for _, tt := range tests { + for idx, pmf := range PolicyManagerFuncsForTest([]byte(tt.policy)) { + version := idx + 1 + t.Run(fmt.Sprintf("%s-v%d", tt.name, version), func(t *testing.T) { + if version == 1 && tt.skipV1 { + t.Skip() + } + + var pm PolicyManager + var err error + pm, err = pmf(users, append(tt.peers, &tt.targetNode)) + + if tt.expectErr { + require.Error(t, err) + require.Contains(t, err.Error(), tt.errorMessage) + return + } + + require.NoError(t, err) + + got, err := pm.SSHPolicy(&tt.targetNode) + require.NoError(t, err) + + if diff := cmp.Diff(tt.wantSSH, got); diff != "" { + t.Errorf("SSHPolicy() unexpected result (-want +got):\n%s", diff) + } + }) + } + } +} diff --git a/hscontrol/policy/v1/acls_test.go b/hscontrol/policy/v1/acls_test.go index 03dcd431..f2871064 100644 --- a/hscontrol/policy/v1/acls_test.go +++ b/hscontrol/policy/v1/acls_test.go @@ -2159,200 +2159,6 @@ func Test_getTags(t *testing.T) { } } -func TestSSHRules(t *testing.T) { - users := []types.User{ - { - Name: "user1", - }, - } - tests := []struct { - name string - node types.Node - peers types.Nodes - pol ACLPolicy - want *tailcfg.SSHPolicy - }{ - { - name: "peers-can-connect", - node: types.Node{ - Hostname: "testnodes", - IPv4: iap("100.64.99.42"), - UserID: 0, - User: users[0], - }, - peers: types.Nodes{ - &types.Node{ - Hostname: "testnodes2", - IPv4: iap("100.64.0.1"), - UserID: 0, - User: users[0], - }, - }, - 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.SSHPolicy{Rules: []*tailcfg.SSHRule{ - { - Principals: []*tailcfg.SSHPrincipal{ - { - UserLogin: "user1", - }, - }, - SSHUsers: map[string]string{ - "autogroup:nonroot": "=", - }, - Action: &tailcfg.SSHAction{ - Accept: true, - AllowAgentForwarding: true, - AllowLocalPortForwarding: true, - }, - }, - { - SSHUsers: map[string]string{ - "autogroup:nonroot": "=", - }, - Principals: []*tailcfg.SSHPrincipal{ - { - Any: true, - }, - }, - Action: &tailcfg.SSHAction{ - Accept: true, - AllowAgentForwarding: true, - AllowLocalPortForwarding: true, - }, - }, - { - Principals: []*tailcfg.SSHPrincipal{ - { - UserLogin: "user1", - }, - }, - SSHUsers: map[string]string{ - "autogroup:nonroot": "=", - }, - Action: &tailcfg.SSHAction{ - Accept: true, - AllowAgentForwarding: true, - AllowLocalPortForwarding: true, - }, - }, - { - SSHUsers: map[string]string{ - "autogroup:nonroot": "=", - }, - Principals: []*tailcfg.SSHPrincipal{ - { - Any: true, - }, - }, - Action: &tailcfg.SSHAction{ - Accept: true, - AllowAgentForwarding: true, - AllowLocalPortForwarding: true, - }, - }, - }}, - }, - { - name: "peers-cannot-connect", - node: types.Node{ - Hostname: "testnodes", - IPv4: iap("100.64.0.1"), - UserID: 0, - User: users[0], - }, - peers: types.Nodes{ - &types.Node{ - Hostname: "testnodes2", - IPv4: iap("100.64.99.42"), - UserID: 0, - User: users[0], - }, - }, - 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.SSHPolicy{Rules: nil}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := tt.pol.CompileSSHPolicy(&tt.node, users, tt.peers) - require.NoError(t, err) - - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("TestSSHRules() unexpected result (-want +got):\n%s", diff) - } - }) - } -} - func TestParseDestination(t *testing.T) { tests := []struct { dest string diff --git a/hscontrol/policy/v1/acls_types.go b/hscontrol/policy/v1/acls_types.go index c44d8df7..c7c59328 100644 --- a/hscontrol/policy/v1/acls_types.go +++ b/hscontrol/policy/v1/acls_types.go @@ -90,7 +90,7 @@ func (hosts *Hosts) UnmarshalJSON(data []byte) error { // IsZero is perhaps a bit naive here. func (pol ACLPolicy) IsZero() bool { - if len(pol.Groups) == 0 && len(pol.Hosts) == 0 && len(pol.ACLs) == 0 { + if len(pol.Groups) == 0 && len(pol.Hosts) == 0 && len(pol.ACLs) == 0 && len(pol.SSHs) == 0 { return true } diff --git a/hscontrol/policy/v2/filter.go b/hscontrol/policy/v2/filter.go index b94620a3..6bbc8030 100644 --- a/hscontrol/policy/v2/filter.go +++ b/hscontrol/policy/v2/filter.go @@ -130,7 +130,7 @@ func (pol *Policy) compileSSHPolicy( case "accept": action = sshAction(true, 0) case "check": - action = sshAction(true, rule.CheckPeriod) + action = sshAction(true, time.Duration(rule.CheckPeriod)) default: return nil, fmt.Errorf("parsing SSH policy, unknown action %q, index: %d: %w", rule.Action, index, err) } diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index 2ee998b6..511e19bb 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -6,12 +6,12 @@ import ( "fmt" "net/netip" "strings" - "time" "slices" "github.com/juanfont/headscale/hscontrol/types" "github.com/juanfont/headscale/hscontrol/util" + "github.com/prometheus/common/model" "github.com/tailscale/hujson" "go4.org/netipx" "tailscale.com/net/tsaddr" @@ -383,6 +383,12 @@ type AutoGroup string const ( AutoGroupInternet AutoGroup = "autogroup:internet" + AutoGroupNonRoot AutoGroup = "autogroup:nonroot" + + // These are not yet implemented. + AutoGroupSelf AutoGroup = "autogroup:self" + AutoGroupMember AutoGroup = "autogroup:member" + AutoGroupTagged AutoGroup = "autogroup:tagged" ) var autogroups = []AutoGroup{AutoGroupInternet} @@ -915,6 +921,99 @@ type Policy struct { SSHs []SSH `json:"ssh"` } +var ( + autogroupForSrc = []AutoGroup{} + autogroupForDst = []AutoGroup{AutoGroupInternet} + autogroupForSSHSrc = []AutoGroup{} + autogroupForSSHDst = []AutoGroup{} + autogroupForSSHUser = []AutoGroup{AutoGroupNonRoot} + autogroupNotSupported = []AutoGroup{AutoGroupSelf, AutoGroupMember, AutoGroupTagged} +) + +func validateAutogroupSupported(ag *AutoGroup) error { + if ag == nil { + return nil + } + + if slices.Contains(autogroupNotSupported, *ag) { + return fmt.Errorf("autogroup %q is not supported in headscale", *ag) + } + + return nil +} + +func validateAutogroupForSrc(src *AutoGroup) error { + if src == nil { + return nil + } + + if src.Is(AutoGroupInternet) { + return fmt.Errorf(`"autogroup:internet" used in source, it can only be used in ACL destinations`) + } + + if !slices.Contains(autogroupForSrc, *src) { + return fmt.Errorf("autogroup %q is not supported for ACL sources, can be %v", *src, autogroupForSrc) + } + + return nil +} + +func validateAutogroupForDst(dst *AutoGroup) error { + if dst == nil { + return nil + } + + if !slices.Contains(autogroupForDst, *dst) { + return fmt.Errorf("autogroup %q is not supported for ACL destinations, can be %v", *dst, autogroupForDst) + } + + return nil +} + +func validateAutogroupForSSHSrc(src *AutoGroup) error { + if src == nil { + return nil + } + + if src.Is(AutoGroupInternet) { + return fmt.Errorf(`"autogroup:internet" used in SSH source, it can only be used in ACL destinations`) + } + + if !slices.Contains(autogroupForSSHSrc, *src) { + return fmt.Errorf("autogroup %q is not supported for SSH sources, can be %v", *src, autogroupForSSHSrc) + } + + return nil +} + +func validateAutogroupForSSHDst(dst *AutoGroup) error { + if dst == nil { + return nil + } + + if dst.Is(AutoGroupInternet) { + return fmt.Errorf(`"autogroup:internet" used in SSH destination, it can only be used in ACL destinations`) + } + + if !slices.Contains(autogroupForSSHDst, *dst) { + return fmt.Errorf("autogroup %q is not supported for SSH sources, can be %v", *dst, autogroupForSSHDst) + } + + return nil +} + +func validateAutogroupForSSHUser(user *AutoGroup) error { + if user == nil { + return nil + } + + if !slices.Contains(autogroupForSSHUser, *user) { + return fmt.Errorf("autogroup %q is not supported for SSH user, can be %v", *user, autogroupForSSHUser) + } + + return nil +} + // validate reports if there are any errors in a policy after // the unmarshaling process. // It runs through all rules and checks if there are any inconsistencies @@ -938,20 +1037,70 @@ func (p *Policy) validate() error { } case *AutoGroup: ag := src.(*AutoGroup) - if ag.Is(AutoGroupInternet) { - errs = append(errs, fmt.Errorf(`"autogroup:internet" used in source, it can only be used in ACL destinations`)) + + if err := validateAutogroupSupported(ag); err != nil { + errs = append(errs, err) + continue + } + + if err := validateAutogroupForSrc(ag); err != nil { + errs = append(errs, err) + continue + } + } + } + + for _, dst := range acl.Destinations { + switch dst.Alias.(type) { + case *Host: + h := dst.Alias.(*Host) + if !p.Hosts.exist(*h) { + errs = append(errs, fmt.Errorf(`Host %q is not defined in the Policy, please define or remove the reference to it`, *h)) + } + case *AutoGroup: + ag := dst.Alias.(*AutoGroup) + + if err := validateAutogroupSupported(ag); err != nil { + errs = append(errs, err) + continue + } + + if err := validateAutogroupForDst(ag); err != nil { + errs = append(errs, err) + continue } } } } for _, ssh := range p.SSHs { + if ssh.Action != "accept" && ssh.Action != "check" { + errs = append(errs, fmt.Errorf("SSH action %q is not valid, must be accept or check", ssh.Action)) + } + + for _, user := range ssh.Users { + if strings.HasPrefix(string(user), "autogroup:") { + maybeAuto := AutoGroup(user) + if err := validateAutogroupForSSHUser(&maybeAuto); err != nil { + errs = append(errs, err) + continue + } + } + } + for _, src := range ssh.Sources { switch src.(type) { case *AutoGroup: ag := src.(*AutoGroup) - if ag.Is(AutoGroupInternet) { - errs = append(errs, fmt.Errorf(`"autogroup:internet" used in SSH source, it can only be used in ACL destinations`)) + + if err := validateAutogroupSupported(ag); err != nil { + errs = append(errs, err) + continue + } + + if err := validateAutogroupForSSHSrc(ag); err != nil { + errs = append(errs, err) + continue } } } @@ -959,8 +1108,14 @@ func (p *Policy) validate() error { switch dst.(type) { case *AutoGroup: ag := dst.(*AutoGroup) - if ag.Is(AutoGroupInternet) { - errs = append(errs, fmt.Errorf(`"autogroup:internet" used in SSH destination, it can only be used in ACL destinations`)) + if err := validateAutogroupSupported(ag); err != nil { + errs = append(errs, err) + continue + } + + if err := validateAutogroupForSSHDst(ag); err != nil { + errs = append(errs, err) + continue } } } @@ -976,11 +1131,11 @@ func (p *Policy) validate() error { // SSH controls who can ssh into which machines. type SSH struct { - Action string `json:"action"` // TODO(kradalby): add strict type - Sources SSHSrcAliases `json:"src"` - Destinations SSHDstAliases `json:"dst"` - Users []SSHUser `json:"users"` - CheckPeriod time.Duration `json:"checkPeriod,omitempty"` + Action string `json:"action"` + Sources SSHSrcAliases `json:"src"` + Destinations SSHDstAliases `json:"dst"` + Users []SSHUser `json:"users"` + CheckPeriod model.Duration `json:"checkPeriod,omitempty"` } // SSHSrcAliases is a list of aliases that can be used as sources in an SSH rule. @@ -997,7 +1152,7 @@ func (a *SSHSrcAliases) UnmarshalJSON(b []byte) error { *a = make([]Alias, len(aliases)) for i, alias := range aliases { switch alias.Alias.(type) { - case *Username, *Group, *Tag, *AutoGroup: + case *Group, *Tag, *AutoGroup: (*a)[i] = alias.Alias default: return fmt.Errorf("type %T not supported", alias.Alias) @@ -1042,8 +1197,8 @@ func (a *SSHDstAliases) UnmarshalJSON(b []byte) error { // so we will leave it in as there is no other option // to dynamically give all access // https://tailscale.com/kb/1193/tailscale-ssh#dst - Asterix, - *Group: + // TODO(kradalby): remove this when we support autogroup:tagged and autogroup:member + Asterix: (*a)[i] = alias.Alias default: return fmt.Errorf("type %T not supported", alias.Alias) diff --git a/integration/ssh_test.go b/integration/ssh_test.go index f6e0e66d..25ede0c4 100644 --- a/integration/ssh_test.go +++ b/integration/ssh_test.go @@ -172,7 +172,7 @@ func TestSSHMultipleUsersAllToAll(t *testing.T) { { Action: "accept", Sources: []string{"group:integration-test"}, - Destinations: []string{"group:integration-test"}, + Destinations: []string{"user1@", "user2@"}, Users: []string{"ssh-it-user"}, }, }, @@ -267,7 +267,7 @@ func TestSSHIsBlockedInACL(t *testing.T) { { Action: "accept", Sources: []string{"group:integration-test"}, - Destinations: []string{"group:integration-test"}, + Destinations: []string{"user1@"}, Users: []string{"ssh-it-user"}, }, }, @@ -317,13 +317,13 @@ func TestSSHUserOnlyIsolation(t *testing.T) { { Action: "accept", Sources: []string{"group:ssh1"}, - Destinations: []string{"group:ssh1"}, + Destinations: []string{"user1@"}, Users: []string{"ssh-it-user"}, }, { Action: "accept", Sources: []string{"group:ssh2"}, - Destinations: []string{"group:ssh2"}, + Destinations: []string{"user2@"}, Users: []string{"ssh-it-user"}, }, },