Enforce MD5 auth string invariants via new type - #792
Conversation
Intoduces Md5AuthString type to wrap a String behind a fallible constructor and immutable getters. This ensures that an API client is unable to configure an MD5 auth key that does not comply with the inputs recommended by RFC 2385 (TCP MD5 option for BGP): ``` 4.5 Key configuration It should be noted that the key configuration mechanism of routers may restrict the possible keys that may be used between peers. It is strongly recommended that an implementation be able to support at minimum a key composed of a string of printable ASCII of 80 bytes or less, as this is current practice. ``` The Illumos and Linux kernel implementations both accept 80-byte keys with arbitrary contents, so the printable ASCII constraint is purely coming from RFC 2385 and the need for keys to round-trip through json via OpenAPI / Dropshot. Fixes: #765 Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
e5f8845 to
7367dc4
Compare
|
The libnet dependency is also bumped in this ticket in order to pull in oxidecomputer/netadm-sys#210. This gives us an additional layer of checking around the MD5 auth keys we submit to the kernel. |
Makes a bunch of the version-suffixed API endpoint handlers into default methods. Updates the calling convention for all the different versioned endpoint handlers such that they each call the next latest version and convert via .into(). Also updates the bgp_apply path to properly walk the conversion path v1 -> v4 -> v8 instead of skipping v4. Also updates the bgp_apply group removal test to call do_bgp_apply with latest rather than using a conversion chain. Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
- Adds schemars bounds (min/max len + regex pattern) for Md5AuthString. - Adds unit tests for converting neighbors between API revs before/after MD5 changes. - Remove unused ApplyRequest conversion (between API revs) - Unify Md5AuthString validation instead of duplicating for num/unnum - Stop cloning a bunch of fields in conversions that consume self - Make Neighbor.into_api_types() infallible Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>
|
@rcgoodfellow @jgallagher I've rebased this on main and applied some fixes found during an AI code review. |
jgallagher
left a comment
jgallagher
left a comment
There was a problem hiding this comment.
Changes LGTM, just a couple small nits.
| enforce_first_as, | ||
| vlan_id, | ||
| ipv4_unicast, | ||
| // dropped: no v1 representation. |
There was a problem hiding this comment.
Is it okay to unconditionally drop these fields, or should this be a try_from that fails if these are populated in a way that can't be represented in v1? (Although if we're on v12 it seems pretty unlikely anything with v1 will come up anyway, so maybe this doesn't matter.)
There was a problem hiding this comment.
At this point the v1 API is getting pretty darn long in the tooth... It doesn't support IPv6 or BGP unnumbered, and it didn't seem like a good idea to completely filter all neighbors from a v1 GET just because a newer API client configured the neighbor with IPv6.
As a whole though, I think this is definitely one of those situations where we (maghemite in particular) have a ton of API versions without consumers that just need to be retired.
I believe the only consumers of mg-api are mgadm, falcon-lab and omicron... And omicron is the only one that doesn't live in this repo. I'd have to check with Justin to see if any of the rkdeploy and similar tools make use of mg-api, but it should be pretty easy to bump their API rev the same way we do with omicron.
There was a problem hiding this comment.
As discussed in matrix, I'll plan to convert these to a fallible conversion rather than stripping out the ipv6 info, then submit a follow-up PR that retires old API versions.
nicolaskagami
left a comment
nicolaskagami
left a comment
There was a problem hiding this comment.
Always happy to see us enforcing invariants through types :)
Changes look good and straightforward, though the amount of boilerplate we need to add is a bit unfortunate.
I'd like to see @jgallagher's points addressed before merging, namely the v1 concern and the HttpError From impl.
Besides that, I noticed that we're changing the types from underneath the Db so we'll lose persisted neighbors that don't have a valid Md5AuthString in get_bgp_neighbors. That's probably fine, but I thought I'd point it out.
4 participants
Intoduces Md5AuthString type to wrap a String behind a fallible
constructor and immutable getters. This ensures that an API client is
unable to configure an MD5 auth key that does not comply with the
inputs recommended by RFC 2385 (TCP MD5 option for BGP):
The Illumos and Linux kernel implementations both accept 80-byte keys
with arbitrary contents, so the printable ASCII constraint is purely
coming from RFC 2385 and the need for keys to round-trip through json
via OpenAPI / Dropshot.
Fixes: #765