Fix regression in secure DiscoverMachine with assigned instances#4123
Open
kensimon wants to merge 1 commit into
Open
Fix regression in secure DiscoverMachine with assigned instances#4123kensimon wants to merge 1 commit into
kensimon wants to merge 1 commit into
Conversation
PR NVIDIA#3561 introduced a regression when securing DiscoverMachine against impersonation: It only supports looking up a machine_interface_address from the connection source IP, not an instance_address. This breaks workflows like machine validation where we run scout on an allocated instance, and thus the source IP will be the instance_address and not the machine_interface_address. Fix this by falling back on a search by instance_addresses if the source IP is not a machine_interface_address, then checking if it belongs to the same machine_id as the provided machine_interface_id. If they don't match, return a permission-denied, if they do, use the caller-provided machine_interface_id (this avoids ambiguity in the event there are multiple interfaces on the machine.)
Contributor
Summary by CodeRabbit
WalkthroughMachine discovery now requires explicit interface IDs for insecure discovery, performs optional remote-IP lookup for secure discovery, and validates fallback interface IDs against the source IP. Database helpers return optional matches, with tests covering same-host, missing, and foreign interfaces. ChangesMachine Discovery Validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant discover_machine
participant MachineInterfaceQueries
Client->>discover_machine: Send discovery request
discover_machine->>MachineInterfaceQueries: Look up interface by remote IP
MachineInterfaceQueries-->>discover_machine: Return interface or no match
discover_machine->>MachineInterfaceQueries: Validate supplied interface against remote IP
MachineInterfaceQueries-->>discover_machine: Return validated interface or no match
discover_machine-->>Client: Return discovery result or validation error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
crates/api-core/src/handlers/machine_discovery.rs (1)
166-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInstrument the impersonation signal, and align the address field name.
Two conventions are missed on this security-relevant path:
- A suspected impersonation attempt is precisely an alert-worthy, countable occurrence — it warrants a declared
carbide_instrument::Event(bounded#[label]s only, withmachine_interface_idand the source address in#[context]) rather than a log line no dashboard can count.- The field is named
remote_iphere butremote_ip_addressat line 117 for the same value, so the two emissions cannot be correlated by a single field name.♻️ Field-name alignment (Event declaration to follow)
tracing::error!( %machine_interface_id, - %remote_ip, + remote_ip_address = %remote_ip, "potential machine impersonation attempt: caller provided machine_interface_id does not belong to this remote IP" );As per coding guidelines: "For countable, rate-based, duration-based, or alert-worthy events, declare one
carbide_instrument::Eventand emit it instead of duplicating metric and log instrumentation" and "Use stable semantic logging field names: … role-specific address/state names … Avoid aliases such as …ip,addr".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/handlers/machine_discovery.rs` around lines 166 - 170, Replace the impersonation-attempt tracing::error! in the machine discovery handler with a declared carbide_instrument::Event, placing machine_interface_id and the source address in #[context] and limiting #[label] fields to bounded values. Name the address field remote_ip_address to match the existing emission, then emit the event on this path instead of duplicating log instrumentation.Source: Coding guidelines
crates/api-core/src/tests/machine_discovery.rs (2)
684-684: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe assertion now contradicts the test name.
Secure discovery does fall back to the caller-provided interface ID; the request fails because that ID does not match the source IP.
test_secure_discovery_does_not_fall_back_to_interface_idwill mislead the next reader into believing the fallback does not exist — rename it to describe the rejection, e.g.test_secure_discovery_rejects_interface_id_from_unrelated_source_ip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/machine_discovery.rs` at line 684, Rename the test containing the PermissionDenied assertion from test_secure_discovery_does_not_fall_back_to_interface_id to a name describing rejection of an interface ID from an unrelated source IP, such as test_secure_discovery_rejects_interface_id_from_unrelated_source_ip; leave the test behavior unchanged.
493-549: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFactor these cloned interface inserts into a small helper. The three
INSERT ... SELECTblocks repeat themachine_interfacescolumn list andinterface_typeliterals; a helper that clones a source row with a requested type would keep this setup in one place and reduce schema drift in the test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-core/src/tests/machine_discovery.rs` around lines 493 - 549, Extract the repeated machine-interface cloning logic into a small test helper near the setup code, taking the source interface ID, MAC address, hostname suffix, and requested interface type as parameters. Update the alternate admin, non-admin, and BMC setup in the surrounding test to call this helper while preserving each insert’s existing values and query behavior.crates/api-db/src/machine_interface.rs (1)
1974-1986: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the dead
find_for_update_by_ipwrapper. Production code already usesfind_optional_for_update_by_ip; only the tests still call this helper, so keep the optional lookup and update the test call sites instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-db/src/machine_interface.rs` around lines 1974 - 1986, Remove the unused find_for_update_by_ip wrapper, leaving find_optional_for_update_by_ip as the supported lookup. Update all test call sites to invoke the optional function and handle the returned Option explicitly, preserving each test’s existing not-found expectations.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/api-core/src/handlers/machine_discovery.rs`:
- Around line 166-170: Replace the impersonation-attempt tracing::error! in the
machine discovery handler with a declared carbide_instrument::Event, placing
machine_interface_id and the source address in #[context] and limiting #[label]
fields to bounded values. Name the address field remote_ip_address to match the
existing emission, then emit the event on this path instead of duplicating log
instrumentation.
In `@crates/api-core/src/tests/machine_discovery.rs`:
- Line 684: Rename the test containing the PermissionDenied assertion from
test_secure_discovery_does_not_fall_back_to_interface_id to a name describing
rejection of an interface ID from an unrelated source IP, such as
test_secure_discovery_rejects_interface_id_from_unrelated_source_ip; leave the
test behavior unchanged.
- Around line 493-549: Extract the repeated machine-interface cloning logic into
a small test helper near the setup code, taking the source interface ID, MAC
address, hostname suffix, and requested interface type as parameters. Update the
alternate admin, non-admin, and BMC setup in the surrounding test to call this
helper while preserving each insert’s existing values and query behavior.
In `@crates/api-db/src/machine_interface.rs`:
- Around line 1974-1986: Remove the unused find_for_update_by_ip wrapper,
leaving find_optional_for_update_by_ip as the supported lookup. Update all test
call sites to invoke the optional function and handle the returned Option
explicitly, preserving each test’s existing not-found expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a06ad880-f24b-4e6d-92b6-5d2df7b4a36b
📒 Files selected for processing (3)
crates/api-core/src/handlers/machine_discovery.rscrates/api-core/src/tests/machine_discovery.rscrates/api-db/src/machine_interface.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR #3561 introduced a regression when securing DiscoverMachine against impersonation: It only supports looking up a machine_interface_address from the connection source IP, not an instance_address. This breaks workflows like machine validation where we run scout on an allocated instance, and thus the source IP will be the instance_address and not the machine_interface_address.
Fix this by falling back on a search by instance_addresses if the source IP is not a machine_interface_address, then checking if it belongs to the same machine_id as the provided machine_interface_id. If they don't match, return a permission-denied, if they do, use the caller-provided machine_interface_id (this avoids ambiguity in the event there are multiple interfaces on the machine.)
Related issues
#4122
Type of Change
Breaking Changes
Testing
Additional Notes