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 1 commit
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) to endpoints that have signaled support for private addresses.
*/
val advertisePrivateCandidates: Boolean by config(
"videobridge.ice.advertise-private-candidates".from(JitsiConfig.newConfig)
Expand Down
2 changes: 1 addition & 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,7 @@ 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, iceControlling, useUniquePort, true, logger, clock)
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment what this "true" means

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,7 @@ class IceTransport @JvmOverloads constructor(
* unique local ports, rather than the configured port.
*/
useUniquePort: Boolean,
private val advertisePrivateAddresses: Boolean,
parentLogger: Logger,
private val clock: Clock = Clock.systemUTC()
) {
Expand Down Expand Up @@ -276,7 +277,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 +513,10 @@ 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 (!(advertisePrivateAddresses && IceConfig.config.advertisePrivateCandidates) &&
transportAddress.isPrivateAddress()
) {
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
3 changes: 2 additions & 1 deletion jvb/src/main/resources/reference.conf
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,8 @@ 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 for endpoints that have signaled support for private addresses.
Copy link
Member

Choose a reason for hiding this comment

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

You made the existing advertise-private-candidates config flag mean "advertise private addresses if jicofo requests it", which changes the current behavior -- I think we want it to be "advertise private candidates even if jicofo doesn't request it" to keep compatibility for community users setting the flag today (e.g. running entirely in a private network).

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