fix ice candidate sending when different credentials are used

Daniel Gultsch created

Change summary

src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java         |  19 
src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java               | 150 
src/main/java/eu/siacs/conversations/xmpp/jingle/stanzas/IceUdpTransportInfo.java |   3 
3 files changed, 117 insertions(+), 55 deletions(-)

Detailed changes

src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java 🔗

@@ -33,8 +33,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Queue;
 import java.util.Set;
-import java.util.Timer;
-import java.util.TimerTask;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
@@ -349,16 +347,16 @@ public class JingleRtpConnection extends AbstractJingleConnection
     private boolean checkForIceRestart(
             final JinglePacket jinglePacket, final RtpContentMap rtpContentMap) {
         final RtpContentMap existing = getRemoteContentMap();
-        final IceUdpTransportInfo.Credentials existingCredentials;
+        final Set<IceUdpTransportInfo.Credentials> existingCredentials;
         final IceUdpTransportInfo.Credentials newCredentials;
         try {
             existingCredentials = existing.getCredentials();
-            newCredentials = rtpContentMap.getCredentials();
+            newCredentials = rtpContentMap.getDistinctCredentials();
         } catch (final IllegalStateException e) {
             Log.d(Config.LOGTAG, "unable to gather credentials for comparison", e);
             return false;
         }
-        if (existingCredentials.equals(newCredentials)) {
+        if (existingCredentials.contains(newCredentials)) {
             return false;
         }
         // TODO an alternative approach is to check if we already got an iq result to our
@@ -1849,9 +1847,16 @@ public class JingleRtpConnection extends AbstractJingleConnection
     public void onIceCandidate(final IceCandidate iceCandidate) {
         final RtpContentMap rtpContentMap =
                 isInitiator() ? this.initiatorRtpContentMap : this.responderRtpContentMap;
-        final String ufrag = rtpContentMap.getCredentials().ufrag;
+        final IceUdpTransportInfo.Credentials credentials;
+        try {
+            credentials = rtpContentMap.getCredentials(iceCandidate.sdpMid);
+        } catch (final IllegalArgumentException e) {
+            Log.d(Config.LOGTAG, "ignoring (not sending) candidate: " + iceCandidate, e);
+            return;
+        }
+        final String uFrag = credentials.ufrag;
         final IceUdpTransportInfo.Candidate candidate =
-                IceUdpTransportInfo.Candidate.fromSdpAttribute(iceCandidate.sdp, ufrag);
+                IceUdpTransportInfo.Candidate.fromSdpAttribute(iceCandidate.sdp, uFrag);
         if (candidate == null) {
             Log.d(Config.LOGTAG, "ignoring (not sending) candidate: " + iceCandidate);
             return;

src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java 🔗

@@ -16,7 +16,6 @@ import org.checkerframework.checker.nullness.compatqual.NullableDecl;
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 
 import eu.siacs.conversations.xmpp.jingle.stanzas.Content;
@@ -39,7 +38,8 @@ public class RtpContentMap {
     }
 
     public static RtpContentMap of(final JinglePacket jinglePacket) {
-        final Map<String, DescriptionTransport> contents = DescriptionTransport.of(jinglePacket.getJingleContents());
+        final Map<String, DescriptionTransport> contents =
+                DescriptionTransport.of(jinglePacket.getJingleContents());
         if (isOmemoVerified(contents)) {
             return new OmemoVerifiedRtpContentMap(jinglePacket.getGroup(), contents);
         } else {
@@ -62,22 +62,30 @@ public class RtpContentMap {
     }
 
     public static RtpContentMap of(final SessionDescription sessionDescription) {
-        final ImmutableMap.Builder<String, DescriptionTransport> contentMapBuilder = new ImmutableMap.Builder<>();
+        final ImmutableMap.Builder<String, DescriptionTransport> contentMapBuilder =
+                new ImmutableMap.Builder<>();
         for (SessionDescription.Media media : sessionDescription.media) {
             final String id = Iterables.getFirst(media.attributes.get("mid"), null);
             Preconditions.checkNotNull(id, "media has no mid");
             contentMapBuilder.put(id, DescriptionTransport.of(sessionDescription, media));
         }
-        final String groupAttribute = Iterables.getFirst(sessionDescription.attributes.get("group"), null);
+        final String groupAttribute =
+                Iterables.getFirst(sessionDescription.attributes.get("group"), null);
         final Group group = groupAttribute == null ? null : Group.ofSdpString(groupAttribute);
         return new RtpContentMap(group, contentMapBuilder.build());
     }
 
     public Set<Media> getMedia() {
-        return Sets.newHashSet(Collections2.transform(contents.values(), input -> {
-            final RtpDescription rtpDescription = input == null ? null : input.description;
-            return rtpDescription == null ? Media.UNKNOWN : input.description.getMedia();
-        }));
+        return Sets.newHashSet(
+                Collections2.transform(
+                        contents.values(),
+                        input -> {
+                            final RtpDescription rtpDescription =
+                                    input == null ? null : input.description;
+                            return rtpDescription == null
+                                    ? Media.UNKNOWN
+                                    : input.description.getMedia();
+                        }));
     }
 
     public List<String> getNames() {
@@ -90,7 +98,8 @@ public class RtpContentMap {
         }
         for (Map.Entry<String, DescriptionTransport> entry : this.contents.entrySet()) {
             if (entry.getValue().description == null) {
-                throw new IllegalStateException(String.format("%s is lacking content description", entry.getKey()));
+                throw new IllegalStateException(
+                        String.format("%s is lacking content description", entry.getKey()));
             }
         }
     }
@@ -106,15 +115,24 @@ public class RtpContentMap {
         for (Map.Entry<String, DescriptionTransport> entry : this.contents.entrySet()) {
             final IceUdpTransportInfo transport = entry.getValue().transport;
             final IceUdpTransportInfo.Fingerprint fingerprint = transport.getFingerprint();
-            if (fingerprint == null || Strings.isNullOrEmpty(fingerprint.getContent()) || Strings.isNullOrEmpty(fingerprint.getHash())) {
-                throw new SecurityException(String.format("Use of DTLS-SRTP (XEP-0320) is required for content %s", entry.getKey()));
+            if (fingerprint == null
+                    || Strings.isNullOrEmpty(fingerprint.getContent())
+                    || Strings.isNullOrEmpty(fingerprint.getHash())) {
+                throw new SecurityException(
+                        String.format(
+                                "Use of DTLS-SRTP (XEP-0320) is required for content %s",
+                                entry.getKey()));
             }
             final IceUdpTransportInfo.Setup setup = fingerprint.getSetup();
             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()));
+                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)");
+                throw new SecurityException(
+                        "Initiator needs to offer ACTPASS as setup for DTLS-SRTP (XEP-0320)");
             }
         }
     }
@@ -135,41 +153,66 @@ public class RtpContentMap {
         return jinglePacket;
     }
 
-    RtpContentMap transportInfo(final String contentName, final IceUdpTransportInfo.Candidate candidate) {
+    RtpContentMap transportInfo(
+            final String contentName, final IceUdpTransportInfo.Candidate candidate) {
         final RtpContentMap.DescriptionTransport descriptionTransport = contents.get(contentName);
-        final IceUdpTransportInfo transportInfo = descriptionTransport == null ? null : descriptionTransport.transport;
+        final IceUdpTransportInfo transportInfo =
+                descriptionTransport == null ? null : descriptionTransport.transport;
         if (transportInfo == null) {
-            throw new IllegalArgumentException("Unable to find transport info for content name " + contentName);
+            throw new IllegalArgumentException(
+                    "Unable to find transport info for content name " + contentName);
         }
         final IceUdpTransportInfo newTransportInfo = transportInfo.cloneWrapper();
         newTransportInfo.addChild(candidate);
-        return new RtpContentMap(null, ImmutableMap.of(contentName, new DescriptionTransport(null, newTransportInfo)));
+        return new RtpContentMap(
+                null,
+                ImmutableMap.of(contentName, new DescriptionTransport(null, newTransportInfo)));
     }
 
     RtpContentMap transportInfo() {
         return new RtpContentMap(
                 null,
-                Maps.transformValues(contents, dt -> new DescriptionTransport(null, dt.transport.cloneWrapper()))
-        );
+                Maps.transformValues(
+                        contents,
+                        dt -> new DescriptionTransport(null, dt.transport.cloneWrapper())));
     }
 
-    public IceUdpTransportInfo.Credentials getCredentials() {
-        final Set<IceUdpTransportInfo.Credentials> allCredentials = ImmutableSet.copyOf(Collections2.transform(
-                contents.values(),
-                dt -> dt.transport.getCredentials()
-        ));
-        final IceUdpTransportInfo.Credentials credentials = Iterables.getFirst(allCredentials, null);
+    public IceUdpTransportInfo.Credentials getDistinctCredentials() {
+        final Set<IceUdpTransportInfo.Credentials> allCredentials = getCredentials();
+        final IceUdpTransportInfo.Credentials credentials =
+                Iterables.getFirst(allCredentials, null);
         if (allCredentials.size() == 1 && credentials != null) {
             return credentials;
         }
         throw new IllegalStateException("Content map does not have distinct credentials");
     }
 
+    public Set<IceUdpTransportInfo.Credentials> getCredentials() {
+        final Set<IceUdpTransportInfo.Credentials> credentials =
+                ImmutableSet.copyOf(
+                        Collections2.transform(
+                                contents.values(), dt -> dt.transport.getCredentials()));
+        if (credentials.isEmpty()) {
+            throw new IllegalStateException("Content map does not have any credentials");
+        }
+        return credentials;
+    }
+
+    public IceUdpTransportInfo.Credentials getCredentials(final String contentName) {
+        final DescriptionTransport descriptionTransport = this.contents.get(contentName);
+        if (descriptionTransport == null) {
+            throw new IllegalArgumentException(
+                    String.format(
+                            "Unable to find transport info for content name %s", contentName));
+        }
+        return descriptionTransport.transport.getCredentials();
+    }
+
     public IceUdpTransportInfo.Setup getDtlsSetup() {
-        final Set<IceUdpTransportInfo.Setup> setups = ImmutableSet.copyOf(Collections2.transform(
-                contents.values(),
-                dt -> dt.transport.getFingerprint().getSetup()
-        ));
+        final Set<IceUdpTransportInfo.Setup> setups =
+                ImmutableSet.copyOf(
+                        Collections2.transform(
+                                contents.values(), dt -> dt.transport.getFingerprint().getSetup()));
         final IceUdpTransportInfo.Setup setup = Iterables.getFirst(setups, null);
         if (setups.size() == 1 && setup != null) {
             return setup;
@@ -185,13 +228,18 @@ public class RtpContentMap {
         return count == 0;
     }
 
-    public RtpContentMap modifiedCredentials(IceUdpTransportInfo.Credentials credentials, final IceUdpTransportInfo.Setup setup) {
-        final ImmutableMap.Builder<String, DescriptionTransport> contentMapBuilder = new ImmutableMap.Builder<>();
+    public RtpContentMap modifiedCredentials(
+            IceUdpTransportInfo.Credentials credentials, final IceUdpTransportInfo.Setup setup) {
+        final ImmutableMap.Builder<String, DescriptionTransport> contentMapBuilder =
+                new ImmutableMap.Builder<>();
         for (final Map.Entry<String, DescriptionTransport> content : contents.entrySet()) {
             final RtpDescription rtpDescription = content.getValue().description;
             IceUdpTransportInfo transportInfo = content.getValue().transport;
-            final IceUdpTransportInfo modifiedTransportInfo = transportInfo.modifyCredentials(credentials, setup);
-            contentMapBuilder.put(content.getKey(), new DescriptionTransport(rtpDescription, modifiedTransportInfo));
+            final IceUdpTransportInfo modifiedTransportInfo =
+                    transportInfo.modifyCredentials(credentials, setup);
+            contentMapBuilder.put(
+                    content.getKey(),
+                    new DescriptionTransport(rtpDescription, modifiedTransportInfo));
         }
         return new RtpContentMap(this.group, contentMapBuilder.build());
     }
@@ -200,7 +248,8 @@ public class RtpContentMap {
         public final RtpDescription description;
         public final IceUdpTransportInfo transport;
 
-        public DescriptionTransport(final RtpDescription description, final IceUdpTransportInfo transport) {
+        public DescriptionTransport(
+                final RtpDescription description, final IceUdpTransportInfo transport) {
             this.description = description;
             this.transport = transport;
         }
@@ -215,33 +264,38 @@ public class RtpContentMap {
             } else if (description instanceof RtpDescription) {
                 rtpDescription = (RtpDescription) description;
             } else {
-                throw new UnsupportedApplicationException("Content does not contain rtp description");
+                throw new UnsupportedApplicationException(
+                        "Content does not contain rtp description");
             }
             if (transportInfo instanceof IceUdpTransportInfo) {
                 iceUdpTransportInfo = (IceUdpTransportInfo) transportInfo;
             } else {
-                throw new UnsupportedTransportException("Content does not contain ICE-UDP transport");
+                throw new UnsupportedTransportException(
+                        "Content does not contain ICE-UDP transport");
             }
             return new DescriptionTransport(
-                    rtpDescription,
-                    OmemoVerifiedIceUdpTransportInfo.upgrade(iceUdpTransportInfo)
-            );
+                    rtpDescription, OmemoVerifiedIceUdpTransportInfo.upgrade(iceUdpTransportInfo));
         }
 
-        public static DescriptionTransport of(final SessionDescription sessionDescription, final SessionDescription.Media media) {
+        public static DescriptionTransport of(
+                final SessionDescription sessionDescription, final SessionDescription.Media media) {
             final RtpDescription rtpDescription = RtpDescription.of(sessionDescription, media);
-            final IceUdpTransportInfo transportInfo = IceUdpTransportInfo.of(sessionDescription, media);
+            final IceUdpTransportInfo transportInfo =
+                    IceUdpTransportInfo.of(sessionDescription, media);
             return new DescriptionTransport(rtpDescription, transportInfo);
         }
 
         public static Map<String, DescriptionTransport> of(final Map<String, Content> contents) {
-            return ImmutableMap.copyOf(Maps.transformValues(contents, new Function<Content, DescriptionTransport>() {
-                @NullableDecl
-                @Override
-                public DescriptionTransport apply(@NullableDecl Content content) {
-                    return content == null ? null : of(content);
-                }
-            }));
+            return ImmutableMap.copyOf(
+                    Maps.transformValues(
+                            contents,
+                            new Function<Content, DescriptionTransport>() {
+                                @NullableDecl
+                                @Override
+                                public DescriptionTransport apply(@NullableDecl Content content) {
+                                    return content == null ? null : of(content);
+                                }
+                            }));
         }
     }
 

src/main/java/eu/siacs/conversations/xmpp/jingle/stanzas/IceUdpTransportInfo.java 🔗

@@ -1,5 +1,7 @@
 package eu.siacs.conversations.xmpp.jingle.stanzas;
 
+import androidx.annotation.NonNull;
+
 import com.google.common.base.Joiner;
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Objects;
@@ -123,6 +125,7 @@ public class IceUdpTransportInfo extends GenericTransportInfo {
         }
 
         @Override
+        @NonNull
         public String toString() {
             return MoreObjects.toStringHelper(this)
                     .add("ufrag", ufrag)