From 61fb38cd8442086596b347b45ef2e4b69f647af2 Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Wed, 17 Nov 2021 10:49:16 +0100 Subject: [PATCH] clean up some error handling error ICE restarts --- .../eu/siacs/conversations/xml/Namespace.java | 1 + .../xmpp/jingle/JingleConnectionManager.java | 2 +- .../xmpp/jingle/JingleRtpConnection.java | 101 +++++++++++------- .../xmpp/jingle/RtpContentMap.java | 7 ++ 4 files changed, 71 insertions(+), 40 deletions(-) diff --git a/src/main/java/eu/siacs/conversations/xml/Namespace.java b/src/main/java/eu/siacs/conversations/xml/Namespace.java index b0c4fe85c..09bbda4cd 100644 --- a/src/main/java/eu/siacs/conversations/xml/Namespace.java +++ b/src/main/java/eu/siacs/conversations/xml/Namespace.java @@ -28,6 +28,7 @@ public final class Namespace { public static final String SYNCHRONIZATION = "im.quicksy.synchronization:0"; public static final String AVATAR_CONVERSION = "urn:xmpp:pep-vcard-conversion:0"; public static final String JINGLE = "urn:xmpp:jingle:1"; + public static final String JINGLE_ERRORS = "urn:xmpp:jingle:errors:1"; public static final String JINGLE_MESSAGE = "urn:xmpp:jingle-message:0"; public static final String JINGLE_ENCRYPTED_TRANSPORT = "urn:xmpp:jingle:jet:0"; public static final String JINGLE_ENCRYPTED_TRANSPORT_OMEMO = "urn:xmpp:jingle:jet-omemo:0"; diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleConnectionManager.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleConnectionManager.java index 6b94f1f4d..cbf4b85fd 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleConnectionManager.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleConnectionManager.java @@ -206,7 +206,7 @@ public class JingleConnectionManager extends AbstractConnectionManager { final Element error = response.addChild("error"); error.setAttribute("type", conditionType); error.addChild(condition, "urn:ietf:params:xml:ns:xmpp-stanzas"); - error.addChild(jingleCondition, "urn:xmpp:jingle:errors:1"); + error.addChild(jingleCondition, Namespace.JINGLE_ERRORS); account.getXmppConnection().sendIqPacket(response, null); } diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java index 26c068c08..1295bf908 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java @@ -306,6 +306,9 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web if (existingCredentials.equals(newCredentials)) { return false; } + //TODO an alternative approach is to check if we already got an iq result to our ICE-restart + // and if that's the case we are seeing an answer. + // This might be more spec compliant but also more error prone potentially final boolean isOffer = rtpContentMap.emptyCandidates(); final RtpContentMap restartContentMap; try { @@ -330,8 +333,8 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web respondOk(jinglePacket); final Throwable rootCause = Throwables.getRootCause(exception); if (rootCause instanceof WebRTCWrapper.PeerConnectionNotInitialized) { - Log.d(Config.LOGTAG, "ignoring PeerConnectionNotInitialized"); - //TODO don’t respond OK but respond with out-of-order + //If this happens a termination is already in progress + Log.d(Config.LOGTAG, "ignoring PeerConnectionNotInitialized on ICE restart"); return true; } Log.d(Config.LOGTAG, "failure to apply ICE restart", rootCause); @@ -484,8 +487,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web private void receiveSessionInitiate(final JinglePacket jinglePacket, final RtpContentMap contentMap) { try { contentMap.requireContentDescriptions(); - //TODO require actpass - contentMap.requireDTLSFingerprint(); + contentMap.requireDTLSFingerprint(true); } catch (final RuntimeException e) { Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": improperly formatted contents", Throwables.getRootCause(e)); respondOk(jinglePacket); @@ -1072,36 +1074,48 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web private synchronized void handleIqResponse(final Account account, final IqPacket response) { if (response.getType() == IqPacket.TYPE.ERROR) { - final String errorCondition = response.getErrorCondition(); - Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": received IQ-error from " + response.getFrom() + " in RTP session. " + errorCondition); - if (isTerminated()) { - Log.i(Config.LOGTAG, id.account.getJid().asBareJid() + ": ignoring error because session was already terminated"); - return; - } - this.webRTCWrapper.close(); - final State target; - if (Arrays.asList( - "service-unavailable", - "recipient-unavailable", - "remote-server-not-found", - "remote-server-timeout" - ).contains(errorCondition)) { - target = State.TERMINATED_CONNECTIVITY_ERROR; - } else { - target = State.TERMINATED_APPLICATION_FAILURE; - } - transitionOrThrow(target); - this.finish(); - } else if (response.getType() == IqPacket.TYPE.TIMEOUT) { - Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": received IQ timeout in RTP session with " + id.with + ". terminating with connectivity error"); - if (isTerminated()) { - Log.i(Config.LOGTAG, id.account.getJid().asBareJid() + ": ignoring error because session was already terminated"); - return; - } - this.webRTCWrapper.close(); - transitionOrThrow(State.TERMINATED_CONNECTIVITY_ERROR); - this.finish(); + handleIqErrorResponse(response); + return; } + if (response.getType() == IqPacket.TYPE.TIMEOUT) { + handleIqTimeoutResponse(response); + } + } + + private void handleIqErrorResponse(final IqPacket response) { + Preconditions.checkArgument(response.getType() == IqPacket.TYPE.ERROR); + final String errorCondition = response.getErrorCondition(); + Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": received IQ-error from " + response.getFrom() + " in RTP session. " + errorCondition); + if (isTerminated()) { + Log.i(Config.LOGTAG, id.account.getJid().asBareJid() + ": ignoring error because session was already terminated"); + return; + } + this.webRTCWrapper.close(); + final State target; + if (Arrays.asList( + "service-unavailable", + "recipient-unavailable", + "remote-server-not-found", + "remote-server-timeout" + ).contains(errorCondition)) { + target = State.TERMINATED_CONNECTIVITY_ERROR; + } else { + target = State.TERMINATED_APPLICATION_FAILURE; + } + transitionOrThrow(target); + this.finish(); + } + + private void handleIqTimeoutResponse(final IqPacket response) { + Preconditions.checkArgument(response.getType() == IqPacket.TYPE.ERROR); + Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": received IQ timeout in RTP session with " + id.with + ". terminating with connectivity error"); + if (isTerminated()) { + Log.i(Config.LOGTAG, id.account.getJid().asBareJid() + ": ignoring error because session was already terminated"); + return; + } + this.webRTCWrapper.close(); + transitionOrThrow(State.TERMINATED_CONNECTIVITY_ERROR); + this.finish(); } private void terminateWithOutOfOrder(final JinglePacket jinglePacket) { @@ -1503,8 +1517,9 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web try { sessionDescription = setLocalSessionDescription(); } catch (final Exception e) { - Log.d(Config.LOGTAG, "failed to renegotiate", e); - //TODO send some sort of failure (comparable to when initiating) + final Throwable cause = Throwables.getRootCause(e); + Log.d(Config.LOGTAG, "failed to renegotiate", cause); + sendSessionTerminate(Reason.FAILED_APPLICATION, cause.getMessage()); return; } final RtpContentMap rtpContentMap = RtpContentMap.of(sessionDescription); @@ -1517,10 +1532,18 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web Log.d(Config.LOGTAG, "received success to our ice restart"); setLocalContentMap(rtpContentMap); webRTCWrapper.setIsReadyToReceiveIceCandidates(true); - } else { - Log.d(Config.LOGTAG, "received failure to our ice restart"); - //TODO ignore tie break (maybe rollback?) - //TODO handle other errors + return; + } + if (response.getType() == IqPacket.TYPE.ERROR) { + final Element error = response.findChild("error"); + if (error != null && error.hasChild("tie-break", Namespace.JINGLE_ERRORS)) { + Log.d(Config.LOGTAG, "received tie-break as result of ice restart"); + return; + } + handleIqErrorResponse(response); + } + if (response.getType() == IqPacket.TYPE.TIMEOUT) { + handleIqTimeoutResponse(response); } }); } diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java index ea351cb1b..21684a165 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java @@ -96,6 +96,10 @@ public class RtpContentMap { } void requireDTLSFingerprint() { + requireDTLSFingerprint(false); + } + + void requireDTLSFingerprint(final boolean requireActPass) { if (this.contents.size() == 0) { throw new IllegalStateException("No contents available"); } @@ -109,6 +113,9 @@ public class RtpContentMap { if (setup == null) { throw new SecurityException(String.format("Use of DTLS-SRTP (XEP-0320) is required for content %s but missing setup attribute", entry.getKey())); } + if (requireActPass && setup != IceUdpTransportInfo.Setup.ACTPASS) { + throw new SecurityException("Initiator needs to offer ACTPASS as setup for DTLS-SRTP (XEP-0320)"); + } } }