oxidecomputer / oxidecomputer/maghemite

`mgd` panics on overlong MD5 auth key

Open
#765 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
94
Forks
6
Avg merge
1d 8h
Merged PRs (30d)
12

Description

(This fell out of proptesting the new MGD BGP scrimlet reconciler.)

The MGD API defines the MD5 auth key field as an Option<String>: https://github.com/oxidecomputer/maghemite/blob/7696ee48d5ee29a917dea459e281fe2e8ff20513/bgp/src/params.rs#L933

However, internally it assumes that key is at most MAX_MD5SIG_KEYLEN (80) bytes, and if given a key longer than that, we panic on this copy_from_slice on Linux: https://github.com/oxidecomputer/maghemite/blob/7696ee48d5ee29a917dea459e281fe2e8ff20513/bgp/src/connection_tcp.rs#L274-L276

There's no immediately-obvious equivalent on illumos, but it looks like mgd eventually passes the key down to libnet, which makes the same assumption: https://github.com/oxidecomputer/netadm-sys/blob/201992c327b69211c29f097cc767dc2801315fa3/libnet/src/pf_key.rs#L367-L377

I'm not sure what the relationship between mgd and libnet is - maybe libnet needs its own issue and should do validation? But mgd should reject these requests. I don't have a strong feeling whether the API type should remain Option<String> and mgd does validation on that string, or if we should have a stronger type here (Option<Md5AuthKey> or something like that) that does its own validation. The latter seems like a good idea on the surface, at least.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in bgp/params.rs around the MGD API's MD5 auth key and trace its use into bgp/connection_tcp.rs:274-276, where overlong keys panic on Linux. Compare the illumos path through libnet/src/pf_key.rs:367-377. Done means an over-80-byte key is rejected safely rather than reaching a fixed-size copy, with the API or validation approach resolved.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
52/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.