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

[Feature] Manual approval of the node #2176

Open
2 tasks done
hopleus opened this issue Oct 7, 2024 · 9 comments
Open
2 tasks done

[Feature] Manual approval of the node #2176

hopleus opened this issue Oct 7, 2024 · 9 comments
Labels
enhancement New feature or request

Comments

@hopleus
Copy link

hopleus commented Oct 7, 2024

Use case

It is necessary to control the nodes connecting to my network.

Description

For example: registering a new node via CLI requires an explicit action by the administrator, which reduces the risk of unauthorised access to the network, unlike registering a node via OIDC, where all responsibility falls on an external OIDC system that can be compromised. To reduce the risk, additional approval (Device approval - https://tailscale.com/kb/1099/device-approval) is required for new nodes on the network.

Contribution

  • I can write the design doc for this feature
  • I can contribute this feature

How can it be implemented?

  • Add an additional field authorised: datetime IS NULL to the nodes table database.
  • Add IsAuthorised validation methods to the Node model that will check that the field is not NULL or IsZero.
  • Replace MachineAuthorised: !node.IsExpired() with MachineAuthorized: node.IsAuthorized().
  • Add an additional CLI command to approve a node
  • Add forced approval of a node if it is registered via CLI
  • Add additional keys to PreAuthKey to register immediately approved node
  • Introduce an additional check in the CanAccess method to block approved nodes from communicating with unapproved nodes

P.S. I'm already using this change on my network.

@hopleus hopleus added the enhancement New feature or request label Oct 7, 2024
@hopleus
Copy link
Author

hopleus commented Oct 7, 2024

@kradalby If it seems reasonable, I can do a PR.

@IamTaoChen
Copy link

I have a question regarding this:

I am logged in on two devices (A and B) with my OIDC account. Since I’m accessing my own resources, I wonder if it should really be necessary to manually approve device B after I’ve already logged in on device A.

@hopleus
Copy link
Author

hopleus commented Oct 7, 2024

I have a question regarding this:

I am logged in on two devices (A and B) with my OIDC account. Since I’m accessing my own resources, I wonder if it should really be necessary to manually approve device B after I’ve already logged in on device A.

That's the idea, device validation. If enabled, you should validate each device to reduce the risk of adding an unauthorized node if your OIDC account is compromised.

P.S. If this functionality is not needed, it could always be switched off via config.yaml

@kradalby
Copy link
Collaborator

kradalby commented Oct 7, 2024

This will implement what Tailscale calls device approval, which I am open to, but it will have to align with the upstream behaviour.

  • Add an additional field authorised: datetime IS NULL to the nodes table database.

I feel like a boolean should be sufficient here, either the device is approved or not.

  • Add IsAuthorised validation methods to the Node model that will check that the field is not NULL or IsZero.
  • Replace MachineAuthorised: !node.IsExpired() with MachineAuthorized: node.IsAuthorized().
  • Introduce an additional check in the CanAccess method to block approved nodes from communicating with unapproved nodes

I think all of these can be omitted, as per the docs, the device should not be allowed to do anything:
From the Tailscale docs:

A device awaiting approval cannot send or receive traffic on your Tailscale network until it is approved.

So I think it would be a lot more meaningful to just filter ListPeers by authorised if it is enabled in the configuration.
That way we just save a bunch of compute and the implementation can be kept quite simple, and have little change.

When a device is approved, a state update is sent to make new node lists being sent.

  • Add an additional CLI command to approve a node
  • Add forced approval of a node if it is registered via CLI

I think an approve command makes sense for OIDC, while for what we call "web auth" where you have to issue a command, it doesn't really make sense to have to execute two commands instead of just one.

I am willing to be convinced that we should have it for both, but at least OIDC makes sense from the start.

  • Add additional keys to PreAuthKey to register immediately approved node

As per tailscale docs, all preauthorisation keys should be automatically approved.

As you mentioned in the follow up comment, this should be opt-in in the configuration and not on by default.

I think we will also need a webpage explaining to the users to contact their admin (see tailscale docs) that is at least given after you have logged in with OIDC. This web page should reuse the same style as the current OIDC one, but should be written using go-elem and not gotemplate.

As long as we end up in a state with the same features as upstream, I am quite positive for you to contribute this!

@hopleus
Copy link
Author

hopleus commented Oct 7, 2024

I feel like a boolean should be sufficient here, either the device is approved or not.

I believe it is necessary to store the date so that network administrators can know at any time when a particular node was approved.

@hopleus
Copy link
Author

hopleus commented Oct 8, 2024

Add additional keys to PreAuthKey to register immediately approved node

As per tailscale docs, all preauthorisation keys should be automatically approved.

@kradalby but the Tailscale docs says:

When you generate a new auth key, you can specify that the key should automatically approve devices for which the auth key is used.

From which I conclude that you can create AuthKey with or without automatic node approval.
And so it is necessary to come to a common decision how it will be implemented in HeadScale.

P.S. I think it is necessary to do the same as in TailScale, because very often when describing HeadScale functionality references to TailScale documentation are used.

@kradalby
Copy link
Collaborator

kradalby commented Oct 8, 2024

I believe it is necessary to store the date so that network administrators can know at any time when a particular node was approved.

Then we have the problem of how we will treat deauthorised nodes and how to note when that happens. When something happens, particularly when it can happen on and off multiple times should go in logs.

So bool + logs make sense.

From which I conclude that you can create AuthKey with or without automatic node approval.
And so it is necessary to come to a common decision how it will be implemented in HeadScale.

It is not an option in Tailscale, it will always be approved (see screenshot)
image

@hopleus
Copy link
Author

hopleus commented Oct 8, 2024

I believe it is necessary to store the date so that network administrators can know at any time when a particular node was approved.

Then we have the problem of how we will treat deauthorised nodes and how to note when that happens. When something happens, particularly when it can happen on and off multiple times should go in logs.

So bool + logs make sense.

I'll agree

From which I conclude that you can create AuthKey with or without automatic node approval.
And so it is necessary to come to a common decision how it will be implemented in HeadScale.

It is not an option in Tailscale, it will always be approved (see screenshot)

@kradalby No)
This option is provided in Tailscale (With Manually approve new devices enabled in Device management) (see screenshot).
Screenshot 2024-10-08 at 10 36 15

@kradalby
Copy link
Collaborator

kradalby commented Oct 8, 2024

This option is provided in Tailscale (With Manually approve new devices enabled in Device management) (see screenshot).

ah cool, I did not have it enabled, which makes sense, please proceed then!

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

No branches or pull requests

3 participants