Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SSH ACL support #847

Merged
merged 18 commits into from
Nov 26, 2022
Merged

Add SSH ACL support #847

merged 18 commits into from
Nov 26, 2022

Conversation

evenh
Copy link
Contributor

@evenh evenh commented Oct 6, 2022

Based on the fork of @db48x from #661. Doesn’t support the ‘autogroup’ ACL functionality.

This ACL works for us (formatted slightly for readability)

{
  "acls":[{
      "action":"accept",
      "src":["group:employees"],
      "dst":["tag:proxy:*"]
    }
  ],
  "hosts":{},
  "groups":{
    "group:employees":[
      "john.doe",
      "jane.doe"
    ],
    "group:proxy":[
      "mycorp"
    ]
  },
  "tagOwners":{
    "tag:proxy":[
      "group:proxy"
    ]
  },
  "ssh":[
    {
      "action":"check",
      "src":["group:employees"],
      "dst":["tag:proxy"],
      "users":["some-allowlisted-user"],
      "checkPeriod":"8h"
    }
  ],
  "disableIPv4":false,
  "randomizeClientPort":false
}
  • read the CONTRIBUTING guidelines
  • raised a GitHub issue or discussed it on the projects chat beforehand
  • added unit tests
  • added integration tests
  • updated documentation if needed
  • updated CHANGELOG.md

machine.go Show resolved Hide resolved
Copy link
Contributor

@db48x db48x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s starting to look pretty good.

acls.go Show resolved Hide resolved
return rules, nil
}

func sshCheckAction(duration string) (*tailcfg.SSHAction, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should make similar constructor functions for the accept and reject actions as well.

Copy link
Collaborator

@kradalby kradalby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks like sensible work on this feature, I have not worked too much on the ACLs and our coverage of the feature and unit tests are good, but it suffers from the problem of testing "our interpretation" of how it should work.

My main concern with this is that if we get it wrong, and users start relying on that, we have a hard time fixing it. With things like SSH, the worst possible scenario is that someone lock themselves out from a machine.

As mentioned in Discord, integration tests would make a lot of sense for this feature, so we have the opportunity to actually verify that the client does what we expect and intend. I am not sure if Tailscale SSH will play nicely in Docker, but I think it is worth a shot.

I don't want to block this feature, the question is, how should we roll it out to minimise the impact if we get something wrong? (This hesitant approach comes from the Namespace/User/Tailnet issue we had earlier)

@evenh evenh force-pushed the ssh-support branch 2 times, most recently from 088ea4c to 82f1e5d Compare November 11, 2022 12:20
@iSchluff
Copy link
Contributor

@kradalby maybe put it behind a feature toggle to let users know that the feature is experimental for now?

@kradalby
Copy link
Collaborator

Progress update: This is looking good, we are missing a few more test cases and @evenh and I have figured that we at least want:

  • Given empty ACL file, no SSH should work
  • Verify that given two namespaces, which only allow :22 access in its own namespace, cross namespace SSH should not work
  • SSH ACL rules are configured, but no rules to allow :22, should not allow SSH access.

I think that should be sufficient to call it beta and add more tests later as we find broken behaviour.

@evenh evenh force-pushed the ssh-support branch 2 times, most recently from 71d1f7a to f48abd1 Compare November 21, 2022 22:24
evenh and others added 15 commits November 25, 2022 14:49
Advertises the SSH capability, and parses the SSH ACLs to pass to the
tailscale client. Doesn’t support ‘autogroup’ ACL functionality.

Co-authored-by: Daniel Brooks <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
This commit makes the initial SSH test a bit simpler:

- Use the same pattern/functions for all clients as other tests
- Only test within _one_ namespace/user to confirm the base case
- Use retry function, same as taildrop, there is some funky going on
  there...

Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
Signed-off-by: Kristoffer Dalby <[email protected]>
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@kradalby kradalby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this, letting @juanfont and then we can get it in.

Copy link
Owner

@juanfont juanfont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent!

@kradalby kradalby merged commit 36b8862 into juanfont:main Nov 26, 2022
@dedene
Copy link

dedene commented Apr 15, 2023

@juanfont should the ssh "check" feature in ACLs work? I never seems to be getting the Tailscale SSH requires an additional check (as shown on https://tailscale.com/kb/1193/tailscale-ssh/#configure-tailscale-ssh-with-check-mode) although the ssh ACL uses "check" instead of "accept" with a low checkPeriod.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants