✨ feat: implements APIs for managing ACLs #1792

pallabpain · 2024-02-24T11:39:13Z

Description

Headscale currently lacks the APIs to manange the ACLs. The only way possible currently is to load the ACLs via file and changes to the policy requires reloading the headscale process. This also makes it difficult to integrate your headscale via APIs with no ACL management.

This commit introduces two APIs that allow your to get and set the policy.

APIs

Set ACL Policy

PUT /api/v1/acl

Payload (Example only)

{"policy": {"groups": {}, "tags": {}}}

Get ACL Policy

GET /api/v1/acl

Response

{"policy": {"groups": {}, "tags": {}}}

CLI

→ go run cmd/headscale/headscale.go --config ./config.yaml acl --help
Manage the Headscale ACL Policy

Usage:
  headscale acl [command]

Available Commands:
  get         Print the current ACL Policy JSON
  set         Updates the ACL Policy

Flags:
  -h, --help   help for acl

Global Flags:
  -c, --config string   config file (default is /etc/headscale/config.yaml)
      --force           Disable prompts and forces the execution
  -o, --output string   Output format. Empty for human-readable, 'json', 'json-line' or 'yaml'

Use "headscale acl [command] --help" for more information about a command.

Testing

acls_demo.mp4

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

Resolves

Add ACL management via API #582

hscontrol/app.go

pallabpain · 2024-03-05T18:16:12Z

@evenh @kradalby Please take a look. 🙇🏼

evenh · 2024-03-06T08:34:09Z

I might have some free time this evening to do a review

pallabpain · 2024-03-06T20:11:05Z

I've attached a screen recording to the PR in case someone wishes to see how the APIs work.

Headscale currently lacks the APIs to manange the ACLs. The only way possible currently is to load the ACLs via file and changes to the policy requires reloading the headscale process. This also makes it difficult to integrate your headscale via APIs with no ACL management. This commit introduces two APIs that allow your to get and set the policy.

evenh

Overall it looks good - I've left some inline comments. One open question is whether to support reading the current ACL via the new APIs/CLI commands even if the policy is backed by a file (for convenience)?

evenh · 2024-03-06T21:29:33Z

hscontrol/types/acl.go

+	Policy datatypes.JSON
+
+	CreatedAt  *time.Time
+	Expiration *time.Time


Is Expiration and LastSeen copy pasta? :-)

Yes. 😅 I'll fix this.

GORM also adds created at I believe, so that can also be omitted

evenh · 2024-03-06T21:32:31Z

hscontrol/types/config.go

@@ -180,6 +189,8 @@ func LoadConfig(path string, isFile bool) error {
 	viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
 	viper.AutomaticEnv()

+	viper.SetDefault("acl.policy_mode", ACLPolicyModeDB)


Wouldn't this break existing setups which already uses a file? I'm not a maintainer, but I would be fine with the DB being the default going forward but in that case we should not break existing users setups.

Yes, this will be a breaking change. I can set up an alias for existing users. However, their setups may already be broken from the recent database config restructure. I'm of the opinion that we need not keep it backward compatible since users will have to update their configs anyway.

See comment earlier, I think file should still be the default. I'm open to be convinced.

evenh · 2024-03-06T21:40:53Z

hscontrol/grpcv1.go

+		return nil, types.ErrACLPolicyUpdateIsDisabled
+	}
+
+	polBytes, err := request.GetPolicy().MarshalJSON()


Would this fail if the input is HuJSON (default Tailscale ACL format)? policy.LoadACLPolicyFromBytes will use the hujson parser by default when the format is not yaml.

Thanks for bringing this up. I will test and verify it.

evenh · 2024-03-06T21:43:32Z

hscontrol/grpcv1.go

+		return nil, errors.Wrap(err, types.ErrInvalidACLPolicyFormat.Error())
+	}
+
+	api.h.ACLPolicy = a


Should this line be moved down to after the ACL is successfully persisted?

Yes. I've moved it after we set the ACL in the database.

evenh · 2024-03-06T21:47:05Z

hscontrol/app.go

