Skip to content

Add support for network-names caveat. - #51

Merged
timflyio merged 5 commits into
mainfrom
network-caveat
Nov 12, 2026
Merged

timflyio merged 5 commits into
mainfrom
network-caveat

Conversation

@timflyio

@timflyio timflyio commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

The network-names caveat has a list of network names that are allowed. If the access request does not provide a name, it fails. If the access request provides a name that is not in the set of allowed names, it fails. It can be used with the if-present caveat to allow access when a network resource is not requested, or if it is requested, if the network name matches.

The network-names caveat has a list of network names that are allowed. If the access request does not provide a name, it fails. If the access request provides a name that is not in the set of allowed names, it fails.  It can be used with the if-present caveat to allow access when a network resource is not requested, or if it is requested, if the network name matches.
@timflyio
timflyio marked this pull request as ready for review October 2, 2026 19:40
Comment thread flyio/access.go Outdated
if f.AppFeature != nil {
appResources = append(appResources, *f.AppFeature)
}
if f.NetworkName != nil {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the network name app-owned, so a check that names a network also needs an AppID. wggwd's token check asks about the org and the wg feature, with no app: tokenauth.go#L152-L156

Validate() runs before the caveats (caveat_set.go#L58), so wggwd gets must specify app if app-owned resource is specified as soon as it sets NetworkName. Can we drop this, so the name can go with a feature or an app?

Suggested change
if f.NetworkName != nil {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yah, we can drop the app requirement for this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to an org resource check instead of an app resource check, which I think is the appropriate thing here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem with making it an org resource: org resources are mutually exclusive (access.go#L67-L72), so wggwd can't ask about the wg feature and network name in one check. It gets resources are mutually exclusive: wg, network-name.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And splitting the check into two checks wouldn't work for existing tokens. fly tokens create wireguard allows read only on anything that isn't an app or wg (create.go#L590-L600), so a write check on the network name is refused.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the network name go with a feature or an app, and only require the org? OrgID is already required for every access

Comment thread flyio/caveats.go
}

type NetworkNames struct {
NetworkNames resset.ResourceSet[string, resset.Action] `json:"network_names"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should a token name the default network? Its name is empty (provision.go#L89-L93), and an empty key in a string ResourceSet matches every resource:

if zeroPerm, hasZero := rs[zeroID]; hasZero {
perm &= zeroPerm
foundPerm = true
allowedIDs = append(allowedIDs, zeroID)
}

Would you agree to checking the default network as default in wggwd?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. We could assign a name for the default network and use that when doing access checks, but that leaves open the possibility of collision with some network that happens to use that name. Or we could change the resource set type from "string" to something like {bool,string} (or alternately, encode it into the string like "/networkname" vs ":default" or something ad hoc like that).
Do you have thoughts?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right about possible collisions. We can take advantage of the name validation, which ensures that all names start with a letter (network.rb#L29-L37) to use a reserved name, like :default. It'd be good to define the constant in this library and reuse it everywhere.

@timflyio
timflyio requested a review from a team as a code owner November 12, 2026 18:36
@timflyio
timflyio merged commit c9aa829 into main Nov 12, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants