Skip to content

feat(ip): the last 29 routes — licences, registry, delegation and the exit door - #257

Draft
Denis-hamon wants to merge 22 commits into
ovh:feat/ip-incident-surfacefrom
Denis-hamon:feat/ip-administration
Draft

feat(ip): the last 29 routes — licences, registry, delegation and the exit door#257
Denis-hamon wants to merge 22 commits into
ovh:feat/ip-incident-surfacefrom
Denis-hamon:feat/ip-administration

Conversation

@Denis-hamon

@Denis-hamon Denis-hamon commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What this adds

This closes the ip domain. It had 15 of its 81 routes; with #255 and #256 below it, all the live
ones are reachable.

ovhcloud ip campus
ovhcloud ip licenses <ip_block>                          # the eight licence routes, in one answer
ovhcloud ip delegation list|get|add|remove
ovhcloud ip ripe get|set
ovhcloud ip byoip aggregations|slices|aggregate|slice
ovhcloud ip migration-token get|create [--reveal]
ovhcloud ip change-org <ip_block> <organisation>
ovhcloud ip service list|get|edit|change-contact|terminate|confirm-termination
ovhcloud ip service service-info get|edit

29 routes, 22 commands. Stacked on #256; only the last commit belongs to this pull request.

What the account said

GET /ip/{ip}/delegation answers HTTP 500 on 492 of 537 blocks. The 45 that answer 200 are
exactly the IPv6 /56 and /64 blocks — 27 and 18. The failures are every IPv4 mask (336 × /32,
18 × /30, 8 × /28, 1 × /26) and every IPv6 /128 (129). No prefix falls on both sides. A block
that does not exist answers a correct 404 This service does not exist, so the 500 is not a
not-found; and the sibling bring-your-own-IP routes show the API can do this properly — they answer
400 This IP is not part of the Bring you own IP product. on all 537. The API summarises the route
as "Reverse delegation on IPv6 subnet", so that sentence is attached to the failure when the block
is IPv4. This is the third API defect this audit has turned up, after install/hardwareRaidSize
(404 on a route badged "Stable production version") and /ip/{ip}/phishing (500 outside Europe,
in #256). Worth a look by the route's owner.

Three live migration tokens were sitting on this account. The token is a bearer credential:
whoever holds it, with the customer identifier printed beside it, can take the address. So
migration-token get prints a fingerprint — jamE… (20 characters) — and --reveal prints the
token. The substitution happens on the object rather than in the template, so -o json is covered
by the same decision as the table: a masking that only applies to human-readable output protects
nothing, because the pipeline is where the value would have been logged.

The eight licence routes have no index, so "does this address carry a licence" was eight
requests. ip licenses asks them once. Measured: 139.99.131.186/32 carries plesk-ca-75831 and
windows-ca-75830, and everything else on the sample carries none.

ripe set reads before it writes. The API takes the whole RipeInfos object, so a request
carrying only --description would publish an empty netname — in the public registry, without
anybody asking. The record is read and sent back with the changed field replaced.

ip list and ip service list stay two commands because they are two resources: 537 blocks and
80 services. A block is what gets routed; a service is what gets renewed, has contacts and can be
terminated — and one of the eighty cannot, which --filter 'canBeTerminated==false' finds.

One route is deliberately absent: POST /ip/{ip}/terminate, deprecated on 2016-07-21 with
deletion announced for 2017-01-21 and an x-replacement-route pointing at
/ip/service/{serviceName}/terminate — which is what this wires.

Verified against the API

command measured
ip campus 15 sites, their datacenters and the registries each accepts
ip licenses 139.99.131.186/32 2 licences · another block: none
ip ripe get 151.80.69.32/30 OVH_251564576 / Failover Ips
ip delegation list on an IPv6 /64 no delegation set
ip delegation list on an IPv4 /32 the 500, with what the route covers
ip service list --filter 'canBeTerminated==false' the one service of eighty
ip service service-info get contacts, renewal, expiry
ip migration-token get customer named, token withheld — -o json too
ip migration-token get --reveal the token
ip byoip aggregations / slices the API's own "not part of the Bring you own IP product"

Ten refusals executed against the API: an IPv4 delegation, both byoip previews, a ripe set with
nothing to set, a token creation without a customer, a contact change with no contact, an empty
termination token, an unknown termination reason, and both byoip writes without their required
value.

Ten writes proven by --dry-run, including the termination preview, which names every field it
would send and fingerprints the token: token: Supe… (18 characters), reason: TOO_EXPENSIVE.
Nothing was written — the addresses carrying game rules, mitigation and RIPE records belong to
running services.

Notes

  • The termination helpers — the survey enum check, the completion, the token fingerprint — move to
    internal/services/common in the commit below this one. They were written for baremetal in
    feat(baremetal): add the service life cycle, and say what each half of it does #244; the enums are shared API types, and a second copy of a routine that withholds a credential
    does not expire, it stays wrong in silence while the original is fixed. The six existing baremetal
    termination tests are the regression net for the move and pass unchanged.
  • The accepted termination reasons are read from the specification embedded in the binary rather
    than transcribed: fourteen values today, and a list copied into Go drifts the day the API gains a
    fifteenth — silently, into a 400 nobody can explain.

Review round — second commit

An adversarial review of the first commit found five defects, all real.

The delegation note covered IPv4 and not the IPv6 /128 — the one case its own comment names,
quoting the measurement that says the 500 hits every IPv4 mask and every /128. A single address
got a bare 500 and no explanation. delegation get had it worse: it went through the generic
object helper and never reached the explanation at all.

Three guards were bypassable. --size -1 satisfied "a value was given" and travelled to the
API as a prefix length, through a confirmation offering to slice into /-1 blocks. --netname " "
satisfied "something to change" and would have published a blank netname to the public registry,
behind a yes/no. --customer-id " " did the same on a token creation. The termination token was
already trimmed before being tested; these three were not.

byoip aggregate and byoip slice reported a failed read after a refused write, because they
shared the prefix of the two preview routes.

migration-token get blamed a missing token for every failure, sending the operator to
migration-token create after a 403 or a 500 — which fails for the reason that message just hid.

Seven sabotages against the fixes, seven red. The five refusals were also run against the API,
including the /128 that now says what it is:

🛑 failed to read the reverse delegation of 2001:…::77b/128: … (status code 500) …
   This route covers reverse delegation on IPv6 subnets; 2001:…::77b/128 is a single IPv6 address.

Denis-hamon and others added 3 commits August 18, 2026 17:02
…to change

`vps service-info edit myvps --renew-period 12` used to send this:

    {"renew":{"automatic":false,"deleteAtExpiration":false,"forced":false,
              "manualPayment":false,"period":12}}

The renewal settings are booleans bound to a shared struct carrying no
`omitempty`, so every one of them was marshalled at its zero value and won the
merge against the fetched resource. Changing the renewal period therefore also
switched automatic renewal off — on a service that had been renewing itself for
years, without a word in the output saying so.

Webhosting already built its payload from `cmd.Flags().Changed`, and did not
have the defect. This promotes that builder into `common`, so `vps` gets it too
and the next `service-info edit` cannot reintroduce the bug by reaching for the
struct.

Reading `Changed` rather than the values keeps `--renew-automatic=false`
working: pflag records a flag as changed whatever value it was given, so an
explicit false is still sent while an absent flag stays absent. Both cases are
covered by a test, and each test was checked against the failure it exists to
catch.

The shared mutable `ServiceInfoSpec` goes away with the last thing that read
it, and the five flag registrations repeated across four commands become one
call, which also settles the two spellings of the period's help text.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
…ayer

Declaring cobra flags is the command layer's job, and internal/services/common
was the only service package doing it — the shared flag helpers all live in
internal/cmd. Raised in review of this PR.

The registration moves; the table does not. Both halves need the flag name —
one to declare it, the other to read whether the operator set it — and that
name is the only thing tying them together. Splitting it into two copies would
mean a rename could touch one side and leave the other silently no longer
sending a setting, which is the exact failure this PR exists to fix. So
common.ServiceInfoRenewFlags becomes the exported description, internal/cmd
registers from it, and the payload builder keeps reading it.

Checked by renaming an entry in that table and watching the service-info tests
fall: the two halves still move together.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… exit door

This closes the domain. `ip` had 15 of its 81 routes; with the two commits
before this one it has all the live ones.

`licenses` is the shape of the whole lot: the API has one route per product and
no index of them, so "does this address carry a licence" was eight requests. It
is one command, and the eight products are spelled out in the code because
there is nothing else to read them from.

What the account had to be asked, on all 537 of its blocks:

`GET /ip/{ip}/delegation` answers HTTP 500 on 492 of them. The 45 that answer
200 are exactly the IPv6 /56 and /64 blocks; the failures are every IPv4 mask
and every IPv6 /128, with no prefix falling on both sides. A block that does
not exist answers a correct 404, so the 500 is not a not-found, and the sibling
bring-your-own-IP routes show the API can do this properly — they answer 400
"This IP is not part of the Bring you own IP product." The route is summarised
by the API itself as "Reverse delegation on IPv6 subnet", so the error carries
that sentence when the block is IPv4. It is attached to the failure rather than
replacing the call: the scope is the route's documented one, not a rule this
CLI should enforce on the API's behalf.

The migration token is a bearer credential — whoever holds it, with the
customer identifier printed beside it, can take the address — and three of them
were sitting on this account. `migration-token get` prints a fingerprint and
`--reveal` prints the token. The substitution happens on the object rather than
in the template, so -o json is covered by the same decision as the table: a
masking that only applies to human-readable output protects nothing, because
the pipeline is where the value would have been logged.

`ripe set` reads the record before writing it. The API takes the whole object,
so a request carrying only --description would publish an empty netname, in the
public registry, without anybody asking.

And `ip list` and `ip service list` stay two commands because they are two
resources: 537 blocks and 80 services on this account. A block is what gets
routed; a service is what gets renewed, has contacts, and can be terminated —
one of the eighty cannot.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Denis-hamon
Denis-hamon requested a review from a team as a code owner August 19, 2026 14:52
Five defects found by an adversarial review of the previous commit, all real.

The delegation note said what the route covers only for IPv4 blocks, while the
measurement it quotes says the 500 hits every IPv4 mask *and* every IPv6 /128.
So the one case the comment named and the code missed — a single IPv6 address —
got a bare 500 and no explanation. `delegation get` had it worse: it went
through the generic object helper and never reached the explanation at all.

Three guards were bypassable. `--size -1` satisfied "a value was given" and
travelled to the API as a prefix length, through a confirmation offering to
slice into /-1 blocks. `--netname "  "` satisfied "something to change" and
would have published a blank netname to the public registry, behind a yes/no.
`--customer-id "  "` did the same on a token creation. The termination token was
already trimmed before being tested; these three were not.

`byoip aggregate` and `byoip slice` reported that a *read* had failed after a
refused write, because they shared the prefix of the two preview routes.

And `migration-token get` announced "no migration token exists" for every
failure, sending the operator to `migration-token create` after a 403 or a 500 —
which fails for the reason that message just hid. Only a 404 means there is
none.

Seven sabotages against the fixes, seven red. The five refusals were also run
against the API, including the /128 that now says what it is.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Denis-hamon added a commit to Denis-hamon/ovhcloud-cli that referenced this pull request Aug 20, 2026
The IP of the machine these tests were written against ended up in the
fixtures, five times. It is not a secret -- it is in the DNS name of the server
-- but it ties this repository to one account for no benefit, and the same
habit put a live migration token in a test once already (fixed in ovh#257).

Replaced with 203.0.113.7, which RFC 5737 reserves for documentation.

Signed-off-by: Denis <denis.hamon@ovhcloud.com>
@Denis-hamon
Denis-hamon marked this pull request as draft August 20, 2026 13:09
Denis-hamon and others added 4 commits August 20, 2026 16:14
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
A hostname, an address, an order id and a colleague's name read off a live
account had been used as fixtures and as examples in comments. This repository
is public: what goes in stays in. The values are replaced with synthetic ones
of the same shape — RFC 5737 documentation addresses, hostnames built on them,
identifiers of the same length — so the tests keep exercising the same parsing.

This is the fourth time in this series that live account data reached a commit,
after a live IP migration token, a test server's address, and a real invoice
id. The first three were fixed one at a time as they were noticed; this is the
sweep that should have followed the first one.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
@Denis-hamon
Denis-hamon changed the base branch from main to feat/ip-incident-surface August 20, 2026 15:19
Denis-hamon and others added 9 commits August 20, 2026 22:45
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
delegationError carries a note worth keeping — this route serves reverse
delegation on IPv6 subnets, and 492 of the 537 blocks on the account measured
answer HTTP 500 because they are not one. But it hardcoded the verb "read", and
two of its four call sites are writes. A 403 on the POST printed "failed to read
the reverse delegation of X", sending the operator to check a read right they
had never used.

The verb is a parameter now: read, "add to", "remove from". The IPv6 note and
its mask heuristic are untouched.

Also here, because the audit had it down as a defect and it is not: the eight
licence products are a Go list, and the comment saying the API has no index of
them is literally true — there is no enum anywhere in ip.json and no
GET /ip/{ip}/license. What is true is weaker: they are eight paths of the
embedded schema, so the list can drift from the schema silently. A guard test
reads those paths and requires the two to agree; it fails when the schema has no
such path at all, so it cannot pass by measuring nothing.

Two tests on the write paths plus the guard. Two sabotages: dropping a product
from the Go list turns the guard red, pinning the verb back to "read" turns the
write test red.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
EditIpService handed EditResource a copy of IPServiceSpec. On this branch alone
that changes nothing, because the flag-to-field matching does not exist here
yet; once ovh#235 is in, flags match their field by address, and a copy holds none
of those addresses. --sla-monitoring=false and every other flag set to its zero
value would then be dropped silently.

EditIpServiceInfo built the renewal payload inline. Same reasoning: the AST
check ovh#235 adds only recognises the payload through a named variable, as
vps.go already does. Both are no-ops today and stop a real defect from
appearing the moment the two branches meet.

Found by assembling the whole review stack, not by reading either branch.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
ovh#243 moved the Cobra registration of the renewal flags out of
internal/services/common and into the command layer, and exported the
descriptor table so both halves keep reading one list. That commit landed after
this branch had already taken its parent, so this branch still carried the
version where a service package declares flags — and being the more recent side
of the merge, it would have won and quietly undone the refactor.

Merged rather than rebased: nothing is rewritten, so the review threads on this
PR stay attached to their lines.

Signed-off-by: Denis Hamon <denis.hamon@ovhcloud.com>
@Denis-hamon

Copy link
Copy Markdown
Contributor Author

Human review — Does not work

G1 · A migration token is a bearer secret
Dimension: Secrets

Expected: A fingerprint is shown, not the token. --reveal shows it, and the substitution happens on the object — so -o json is masked too.

🛑 no migration token exists for 162.19.234.217/32: OVHcloud API error (status code 404): "The requested object (migrationToken) does not exist" (X-OVH-Query-Id: EU.ext-5.6a8b473c.1090048.2fb32ca664b8709a7693d9287ca3b0a5)
Create one with: ovhcloud ip migration-token create 162.19.234.217/32 --customer-id
exit 1

— Denis (Product Manager), through CLI sandbox review page

@Denis-hamon

Copy link
Copy Markdown
Contributor Author

Follow-up on G1 and G3.

G1 was a false negative, and it belonged to the review sandbox. The check ran against the IP the sandbox retargets everything to, and that address has no pending migration token — hence the 404. Measured across the account on 24/08: 3 of 537 IPs carry a token. Pointing the check at one of them shows exactly what it asks for:

 Key                                 | Value
-------------------------------------|------------------------------------
 Customer                            | ts29436-ovh
 Token                               | 2pDY… (20 characters)

💡 The token is withheld. Print it with --reveal — whoever holds it can claim this IP.

The command is correct: fingerprint by default, --reveal to print, substitution on the object so -o json is masked too. The sandbox now retargets per command rather than sending everything to one machine — a fictitious identifier does not have one real equivalent, it has one per command — and a check whose object exists nowhere on the account is now announced as replay-only, with the reason, instead of returning a 404 the reviewer charges to the CLI.

G3 cannot be judged from the button, and the sandbox now says so before the click. The guard turns every write into --dry-run, and a dry run has no prompt — which is the entire subject of that check. It is judged on the recording.

A --dry-run already carries the parameters in its message, and a
log.Println sitting just above the branch repeated the same JSON behind
a Go timestamp no other command in this CLI emits:

    🔍 Dry run: nothing was sent. This would have been posted to …
    { "operatingSystem": "debian12_64" }
    2026/08/23 23:47:22 Final parameters:
    { "operatingSystem": "debian12_64" }

The log line moves below the branch. A real run still logs what it is
about to send, which is what it was for; a dry run logs nothing, because
it sends nothing.

The line goes to stderr, so no assertion on stdout could ever have seen
it — which is why it survived every green run. Two tests now redirect the
logger: one that a dry run does not log, and its positive control that a
real run still does, so deleting the line outright would not pass.

Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Mechanical, not a change of behaviour. Placing them just after
TestBaremetalReinstallDryRun put them in the one region of
baremetal_test.go that 23 downstream branches also append to, and in the
import block they all touch as well. Merging the parent into those
branches failed 23 times out of 23, every one of them on adjacency
rather than on a disagreement.

baremetal_test.go goes back to what it was before the previous commit,
so this branch now leaves that file untouched. A new file can only clash
with a file of the same name, and nothing else carries this one.

Signed-off-by: Denis <denis.hamon@ovhcloud.com>
Signed-off-by: Denis <denis.hamon@ovhcloud.com>
The warning read: "This registers 192.0.2.0/24 to the organisation RIPE_66451.
The change is published to the regional registry." Both statements are true, and
together they are silent on the only consequence the person typing has to weigh
— the address is no longer theirs afterwards, and getting it back is not their
decision to make.

This is the most irreversible command in the IP tree. Its guard is the strongest
one the CLI has; the sentence it carries should be worth that.

The wording moves into changeOrgWarning() so it can be tested where it is
built, rather than through a dry run: showing the skipped confirmation belongs
to another branch of this stack, and this test has no business depending on it.
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.

1 participant