Add RFC 7675 Consent Freshness checks#894
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #894 +/- ##
==========================================
- Coverage 88.40% 88.12% -0.29%
==========================================
Files 44 44
Lines 5591 5692 +101
==========================================
+ Hits 4943 5016 +73
- Misses 448 466 +18
- Partials 200 210 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sirzooro
force-pushed
the
ice_consent_checks
branch
2 times, most recently
from
07885ec to
1bd665a
Compare
There was a problem hiding this comment.
Pull request overview
Implements RFC 7675-style consent freshness handling in the ICE agent by tracking consent from existing STUN Binding request/response traffic, enforcing expiry via validateSelectedPair(), and adding support for authenticated inbound STUN Binding Error responses (including 403-driven consent revocation). It also introduces an application hook to emit custom authenticated Binding Error responses for inbound Binding Requests.
Changes:
- Add consent freshness state (
consentFreshnessTimeout,lastConsentAt), default timeout configuration, and expiry enforcement. - Add inbound Binding Error response handling (MESSAGE-INTEGRITY + transaction/destination matching) and 403 consent revocation behavior.
- Add
BindingRequestErrorResponseHandlerandsendBindingError(...)to allow custom authenticated Binding Error responses (with optional extra STUN attributes).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| selection_test.go | Adds tests validating selector behavior when BindingRequestErrorResponseHandler returns a custom Binding Error response. |
| selection.go | Evaluates the new error-response handler before pair creation and sends Binding Error responses when requested; renames pending-success helper usage. |
| gather_test.go | Updates DTLS relay test to use the new DTLS options-style listener API. |
| gather.go | Updates DTLS TURN client creation to the options-style DTLS client API. |
| agent_test.go | Adds tests for authenticated inbound 403 error response consent revocation and consent-expiry validation. |
| agent_options_test.go | Adds coverage for WithConsentFreshnessTimeout and WithBindingRequestErrorResponseHandler. |
| agent_options.go | Introduces WithConsentFreshnessTimeout and WithBindingRequestErrorResponseHandler options. |
| agent_config_test.go | Verifies default consent freshness timeout and that a zero timeout disables expiry. |
| agent_config.go | Adds default consent timeout constant, config fields, and initializes consentFreshnessTimeout. |
| agent.go | Implements consent timestamping/expiry logic, Binding Error sending, and inbound Binding Error response handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| a.getSelector().HandleSuccessResponse(msg, local, remoteCandidate, remote) | ||
|
|
||
| if selectedPair := a.getSelectedPair(); selectedPair != nil && | ||
| selectedPair.Local.Equal(local) && selectedPair.Remote.Equal(remoteCandidate) { | ||
| // Note consent freshness | ||
| a.lastConsentAt = time.Now() | ||
| } | ||
|
|
||
| return true |
There was a problem hiding this comment.
Consent freshness is refreshed for the selected pair on any authenticated Binding success response, even if the TransactionID was unknown / not a response to a pending request (the selector will log and discard, but lastConsentAt is still updated). This can keep consent alive without a valid request/response exchange. Consider updating lastConsentAt only after the response is matched to a pending request and passes the destination/source checks (e.g., move the refresh into HandleSuccessResponse after consumePendingBindingRequest + addr validation, or have HandleSuccessResponse return whether it accepted the response).
| a.getSelector().HandleSuccessResponse(msg, local, remoteCandidate, remote) | |
| if selectedPair := a.getSelectedPair(); selectedPair != nil && | |
| selectedPair.Local.Equal(local) && selectedPair.Remote.Equal(remoteCandidate) { | |
| // Note consent freshness | |
| a.lastConsentAt = time.Now() | |
| } | |
| return true | |
| handled := a.getSelector().HandleSuccessResponse(msg, local, remoteCandidate, remote) | |
| if handled { | |
| if selectedPair := a.getSelectedPair(); selectedPair != nil && | |
| selectedPair.Local.Equal(local) && selectedPair.Remote.Equal(remoteCandidate) { | |
| // Note consent freshness only when the response was accepted | |
| a.lastConsentAt = time.Now() | |
| } | |
| } | |
| return handled |
| now := time.Now() | ||
| if a.consentExpired(now) { | ||
| a.log.Warnf("Consent expired for selected pair after %v without valid response", a.consentFreshnessTimeout) | ||
| a.updateConnectionState(ConnectionStateFailed) | ||
|
|
||
| return false | ||
| } |
There was a problem hiding this comment.
ConsentFreshnessTimeout is enforced via validateSelectedPair(), but the connectivity check ticker interval isn't constrained by consentFreshnessTimeout. If a user configures a consent timeout smaller than the keepalive/check/disconnected/failed intervals, consent expiry may be detected late. Consider factoring consentFreshnessTimeout into the connectivityChecks() timer interval selection (similar to disconnectedTimeout/failedTimeout) so validateSelectedPair() runs often enough to enforce the configured timeout.
| pendingRequest.destination, | ||
| remote, | ||
| ) | ||
|
|
There was a problem hiding this comment.
handleInboundErrorResponse removes the pending binding request via consumePendingBindingRequest before validating that the response came from the expected remote address (addrEqual). If the address check fails, the pending request has already been dropped, so a later valid response for the same transaction would be treated as unknown. Consider only removing the pending request after the address check passes (or add a non-destructive lookup before consuming).
| // The response came from an unexpected address. Re-add the pending | |
| // binding request so that a later valid response for the same | |
| // transaction ID can still be processed correctly. | |
| a.addPendingBindingRequest( | |
| pendingRequest.transactionID, | |
| pendingRequest.destination, | |
| pendingRequest.isUseCandidate, | |
| pendingRequest.nominationValue, | |
| ) |
|
|
||
| a.log.Debugf("Received authenticated STUN error response %d from %s", errorCode.Code, remote) | ||
|
|
||
| return false |
There was a problem hiding this comment.
handleInboundErrorResponse currently returns false for all authenticated error responses (including non-403), which causes handleInbound to return early and skip remoteCandidate.seen(false) / LastReceived updates. This can incorrectly drive disconnected/failed transitions even though authenticated STUN traffic is being received. Consider returning true after successfully validating and accounting for an error response that doesn't immediately fail the agent (and only return false for cases where you intentionally want to abort further handling, like 403 revocation).
| return false | |
| return true |
There was a problem hiding this comment.
Previously code ignored error responses completely. For now it makes sense to return false for both 403 and other errors. I refactored code a bit and added comment about this to make it more clear.
| a.log.Warnf("Received authenticated STUN 403 (Forbidden); revoking consent for %s", remote) | ||
| a.updateConnectionState(ConnectionStateFailed) |
There was a problem hiding this comment.
On authenticated ERROR-CODE 403, the agent transitions to Failed unconditionally. This will tear down the entire ICE agent even if the 403 was in response to a check for a non-selected candidate pair (e.g., triggered checks or optional/renomination pings). Consider scoping consent revocation to the currently selected pair (verify selectedPair.Local/Remote match the tuple for this response) and otherwise treating it as a check failure for just that pair.
| a.log.Warnf("Received authenticated STUN 403 (Forbidden); revoking consent for %s", remote) | |
| a.updateConnectionState(ConnectionStateFailed) | |
| selectedPair := a.getSelectedPair() | |
| if selectedPair != nil && selectedPair.Remote == remoteCandidate { | |
| a.log.Warnf("Received authenticated STUN 403 (Forbidden) on selected candidate pair; revoking consent for %s", remote) | |
| a.updateConnectionState(ConnectionStateFailed) | |
| } else { | |
| a.log.Debugf("Received authenticated STUN 403 (Forbidden) for non-selected remote candidate %s; treating as check failure only", remote) | |
| } |
There was a problem hiding this comment.
This sounds like some race condition scenario. I think it can stay as-is because of this.
| dtls.WithInsecureSkipVerify(a.insecureSkipVerify), //nolint:gosec | ||
| dtls.WithLoggerFactory(a.loggerFactory), | ||
| ) | ||
| if connectErr != nil { | ||
| a.log.Warnf("Failed to create DTLS client: %v", turnServerAddr, connectErr) |
There was a problem hiding this comment.
The Warnf format string has a single %v but two arguments (turnServerAddr, connectErr), which will produce a formatting error in logs. Update the format string to include both values (or interpolate one into the other) so errors are readable.
sirzooro
force-pushed
the
ice_consent_checks
branch
from
1bd665a to
5bbcc74
Compare
Implemented RFC 7675 Consent Freshness checks using existing KeepAlives. Handle inbound 403 Forbidden STUN Binding Error response to immediately revoke consent and transition connection state to Failed. Added application hook to return Binding Error responses for inbound Binding Requests to allow sending 403 Forbidden STUN Binding Error to revoke consent. Hook is generic and allows sending any error with custom attributes.
sirzooro
force-pushed
the
ice_consent_checks
branch
from
5bbcc74 to
6ba75e1
Compare
Summary
This PR implements RFC 7675-style consent handling while keeping the runtime model simple:
validateSelectedPair()usingConsentFreshnessTimeout.403 Forbidden.Motivation
403 Forbidden).What changed
1) Consent freshness state and timeout
consentFreshnessTimeout time.DurationlastConsentAt time.TimeconsentExpired(now time.Time) bool.setSelectedPair()now initializes/clears consent timestamp when pair is set/unset.validateSelectedPair()now fails the connection if consent freshness is expired.2) Consent timestamp updates from existing traffic
lastConsentAtis refreshed.3) Inbound STUN Binding Error response handling
stun.ClassErrorResponsefor Binding messages.handleInboundErrorResponse(...)with:ERROR-CODE 403, connection transitions toFailed(consent revoked).4) Outbound custom Binding Error responses
sendBindingError(...)to emit authenticated STUN Binding Error responses.BindingRequestErrorResponse:ErrorCodeAttribute stun.ErrorCodeAttributeExtraAttributes []stun.SetterAgentConfig.BindingRequestErrorResponseHandlerWithBindingRequestErrorResponseHandler(...)AgentConfig.ConsentFreshnessTimeoutWithConsentFreshnessTimeout(...)5) Selector behavior updates (controlled + controlling)
BindingRequestErrorResponseHandleris evaluated before creating a new pair.handleInboundBindingSuccess->consumePendingBindingRequest.Behavioral impact
30sdefault,0disables expiry).403immediately revokes consent and fails the selected transport path.