Skip to content
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.

Sign up for GitHub

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

Jump to bottom

API doesn't enforce valid unicast IPs for NIC transit_ips #7530

Open
taspelund opened this issue Feb 12, 2025 · 1 comment · May be fixed by #7559
Open

API doesn't enforce valid unicast IPs for NIC transit_ips #7530

taspelund opened this issue Feb 12, 2025 · 1 comment · May be fixed by #7559
Labels
api Related to the API. bug Something that isn't working. networking Related to the networking.

Comments

@taspelund
Copy link
Contributor

It seems that the NIC Update API doesn't enforce that the networks supplied to transit_ips are actually valid unicast addresses.
For example, I am able to update the NIC to use IPs from the loopback range (127.0.0.0/8) and multicast range (224.0.0.0/4), which I would expect to return an error:

treyaspelund@Tallon-IV 11:03:49 AM | ~/Downloads
‣ ./oxide --profile sandbox instance nic update --interface cba51266-68a8-4f86-a57d-e06a501f6020 --json-body transit_ips_invalid.json
{
  "description": "default primary interface for loopy",
  "id": "cba51266-68a8-4f86-a57d-e06a501f6020",
  "instance_id": "15b9b706-9298-47dd-860c-b40e9107b5cf",
  "ip": "172.30.0.7",
  "mac": "A8:40:25:F9:DE:CE",
  "name": "net0",
  "primary": true,
  "subnet_id": "1a388abc-89db-429e-8b3c-1ca348f1446a",
  "time_created": "2025-02-12T00:44:24.457520Z",
  "time_modified": "2025-02-12T18:05:30.197462Z",
  "transit_ips": [
    "127.0.0.8/32",         <<<<<<<
    "100.64.1.1/24",
    "224.0.0.44/26"      <<<<<<<<
  ],
  "vpc_id": "3d6cead2-2564-4707-9c89-943ddf559b13"
}

treyaspelund@Tallon-IV 11:05:30 AM | ~/Downloads
‣ cat transit_ips_invalid.json
{
  "transit_ips": ["127.0.0.8/32", "100.64.1.1/24", "224.0.0.44/26"]
}

treyaspelund@Tallon-IV 11:07:02 AM | ~/Downloads
‣ ./oxide --profile sandbox instance nic list --instance loopy --project trey-trey-trey
[
  {
    "description": "default primary interface for loopy",
    "id": "cba51266-68a8-4f86-a57d-e06a501f6020",
    "instance_id": "15b9b706-9298-47dd-860c-b40e9107b5cf",
    "ip": "172.30.0.7",
    "mac": "A8:40:25:F9:DE:CE",
    "name": "net0",
    "primary": true,
    "subnet_id": "1a388abc-89db-429e-8b3c-1ca348f1446a",
    "time_created": "2025-02-12T00:44:24.457520Z",
    "time_modified": "2025-02-12T18:05:30.197462Z",
    "transit_ips": [
      "127.0.0.8/32",
      "100.64.1.1/24",
      "224.0.0.44/26"
    ],
    "vpc_id": "3d6cead2-2564-4707-9c89-943ddf559b13"
  }
]

Another, more pedantic, complaint is that we do not enforce each element of transit_ips is the network address (all host bits are set to 0):

treyaspelund@Tallon-IV 11:23:28 AM | ~/Downloads
‣ cat transit_ips.json
{
  "transit_ips": ["100.64.0.0/10", "100.64.0.10/10", "10.0.0.1/32"]
}

treyaspelund@Tallon-IV 11:23:39 AM | ~/Downloads
‣ ./oxide --profile sandbox instance nic update --interface cba51266-68a8-4f86-a57d-e06a501f6020 --json-body transit_ips.json | jq .transit_ips
[
  "100.64.0.0/10",
  "100.64.0.10/10",
  "10.0.0.1/32"
]

In the above example, 100.64.0.0/10 and 100.64.0.10/10 both represent the same network, 100.64.0.0/10, but occupy two elements in the transit_ips array.

It's not possible to tell from the outside whether this configuration might cause errors, whether these values are flattened internally into a single functional element, or how these will be handled by OPTE.
@taspelund taspelund added api Related to the API. bug Something that isn't working. networking Related to the networking. labels Feb 12, 2025
@FelixMcFelix
Copy link
Contributor

Pulling on the thread a little to establish what does happen today, when the CIDRs are being converted from nexus API types into OPTE's we clear any bits outside of the netmask to guarantee they are well-formed. So something misspecified like 192.168.0.255/24 would be converted to 192.168.0.0/24. Not ideal, but the rule functions -- this would however cause problems if we could add and remove them from NICs on live instances and then fail to remove an element because it's actually different in the installed rules.

I agree on the error cases you've outlined above, they shouldn't be too hard to impose on the external API. We might also prohibit overlapping blocks?

FelixMcFelix added a commit that referenced this issue Feb 19, 2025
@FelixMcFelix
Validate transit IPs on network interface update
351a993 
This PR adds in some checks when setting transit IPs on a NIC to ensure
that we have:
* Only unicast addresses.
* No loopback addresses.
* No duplicates.

As discussed in the ticket, violation of any of these isn't going to
leave OPTE or CRDB in a broken state -- just a confusing one for end
users.

Closes #7530.
@FelixMcFelix FelixMcFelix linked a pull request Feb 19, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
api Related to the API. bug Something that isn't working. networking Related to the networking.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Validate transit IPs on network interface update oxidecomputer/omicron
2 participants
@FelixMcFelix @taspelund