Skip to content

feat(signer): add "Sign voluntary exit" permission - #107

Open
selfuryon wants to merge 2 commits into
attestantio:masterfrom
selfuryon:feat/vem-verb
Open

feat(signer): add "Sign voluntary exit" permission#107
selfuryon wants to merge 2 commits into
attestantio:masterfrom
selfuryon:feat/vem-verb

Conversation

@selfuryon

Copy link
Copy Markdown

Voluntary exits were authorized solely by source IP address (server.rules.admin-ips), which is unusable in environments with dynamic client addresses such as Kubernetes.

Introduce a dedicated "Sign voluntary exit" operation checked against the client certificate, matching how all other actions are authorized. Clients holding the permission (explicitly or via "All") can sign voluntary exits from any address; clients without it fall back to the generic "Sign" permission gated by admin-ips, preserving existing configurations.

Closes #105

Voluntary exits were authorized solely by source IP address
(server.rules.admin-ips), which is unusable in environments with
dynamic client addresses such as Kubernetes.

Introduce a dedicated "Sign voluntary exit" operation checked against
the client certificate, matching how all other actions are authorized.
Clients holding the permission (explicitly or via "All") can sign
voluntary exits from any address; clients without it fall back to the
generic "Sign" permission gated by admin-ips, preserving existing
configurations.

Closes attestantio#105
@selfuryon
selfuryon marked this pull request as ready for review July 16, 2026 12:13

@AntiD2ta AntiD2ta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The new "Voluntary exit" permission (that allows signing an exit gated on domain/certs rather than on IPs) is neatly wired and its implementation is sound. A concern I have is that the feature is opt-out, i.e you have to do the following if you want to use "All" and still rely on admin-ips as a safety measure:

[~Sign voluntary exit, All]

This breaks the current contract we have, where "All" is safe to use and it allows exits from admin-ips only (which is opt-in in a way: you must set the IPs). People using "All" and not opting out from "Sign voluntary exit" may find themselves in a position where clients who couldn't previously get an exit signed, now can.

We are brainstorming internally if we are fine releasing this as a breaking change or requesting changes to make it opt-in only. Do you have a strong opinion on this?

While we discuss this, I raised some minor items I will be grateful if you can address/challenge.

Comment thread services/signer/standard/multisign.go
Comment thread services/signer/standard/helpers.go Outdated
Comment thread docs/permissions.md
Comment thread rules/standard/signvoluntaryexit.go
@selfuryon

selfuryon commented Jul 17, 2026

Copy link
Copy Markdown
Author

The new "Voluntary exit" permission (that allows signing an exit gated on domain/certs rather than on IPs) is neatly wired and its implementation is sound. A concern I have is that the feature is opt-out, i.e you have to do the following if you want to use "All" and still rely on admin-ips as a safety measure:

[~Sign voluntary exit, All]

This breaks the current contract we have, where "All" is safe to use and it allows exits from admin-ips only (which is opt-in in a way: you must set the IPs). People using "All" and not opting out from "Sign voluntary exit" may find themselves in a position where clients who couldn't previously get an exit signed, now can.

We are brainstorming internally if we are fine releasing this as a breaking change or requesting changes to make it opt-in only. Do you have a strong opinion on this?

While we discuss this, I raised some minor items I will be grateful if you can address/challenge.

I also thought about that and implementation of it based on these:

  • Current repo already documented that they have Voluntary exit permission (but I like more Sign voluntary exit - it matches with other permissions) and to exclude it from all dirk offers a special syntax. Yes, it wasn't implemented by fact but already documented
  • admin-ips is the only exception for the rules and semi-documented only in documentation like comment in the example configuration , looks strange if we take into account all other rule system based on PKI and permitted verbs for it.

So I decided to implement it like it was mentioned in the docs. But maybe it's a good idea not to exclude admin-ip (honestly I thought that it will be a good idea to remove it in future) but add it like additional security maybe (dunno honestly). Right now they a mutually excluded and the verb take precedence which I think it fine, but didn't think about other users... If you want, I can combine them to allow it to work together, wdyt?

P.S.: Or let me think about that again...

- carry out the duplicate public key check across the entire Multisign
  batch, as the rules now run separately per action
- fall back to the generic Sign permission only on an authorisation
  denial, not on system failures
- guard against nil request data in OnSignVoluntaryExit
- clarify in the permissions documentation that an explicit denial of
  "Sign voluntary exit" only removes the IP-free path
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.

Problem with admin-ips and VEM

2 participants