Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable private addresses for ICE based on a per-endpoint flag. #2047

Merged
merged 3 commits into from
Sep 13, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions jvb/src/main/java/org/jitsi/videobridge/Conference.java
Original file line number Diff line number Diff line change
Expand Up @@ -722,15 +722,17 @@ public Endpoint createLocalEndpoint(
boolean iceControlling,
boolean sourceNames,
boolean doSsrcRewriting,
boolean visitor)
boolean visitor,
boolean privateAddresses)
{
final AbstractEndpoint existingEndpoint = getEndpoint(id);
if (existingEndpoint != null)
{
throw new IllegalArgumentException("Local endpoint with ID = " + id + "already created");
}

final Endpoint endpoint = new Endpoint(id, this, logger, iceControlling, sourceNames, doSsrcRewriting, visitor);
final Endpoint endpoint = new Endpoint(
id, this, logger, iceControlling, sourceNames, doSsrcRewriting, visitor, privateAddresses);
videobridge.localEndpointCreated(visitor);

subscribeToEndpointEvents(endpoint);
Expand Down
3 changes: 2 additions & 1 deletion jvb/src/main/kotlin/org/jitsi/videobridge/Endpoint.kt
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class Endpoint @JvmOverloads constructor(
* Whether this endpoint is in "visitor" mode, i.e. should be invisible to other endpoints.
*/
override val visitor: Boolean,
supportsPrivateAddresses: Boolean,
private val clock: Clock = Clock.systemUTC()
) : AbstractEndpoint(conference, id, parentLogger),
PotentialPacketHandler,
Expand All @@ -127,7 +128,7 @@ class Endpoint @JvmOverloads constructor(
private val dataChannelHandler = DataChannelHandler()

/* TODO: do we ever want to support useUniquePort for an Endpoint? */
private val iceTransport = IceTransport(id, iceControlling, false, logger)
private val iceTransport = IceTransport(id, iceControlling, false, supportsPrivateAddresses, logger)
private val dtlsTransport = DtlsTransport(logger).also { it.cryptex = CryptexConfig.endpoint }

private var cryptex: Boolean = CryptexConfig.endpoint
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,12 +175,14 @@ class Colibri2ConferenceHandler(
)
val sourceNames = c2endpoint.hasCapability(Capability.CAP_SOURCE_NAME_SUPPORT)
val ssrcRewriting = sourceNames && c2endpoint.hasCapability(Capability.CAP_SSRC_REWRITING_SUPPORT)
val privateAddresses = c2endpoint.hasCapability(Capability.CAP_PRIVATE_ADDRESS_CONNECTIVITY)
conference.createLocalEndpoint(
c2endpoint.id,
transport.iceControlling,
sourceNames,
ssrcRewriting,
c2endpoint.mucRole == MUCRole.visitor
c2endpoint.mucRole == MUCRole.visitor,
privateAddresses
).apply {
c2endpoint.statsId?.let {
statsId = it
Expand Down
2 changes: 1 addition & 1 deletion jvb/src/main/kotlin/org/jitsi/videobridge/ice/IceConfig.kt
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class IceConfig private constructor() {

/**
* Whether to advertise ICE candidates with private IP addresses (RFC1918 IPv4 addresses and
* fec0::/10 or fc00::/7 IPv6 addresses).
* fec0::/10 or fc00::/7 IPv6 addresses) even to endpoints that have not signaled support for private addresses.
*/
val advertisePrivateCandidates: Boolean by config(
"videobridge.ice.advertise-private-candidates".from(JitsiConfig.newConfig)
Expand Down
11 changes: 10 additions & 1 deletion jvb/src/main/kotlin/org/jitsi/videobridge/relay/Relay.kt
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,16 @@ class Relay @JvmOverloads constructor(
private val sctpHandler = SctpHandler()
private val dataChannelHandler = DataChannelHandler()

private val iceTransport = IceTransport(id, iceControlling, useUniquePort, logger, clock)
private val iceTransport = IceTransport(
id = id,
controlling = iceControlling,
useUniquePort = useUniquePort,
// There's no good reason to disable private addresses.
advertisePrivateAddresses = true,
parentLogger = logger,
clock = clock
)

private val dtlsTransport = DtlsTransport(logger).also { it.cryptex = CryptexConfig.relay }

private var cryptex = CryptexConfig.relay
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ class IceTransport @JvmOverloads constructor(
* unique local ports, rather than the configured port.
*/
useUniquePort: Boolean,
/**
* Use private addresses for this [IceTransport] even if [IceConfig.advertisePrivateCandidates] is false.
*/
private val advertisePrivateAddresses: Boolean,
parentLogger: Logger,
private val clock: Clock = Clock.systemUTC()
) {
Expand Down Expand Up @@ -276,7 +280,7 @@ class IceTransport @JvmOverloads constructor(
password = iceAgent.localPassword
ufrag = iceAgent.localUfrag
iceComponent.localCandidates?.forEach { cand ->
cand.toCandidatePacketExtension()?.let { pe.addChildExtension(it) }
cand.toCandidatePacketExtension(advertisePrivateAddresses)?.let { pe.addChildExtension(it) }
}
addChildExtension(IceRtcpmuxPacketExtension())
}
Expand Down Expand Up @@ -512,8 +516,11 @@ private fun generateCandidateId(candidate: LocalCandidate): String = buildString
append(java.lang.Long.toHexString(candidate.hashCode().toLong()))
}

private fun LocalCandidate.toCandidatePacketExtension(): CandidatePacketExtension? {
if (!IceConfig.config.advertisePrivateCandidates && transportAddress.isPrivateAddress()) {
private fun LocalCandidate.toCandidatePacketExtension(advertisePrivateAddresses: Boolean): CandidatePacketExtension? {
if (transportAddress.isPrivateAddress() &&
!advertisePrivateAddresses &&
!IceConfig.config.advertisePrivateCandidates
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this logic is wrong?

return null
}
val cpe = IceCandidatePacketExtension()
Expand Down
4 changes: 3 additions & 1 deletion jvb/src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,9 @@ videobridge {
# "NominateFirstValid", "NominateHighestPriority", "NominateFirstHostOrReflexiveValid", or "NominateBestRTT".
nomination-strategy = "NominateFirstHostOrReflexiveValid"

# Whether to advertise private ICE candidates, i.e. RFC 1918 IPv4 addresses and fec0::/10 and fc00::/7 IPv6 addresses.
# Whether to advertise private ICE candidates, i.e. RFC 1918 IPv4 addresses and fec0::/10 and fc00::/7 IPv6
# addresses even for endpoints that have not signaled support for private addresses.
# Note: Jicofo signals support for private addresses for jigasi and jibri.
advertise-private-candidates = true
}

Expand Down
3 changes: 2 additions & 1 deletion jvb/src/test/kotlin/org/jitsi/videobridge/ConferenceTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ class ConferenceTest : ConfigTest() {
context("Adding local endpoints should work") {
with(Conference(videobridge, "id", name, null, false)) {
endpointCount shouldBe 0
createLocalEndpoint("abcdabcd", true, false, false, false) // TODO cover the case when they're true
// TODO cover the case when they're true
createLocalEndpoint("abcdabcd", true, false, false, false, false)
endpointCount shouldBe 1
debugState.shouldBeValidJson()
}
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>jitsi-xmpp-extensions</artifactId>
<version>1.0-72-gc9dde6c</version>
<version>1.0-74-gc88e006</version>
</dependency>
</dependencies>
</dependencyManagement>
Expand Down
Loading