@@ -761,25 +768,9 @@ func (h *Headscale) Serve() error {
 					Msg("Received SIGHUP, reloading ACL and Config")

 				// TODO(kradalby): Reload config on SIGHUP


Suggested change

// TODO(kradalby): Reload config on SIGHUP

This still isn't implemented. I'll keep this for the time being.

Should probably not allow "large" TODOs like this without an issue... (including me)

evenh · 2024-03-06T21:49:31Z

hscontrol/app.go

+		h.ACLPolicy = pol
+
+		log.Info().
+			Msg("ACL policy successfully reloaded, notifying nodes of change")


Suggested change

Msg("ACL policy successfully reloaded, notifying nodes of change")

Msg("ACL policy successfully loaded, notifying nodes of change")

This function is both used for the initial loading and for the SIGHUP reloading.

Moved the log close to the notify call.

evenh · 2024-03-06T21:52:49Z

hscontrol/app.go

+	}
+
+	if h.ACLPolicy != nil {
+		ctx := types.NotifyCtx(context.Background(), "acl-sighup", "na")


Do we want to notifyCtx + notifyAll if it's not a reload, but a normal startup? 🤔

We do not. 😅 Thanks, I've updated this bit.

pallabpain · 2024-03-07T06:50:25Z

Overall it looks good - I've left some inline comments. One open question is whether to support reading the current ACL via the new APIs/CLI commands even if the policy is backed by a file (for convenience)?

Enabled it. 👍🏼 Now it will work for both modes.

kradalby

I think this looks reasonable, some comments and questions and to larger things:

I am tempted to use this opportunity to change the general phrasing from ACL to Policy through out the code, particularly since we are changing the config, adding commands etc.

I think the base format for the policy should be HuJSON, it is a bit more flexible format that allows preserving comments. This probably means using string/byte for the database and not the JSON type, but we dont really use it (or will use SQL to look up in the JSON) so that should not be a problem.
As part of this, I am willing to discuss dropping the YAML support to just streamline what we support.

kradalby · 2024-03-07T08:58:03Z

cmd/headscale/cli/acl.go

+
+var getACL = &cobra.Command{
+	Use:     "get",
+	Short:   "Print the current ACL Policy JSON",


A very high level note, I think this should support the same HuJSON (and potentially YAML) format that the ACL support.

The main reason for this is that it can preserve comments which would be important for large policies.

I would also assume that some of the WebUIs will implement some sort of editors, and having comments would be important for documentation purposes.

For the record, I am OK with just HuJSON, we might want to remove the YAML support and only support one format.

kradalby · 2024-03-07T09:00:47Z

config-example.yaml

+  # If "file" is selected, the policies will be loaded from a file
+  # located at `policy_path`. It will not be stored in the databse
+  # and you will not be able to get or set the ACL via the ACL APIs.
+  policy_mode: "db"


I'm open for the example config to be db, but the default should still be file.

I'm open for the example config to be db, but the default should still be file.

Personally I strongly disagree.

Many people still use docker. (I know I know, it's not officially supported... still!)
Manipulating files inside it, especially if the container is not running is a very hard task.

IMHO it is more secure to store ACL data inside a db. A separate file can be easier hacked / modified / viewed.
After all, ACL is the "main security" config of HeadScale, telling who has access to what !
Please keep in mind: The main reason for using VPNs is security.
Offering an easy-to-view / edit file to the open is against it.

Eventually more people will use this feature using a web-UI, so it would make much easier to setup with that already.

For those who like to do things manually (by typing text files, etc) -> changing the config to file will not be a big task to do, but for those who are not used to do tasks like this: it would be an "impossible nightmare".

Anyway: is the database password protected by default, so only HS can access it?

or can other programs access it too?

kradalby · 2024-03-07T09:01:38Z

go.mod

@@ -96,6 +98,7 @@ require (
 	github.com/glebarez/go-sqlite v1.22.0 // indirect
 	github.com/go-jose/go-jose/v3 v3.0.1 // indirect
 	github.com/go-ole/go-ole v1.3.0 // indirect
+	github.com/go-sql-driver/mysql v1.7.0 // indirect


Wonder why we suddenly need a mysql driver?

kradalby · 2024-03-07T09:02:49Z

hscontrol/app.go

@@ -761,25 +768,9 @@ func (h *Headscale) Serve() error {
 					Msg("Received SIGHUP, reloading ACL and Config")

 				// TODO(kradalby): Reload config on SIGHUP


Should probably not allow "large" TODOs like this without an issue... (including me)

kradalby · 2024-03-07T09:04:34Z

hscontrol/app.go

+			aclPath := util.AbsolutePathFromConfigPath(h.cfg.ACL.PolicyPath)
+			pol, err := policy.LoadACLPolicyFromPath(aclPath)
+			if err != nil {
+				return pkgerrors.Wrap(err, "failed to load ACL policy from file")


Whats the difference between this error wrap compared to:

fmt.Errorf("failed to load ACL policy from file: %w", err)

kradalby · 2024-03-07T09:11:34Z

hscontrol/types/acl.go

+	Policy datatypes.JSON
+
+	CreatedAt  *time.Time
+	Expiration *time.Time


GORM also adds created at I believe, so that can also be omitted

kradalby · 2024-03-07T09:13:30Z

hscontrol/types/config.go

@@ -180,6 +189,8 @@ func LoadConfig(path string, isFile bool) error {
 	viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_"))
 	viper.AutomaticEnv()

+	viper.SetDefault("acl.policy_mode", ACLPolicyModeDB)


See comment earlier, I think file should still be the default. I'm open to be convinced.

kradalby · 2024-03-07T09:14:41Z

hscontrol/types/config.go

+	policyPath := viper.GetString("acl.policy_path")
+	policyMode := viper.GetString("acl.policy_mode")


Maybe we should just use the term policy?

policy: mode: "" // <-- does "source" or "storage" or "backend" make more sense than mode? path: ""

kradalby · 2024-03-07T09:21:54Z

hscontrol/types/acl.go

+)
+
+// ACL describes the data model for ACLs used to restrict access to resources.
+type ACL struct {


There was a proposal of having the policy versioned. I do not think we should do that now, but I think we should think of if there is anything about the format that should be addressed to enable that in the future. I do not have any concerns per now, but want to hear what you think.

having the policy versioned.

I agree. Having plus one line of a simple version=2024-03-24 is not a big deal now,
but can save us lots of trouble later.

_(In my 30y. of experience I've realized:

it is better to use ISO-dates for versions.

Saves lot's of time and prevents confusion.
Also better to merge with release-notes / what changed.)_

pallabpain · 2024-03-07T12:03:02Z

I think this looks reasonable, some comments and questions and to larger things:

I am tempted to use this opportunity to change the general phrasing from ACL to Policy through out the code, particularly since we are changing the config, adding commands etc.

I think the base format for the policy should be HuJSON, it is a bit more flexible format that allows preserving comments. This probably means using string/byte for the database and not the JSON type, but we dont really use it (or will use SQL to look up in the JSON) so that should not be a problem. As part of this, I am willing to discuss dropping the YAML support to just streamline what we support.

Thanks for the review. This sounds like a good idea. I wrote this implementation to quickly demonstrate the APIs and commands that we can use as a reference to discuss the feature further.

Keeping the base format as HuJSON should be the way to proceed and we can store it as a bytearray or string in the db since we don't intend to query it any further. Although I'm not entirely sure how the API request and response will look over RPC since I do not want to model the policy structure in the proto definition.

I am also okay with keeping the default mode as file.

Let me update this PR with the suggestions and put it our for a second round of review.

evenh · 2024-03-22T10:11:14Z

Hi @pallabpain. Have you had a chance to update the PR yet?

pallabpain · 2024-03-27T06:30:34Z

Hi @pallabpain. Have you had a chance to update the PR yet?

I haven't had the chance to update the PR, yet. I have some untested changes locally that I should be able to work on this weekend. I may change the request-response structure based on the newer comments.

IamTaoChen · 2024-05-09T22:58:24Z

Maybe, we should create 2 new tables to store groups and tags. And sometimes, we just want to update groups or tags. So /api/v1/acl/groups, /api/v1/acl/tags and /api/v1/acl/all may be more useful.

I'm thinking about how to update ACL groups automatically, e.g. AD/LDAP or extract from OIDC (OIDC may be a problem because it can only get groups when users authenticate)

kradalby · 2024-05-24T09:25:36Z

@pallabpain there has been quite some changes, do you think that you might have time to pick this up again?

pallabpain · 2024-05-24T09:28:36Z

@pallabpain there has been quite some changes, do you think that you might have time to pick this up again?

Sure. I'll review the recent changes and update the pull request. I'll try and get a working version ready soon.

pallabpain mentioned this pull request Feb 24, 2024

Add ACL management via API #582

Open

tobru mentioned this pull request Mar 4, 2024

Add ACL ? GoodiesHQ/headscale-admin#13

Open

pallabpain force-pushed the feat/acl-apis branch 4 times, most recently from 4331948 to a536ecb Compare March 5, 2024 06:59

pallabpain commented Mar 5, 2024

View reviewed changes

hscontrol/app.go Outdated Show resolved Hide resolved

pallabpain force-pushed the feat/acl-apis branch from a536ecb to 0b3660c Compare March 5, 2024 18:13

pallabpain marked this pull request as ready for review March 5, 2024 18:14

pallabpain requested review from juanfont and kradalby as code owners March 5, 2024 18:14

pallabpain force-pushed the feat/acl-apis branch from 0b3660c to 6099d3b Compare March 6, 2024 20:46

evenh reviewed Mar 6, 2024

View reviewed changes

pallabpain added 2 commits March 7, 2024 09:26

fix: addresses review comments (fixup)

d0b500b

fix: makes file-based ACL accessible from APIs

a80992d

kradalby reviewed Mar 7, 2024

View reviewed changes

		@@ -761,25 +768,9 @@ func (h *Headscale) Serve() error {
		Msg("Received SIGHUP, reloading ACL and Config")

		// TODO(kradalby): Reload config on SIGHUP

	Msg("ACL policy successfully reloaded, notifying nodes of change")
	Msg("ACL policy successfully loaded, notifying nodes of change")

		policyPath := viper.GetString("acl.policy_path")
		policyMode := viper.GetString("acl.policy_mode")

✨ feat: implements APIs for managing ACLs #1792

Are you sure you want to change the base?

✨ feat: implements APIs for managing ACLs #1792

Conversation

pallabpain commented Feb 24, 2024 • edited

Description

APIs

Set ACL Policy

Get ACL Policy

CLI

Testing

Resolves

pallabpain commented Mar 5, 2024

evenh commented Mar 6, 2024

pallabpain commented Mar 6, 2024

evenh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pallabpain commented Mar 7, 2024

kradalby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pallabpain commented Mar 7, 2024 • edited

evenh commented Mar 22, 2024

pallabpain commented Mar 27, 2024

IamTaoChen commented May 9, 2024

kradalby commented May 24, 2024

pallabpain commented May 24, 2024

pallabpain commented Feb 24, 2024 •

edited

pallabpain commented Mar 7, 2024 •

edited