clean up some error handling error ICE restarts

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";

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);
     }
 

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");
+                //If this happens a termination is already in progress
-                //TODO don’t respond OK but respond with out-of-order
+                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(true);
-            contentMap.requireDTLSFingerprint();
         } 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();
+            handleIqErrorResponse(response);
-            Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": received IQ-error from " + response.getFrom() + " in RTP session. " + errorCondition);
+            return;
-            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();
         }
+        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);
+            final Throwable cause = Throwables.getRootCause(e);
-            //TODO send some sort of failure (comparable to when initiating)
+            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 {
+                return;
-                Log.d(Config.LOGTAG, "received failure to our ice restart");
+            }
-                //TODO ignore tie break (maybe rollback?)
+            if (response.getType() == IqPacket.TYPE.ERROR) {
-                //TODO handle other errors
+                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);
             }
         });
     }

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)");
+            }
         }
     }