ENGDESK-49495: Restrict ICE auto-change to negotiated candidates only

[baloeng]

Summary

Restricts ICE auto-change to only accept STUN Binding Requests from known ICE candidates (from SDP or trickle ICE). STUN from unknown IPs will still receive a response (required by ICE RFC), but will not trigger a change of RTP/DTLS destination.

Problem

A DTLS handshake failure occurred because:

1. Voice SDK sent STUN from a TURN relay IP (64.16.248.199) not included in the SDP
2. b2bua-rtc auto-changed RTP/DTLS destination to that IP
3. Voice SDK responded to DTLS from its direct/srflx IP (102.218.8.246)
4. DTLS handshake failed due to source IP mismatch (security requirement)

Root Cause Analysis

The client's SDP only contained:

• 10.4.248.248:52332 (host - dropped by ACL)
• 102.218.8.246:52332 (srflx - chosen)

No relay candidate was negotiated, yet the client sent STUN from 64.16.248.199:60158 with valid ICE credentials.

Fix

Before allowing auto-change, verify the STUN source IP matches a known candidate:

• If cur_idx >= 0 (match found): proceed with auto-change
• If cur_idx < 0 (no match): log warning, skip auto-change, still send STUN response

Testing

• Verified fix prevents auto-change from unknown IPs
• Verify existing network reconnection scenarios still work (ENGDESK-48822)

Related

• Fixes: ENGDESK-49495
• Related: ENGDESK-48822 (original auto-change feature)

[greptile-apps]

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Additional Comments (1)

src/switch_rtp.c
This loop only checks IP address (from_host) but not port. If a client sends STUN from the same IP with a different port (which is exactly the scenario in the PR description where relay used 64.16.248.199:60158), this will incorrectly match and set cur_idx >= 0, allowing the unwanted auto-change.

The earlier loop at lines 1565-1579 correctly checks both IP and port:

if (!strcmp(...con_addr, from_host) && ...con_port == from_port)

This needs the port check added:

				for (i = 0; i < ice->ice_params->cand_idx[ice->proto]; i++) {
					if (!strcmp(ice->ice_params->cands[i][ice->proto].con_addr, from_host) && ice->ice_params->cands[i][ice->proto].con_port == from_port) {
						cur_idx = i;
					}
				}

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/switch_rtp.c
Line: 1669:1673

Comment:
This loop only checks IP address (`from_host`) but not port. If a client sends STUN from the same IP with a different port (which is exactly the scenario in the PR description where relay used `64.16.248.199:60158`), this will incorrectly match and set `cur_idx >= 0`, allowing the unwanted auto-change.

The earlier loop at lines 1565-1579 correctly checks both IP and port:
```c
if (!strcmp(...con_addr, from_host) && ...con_port == from_port)
```

This needs the port check added:

```suggestion
				for (i = 0; i < ice->ice_params->cand_idx[ice->proto]; i++) {
					if (!strcmp(ice->ice_params->cands[i][ice->proto].con_addr, from_host) && ice->ice_params->cands[i][ice->proto].con_port == from_port) {
						cur_idx = i;
					}
				}
```

How can I resolve this? If you propose a fix, please make it concise.

[damirn]

cur_idx is assigned only in the candidate-search loop inside the else if (!do_adj …) → elapsed_adj > 1000 block — i.e. only on HIT 3/4/5. When do_adj is set earlier by HIT 1 (adj_window) or HIT 2 (switch_cmp_addr same-IP/diff-port, or use_candidate && !is_relay nomination), that loop never runs and cur_idx stays -1, so this guard unconditionally skips auto-change on those paths regardless of whether the source is a known candidate. That's very likely the ENGDESK-48822 reconnection regression. Suggest hoisting the candidate search so cur_idx is computed before this check on every adjust path.

[damirn]

missed_count, rready, and last_ok (further down) were previously set on every do_adj; moving them inside the cur_idx≥0 branch means a peer that keeps sending STUN from an unknown IP never refreshes liveness → can trip ICE timeout/teardown even though we're still answering its binding requests. Suggest keeping rready/last_ok outside this branch and gating only chosen[] + switch_rtp_change_ice_dest.

[damirn]

LOG_WARNING fires on every skipped binding request; STUN retransmits sub-second, so a sustained unknown-IP source will spam the logs. Suggest DEBUG, or log once per source.