starred/headscale
1
0
Fork 0
mirror of https://github.com/juanfont/headscale.git synced 2026-01-11 20:00:28 +01:00
Code Issues 98 Packages Projects Releases 10 Wiki Activity

[Bug] ACL invalid policy accepted #1033

New Issue
Closed
opened 2025-12-29 02:27:50 +01:00 by adam · 3 comments
adam commented 2025-12-29 02:27:50 +01:00
Owner
Copy Link

Originally created by @lethedata on GitHub (May 25, 2025).

Is this a support request?

  • This is not a support request

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

Running policy check with invalid src/dst returns "Policy is valid"

Expected Behavior

Policy check fails and returns an error.

Steps To Reproduce

  1. Create acls.jsonc policy using the configuration below
  2. Run policy check against bad acls.jsonc file: headscale policy check -f ./acls.jsonc
  3. Returns Policy is valid
{
    "acls": [
        { "action": "accept", "BAD": ["FOO:BAR:FOO:BAR"], "BAD": ["BAD:BAD:BAD:BAD"] }
      ]
}

Environment

- OS: Fedora CoreOS 42.20250427.3.0
- Headscale version: v0.26.0

Runtime environment

  • Headscale is behind a (reverse) proxy
  • Headscale runs in a container

Debug information

{
    "acls": [
        { "action": "accept", "BAD": ["FOO:BAR:FOO:BAR"], "BAD": ["BAD:BAD:BAD:BAD"] }
      ]
}
Originally created by @lethedata on GitHub (May 25, 2025). ### Is this a support request? - [x] This is not a support request ### Is there an existing issue for this? - [x] I have searched the existing issues ### Current Behavior Running policy check with invalid src/dst returns "Policy is valid" ### Expected Behavior Policy check fails and returns an error. ### Steps To Reproduce 1) Create `acls.jsonc` policy using the configuration below 2) Run policy check against bad `acls.jsonc` file: `headscale policy check -f ./acls.jsonc` 3) Returns `Policy is valid` ```jsonc { "acls": [ { "action": "accept", "BAD": ["FOO:BAR:FOO:BAR"], "BAD": ["BAD:BAD:BAD:BAD"] } ] } ``` ### Environment ```markdown - OS: Fedora CoreOS 42.20250427.3.0 - Headscale version: v0.26.0 ``` ### Runtime environment - [x] Headscale is behind a (reverse) proxy - [x] Headscale runs in a container ### Debug information ```jsonc { "acls": [ { "action": "accept", "BAD": ["FOO:BAR:FOO:BAR"], "BAD": ["BAD:BAD:BAD:BAD"] } ] } ```
adam added the bugno-stale-botpolicy 📝 labels 2025-12-29 02:27:50 +01:00
adam closed this issue 2025-12-29 02:27:50 +01:00
adam commented 2025-12-29 02:27:51 +01:00
Author
Owner
Copy Link

@Codelica commented on GitHub (May 28, 2025):

Please just keep one thing in mind if the plan is to validate all input properties on the ACL. A few months ago @goodieshq and I noticed that additional properties could be added to an ACL entry which were stored and ignored. This actually allows for some very nice/powerful UI state to be stored for the Headscale-Admin GUI project. For example ACL rules can be named, UI collapse state stored, etc. Currently this is a single property tree named #ha-meta on the ACL entry to keep it isolated. Perhaps when it comes to validation, property names that start with # could be ignored for metadata/comments? Please :)

Image

@Codelica commented on GitHub (May 28, 2025): Please just keep one thing in mind if the plan is to validate **all** input properties on the ACL. A few months ago @goodieshq and I noticed that additional properties could be added to an ACL entry which were stored and ignored. This actually allows for some very nice/powerful UI state to be stored for the Headscale-Admin GUI project. For example ACL rules can be named, UI collapse state stored, etc. Currently this is a single property tree named `#ha-meta` on the ACL entry to keep it isolated. Perhaps when it comes to validation, property names that start with `#` could be ignored for metadata/comments? Please :) ![Image](https://github.com/user-attachments/assets/fa9d0b76-4d32-4774-925a-d95393b9ebc8)
adam commented 2025-12-29 02:27:51 +01:00
Author
Owner
Copy Link

@kradalby commented on GitHub (Sep 9, 2025):

This is interesting. I am not very concerned that the example validates as a valid policy. The most annoying would be mistyped versions of the correct fields that are not caught, srd vs src for example.

The "use the policy as JSON storage" is quite funny too, I suppose that is relatively fine.

We can do what @Codelica suggests and reserve things prefixed with #, and then validate all the other fields to get something in between, both better validation, but also some flexibility, without letting it go "too far".

@kradalby commented on GitHub (Sep 9, 2025): This is interesting. I am not very concerned that the example validates as a valid policy. The most annoying would be mistyped versions of the correct fields that are not caught, `srd` vs `src` for example. The "use the policy as JSON storage" is quite funny too, I suppose that is relatively fine. We can do what @Codelica suggests and reserve things prefixed with `#`, and then validate all the other fields to get something in between, both better validation, but also some flexibility, without letting it go "too far".
adam commented 2025-12-29 02:27:51 +01:00
Author
Owner
Copy Link

@lethedata commented on GitHub (Sep 9, 2025):

This is interesting. I am not very concerned that the example validates as a valid policy. The most annoying would be mistyped versions of the correct fields that are not caught, srd vs src for example.

A mistyped dnt over dst typo is actually what lead to this issue. It was originally going to just be about the typo being accepted but after messing around I realized there was no validations allowing the above example to return valid. I used this unrealistic example as it very obviously shows what is going on at a glance without taking a second to locate the non-obvious typo.

@lethedata commented on GitHub (Sep 9, 2025): > This is interesting. I am not very concerned that the example validates as a valid policy. The most annoying would be mistyped versions of the correct fields that are not caught, `srd` vs `src` for example. > A mistyped `dnt` over `dst` typo is actually what lead to this issue. It was originally going to just be about the typo being accepted but after messing around I realized there was no validations allowing the above example to return valid. I used this unrealistic example as it very obviously shows what is going on at a glance without taking a second to locate the non-obvious typo.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
CLI
DERP
DNS
Nix
OIDC
SSH
bug
database
documentation
duplicate
enhancement
faq
good first issue
grants
help wanted
might-come
needs design doc
needs investigation
no-stale-bot
out of scope
performance
policy 📝
pull-request
Mirrored from GitHub Pull Request
 question
regression
routes
stale
tags
tailscale-feature-gap
well described ❤️
wontfix
No Label bug no-stale-bot policy 📝
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
adam
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/headscale#1033
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?