-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 support for autoApprovers ACL #763
Changes from 9 commits
004ebca
7653ad4
60cc9dd
9810d84
842c28a
548551c
688cba7
a9da953
adb352e
5b12ab9
d764f52
6d2cfd5
7761a7b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -949,6 +949,64 @@ func (h *Headscale) EnableRoutes(machine *Machine, routeStrs ...string) error { | |
return nil | ||
} | ||
|
||
// Enabled any routes advertised by a machine that match the ACL autoApprovers policy. | ||
func (h *Headscale) EnableAutoApprovedRoutes(machine *Machine) { | ||
if len(machine.IPAddresses) == 0 { | ||
return // This machine has no IPAddresses, so can't possibly match any autoApprovers ACLs | ||
} | ||
|
||
approvedRoutes := make([]netip.Prefix, 0, len(machine.HostInfo.RoutableIPs)) | ||
thisMachine := []Machine{*machine} | ||
|
||
for _, advertisedRoute := range machine.HostInfo.RoutableIPs { | ||
if contains(machine.EnabledRoutes, advertisedRoute) { | ||
continue // Skip routes that are already enabled for the node | ||
} | ||
|
||
routeApprovers, err := h.aclPolicy.AutoApprovers.GetRouteApprovers( | ||
advertisedRoute, | ||
) | ||
if err != nil { | ||
log.Err(err). | ||
Str("advertisedRoute", advertisedRoute.String()). | ||
Uint64("machineId", machine.ID). | ||
Msg("Failed to resolve autoApprovers for advertised route") | ||
|
||
return | ||
} | ||
|
||
for _, approvedAlias := range routeApprovers { | ||
if approvedAlias == machine.Namespace.Name { | ||
approvedRoutes = append(approvedRoutes, advertisedRoute) | ||
} else { | ||
approvedIps, err := expandAlias(thisMachine, *h.aclPolicy, approvedAlias, h.cfg.OIDC.StripEmaildomain) | ||
if err != nil { | ||
log.Err(err). | ||
Str("alias", approvedAlias). | ||
Msg("Failed to expand alias when processing autoApprovers policy") | ||
|
||
return | ||
} | ||
|
||
// approvedIPs should contain all of machine's IPs if it matches the rule, so check for first | ||
if contains(approvedIps, machine.IPAddresses[0].String()) { | ||
approvedRoutes = append(approvedRoutes, advertisedRoute) | ||
} | ||
} | ||
} | ||
} | ||
|
||
for _, approvedRoute := range approvedRoutes { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have one concern here, but feel free to let me know if I have missed it: If a node advertise a route, and at the time it is accepted by the ACL autoapprove, what happens if it is removed from the ACL? I believe the current behaviour would result in the route being left active, which is probably ok since the feature is Auto approve and not implying that it will keep the settings in sync. However we probably should call this out somewhere, not sure where tho. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah you're right - if a route advertised by a node is autoApproved under the current policy, and that policy changes, the route will remain approved. As far as I can gather this is similar to the SaaS offering's behaviour: once a route is auto-approved its up to the user to manage it from that point. Let me know if there's a good place to docco it - another option could be (if my other preauthkey related PR is merged) writing a doc on how to use both features together and call out this behaviour there? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this resembles the upstream behaviour, which the name implies, that it will be approved and is not "state management", then it is fine. You can document them together. Another quick question, if a route is already announced by a node, but not approved, and then the autoApprove rule is added, will it get approved? Basically, is this evaluated when a node joins/sends an update, etc. |
||
if !contains(machine.EnabledRoutes, approvedRoute) { | ||
log.Info(). | ||
Str("route", approvedRoute.String()). | ||
Uint64("client", machine.ID). | ||
Msg("Enabling autoApproved route for client") | ||
machine.EnabledRoutes = append(machine.EnabledRoutes, approvedRoute) | ||
} | ||
} | ||
} | ||
|
||
func (machine *Machine) RoutesToProto() *v1.Routes { | ||
availableRoutes := machine.GetAdvertisedRoutes() | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
// This ACL validates autoApprovers support for | ||
// exit nodes and advertised routes | ||
|
||
{ | ||
"tagOwners": { | ||
"tag:exit": ["test"], | ||
}, | ||
|
||
"groups": { | ||
"group:test": ["test"] | ||
}, | ||
|
||
"acls": [ | ||
{"action": "accept", "users": ["*"], "ports": ["*:*"]}, | ||
], | ||
|
||
"autoApprovers": { | ||
"exitNode": ["tag:exit"], | ||
"routes": { | ||
"10.10.0.0/16": ["group:test"], | ||
"10.11.0.0/16": ["test"], | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this logic might be the wrong way around?
i.e if I have an auto approved prefix of
10.0.0.0/8
and then I try to advertise10.1.0.0/24
, I would expectautoApprovedPrefix.Bits()
== 8 andprefix.Bits()
== 24, or am I misunderstanding this?I ask because I tried out the parent prefix thing and it doesn't seem to work for me. For now I'm going to add all 255 /24s I need to my ACL to move on xD. If I get some time later I might try forking and inverting this logic and see if it fixes my issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right, bugger but thanks for the find.
@juanfont @kradalby I'll write and submit a PR to fix this with a regression test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks for the speedy response. And also thanks for shipping this very hype feature :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
np, nothing good on tv right now 😂 should be fixed in #862 hopefully