[Feature] Headscale policy set validate ACL before applying?

[dragon2611]

Use case

Is it possible to have the headscale policy set command run validation before applying the policy and only load the policy if it passed syntax checking.

Description

I accidentally put autogroup:internet instead of autogroup:internet:* and it really didn't like that.

Basically all the clients started throwing back

Could not get the create map update error="strconv.ParseUint: parsing \"internet\": invalid syntax" and I'm pretty sure they all dropped off the tailnet.

Contribution

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

How can it be implemented?

No response

[dragon2611]

Also see #2045 - I'm happy to contribute but writing design docs is rather outside my area of expertise, and you probably wouldn't want any code contributions from me as I'm very much a novice.

[kradalby]

Would make sense, @pallabpain are you able to take a look?

[pallabpain]

Would make sense, @pallabpain are you able to take a look?

Yes, I'll take a look. @dragon2611 It'd be great if you could share the policy file you were trying to apply.

Although the SetPolicy API does validate the payload

headscale/hscontrol/grpcv1.go

Line 723 in 948d53f

valid, err := policy.LoadACLPolicyFromBytes([]byte(p))

, maybe it's missing something.

[dragon2611]

Whilst I'd rather not post the entire ACL The thing that seemed to trip things up was

 { "action": "accept", "src": ["tag:mobiles"], "dst": ["autogroup:internet"] },
 { "action": "accept", "src": ["tag:pcs"], "dst": ["autogroup:internet"] }

What appeared to work was however

 { "action": "accept", "src": ["tag:mobiles"], "dst": ["autogroup:internet:*"] },
 { "action": "accept", "src": ["tag:pcs"], "dst": ["autogroup:internet:*"] }

[pallabpain]

Whilst I'd rather not post the entire ACL The thing that seemed to trip things up was

{ "action": "accept", "src": ["tag:mobiles"], "dst": ["autogroup:internet"] },
{ "action": "accept", "src": ["tag:pcs"], "dst": ["autogroup:internet"] }

What appeared to work was however

{ "action": "accept", "src": ["tag:mobiles"], "dst": ["autogroup:internet:"] },
{ "action": "accept", "src": ["tag:pcs"], "dst": ["autogroup:internet:"] }

Thanks. This helps. I'll use this for debugging the issue.

[dragon2611]

No problem, just be wary github initially parsed it I think as the * wasn't showing.

Was having a bit of problem with the formatting.

[pallabpain]

No problem, just be wary github initially parsed it I think as the * wasn't showing.

Was having a bit of problem with the formatting.

So the issue is that while the policy may be a valid HuJSON/JSON that can be marshaled into the policy struct, it may not have the right values expected in the policy. The current code only ensures format.

[dragon2611]

I'm guessing the tailscale client didn't like the lack of a port definition, which is weird as for no TCP/UDP traffic there wouldn't be a port anyways.

[pallabpain]

I'm guessing the tailscale client didn't like the lack of a port definition, which is weird as for no TCP/UDP traffic there wouldn't be a port anyways.

Per Tailscale docs, the port is expected.
https://tailscale.com/kb/1337/acl-syntax#dst

[pallabpain]

Would make sense, @pallabpain are you able to take a look?

@kradalby I couldn't find anything in the tailscale client that I can use to validate the policy. Looks like it has to be implemented in headscale.

[oneingan]

Slightly offtopic but in our company we have integration tests for ACL changes using NixOS VMs based testing framework. The misconfiguration were a coomon thing before that. So I can imagine how proper validation tests could help a lot with ACLs.

[kradalby]

I think this is being solved, or at least improved in #2089