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

Allow overriding ICE ufrag and pwd fields #1201

Conversation

achingbrain
Copy link
Contributor

@achingbrain achingbrain commented Jun 5, 2024

Adds two new optional config keys - iceUfrag and icePwd which are passed to libjuice.

Needs a libjuice release with paullouisageneau/libjuice#243

Refs: #1166

Adds two new optional config keys - `iceUfrag` and `icePwd` which
are passed to libjuice.

Refs: paullouisageneau#1166
@achingbrain achingbrain force-pushed the feat/allow-overriding-ice-ufrag-and-pwd branch from 23a03e2 to ca2a98c Compare June 5, 2024 15:14
achingbrain added a commit to achingbrain/node-datachannel that referenced this pull request Jun 6, 2024
paullouisageneau/libjuice#243 allows setting
the ICE ufrag and pwd fields instead of generating random ones every
time.

paullouisageneau/libdatachannel#1201 exposes
config keys to allow setting the fields in libjuice from libdatachannel.

This PR allows setting the fields in libdatachannel from the PeerConnection
constructor.

It will require the two PRs above being merged an shipped before this
is ready for merging.

Refs: paullouisageneau/libdatachannel#1166
achingbrain added a commit to libp2p/js-libp2p that referenced this pull request Jun 7, 2024
achingbrain added a commit to libp2p/js-libp2p that referenced this pull request Jun 7, 2024
src/impl/icetransport.cpp Outdated Show resolved Hide resolved
Comment on lines +97 to +99
// Overriding ICE ufrag/pwd
optional<string> iceUfrag;
optional<string> icePwd;
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a fan of exposing something so low-level in the configuration, as some developers tend to fill everything even if they don't understand what it does, and for instance they could easily mistake the fields for ICE server credentials and unknowingly break security or multiplexing.

There is also a smaller issue because ICE ufrag and pwd would change on renegotiation, and you wouldn't be able to override them from here.

I think these overrides should be set in PeeConnection::gatherLocalCandidates() instead, which is a method for more advanced use cases that requires disableAutoGathering = true. You could add an overload:

struct GatheringConfiguration {
    std::vector<IceServer> additionalIceServers = {};
    
    // Override ICE ufrag/pwd
    optional<string> iceUfrag;
    optional<string> icePwd;
}
void gatherLocalCandidates(GatheringConfiguration gatheringConfig);

The existing method would call this new overload without iceUfrag and icePwd. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand you correctly, the PeerConnection object would need to be created with disableAutoGathering = true and the overloaded gatherLocalCandidates method would need to be called right after setLocalDescription?

I think the more moving parts like this are added, the harder it is for the caller to use the API correctly. The simplest thing to do (from the caller's perspective) would be to allow modifying the local description the same way browsers do but given that seems to be out of the question this seems fine?

Copy link
Owner

Choose a reason for hiding this comment

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

If I understand you correctly, the PeerConnection object would need to be created with disableAutoGathering = true and the overloaded gatherLocalCandidates method would need to be called right after setLocalDescription?

That's right, I admit it's cumbersome. What about an overload of setLocalDescription()? It would be quite straightforward, since from an API standpoint it would be similar to editing the local description between createOffer() and setLocalDescription():

struct LocalDescriptionInit {
    Description::Type type = Description::Type::Unspec;
    optional<string> iceUfrag;
    optional<string> icePwd;
}
void setLocalDescription(LocalDescriptionInit init);

This way it would also allow setting ICE ufrag and pwd on renegotiation.

I think the more moving parts like this are added, the harder it is for the caller to use the API correctly. The simplest thing to do (from the caller's perspective) would be to allow modifying the local description the same way browsers do but given that seems to be out of the question this seems fine?

I understand your concern but adding non-standard low-level settings for very specific use cases like this can also make it hard for most users to use the API correctly, and I'd like to prevent the main config struct from being crowded with them. Allowing the caller to modify the local description is out of the question only because it requires a huge architecture change and involves a lot more complexity only to accommodate non-standard behavior.

Copy link
Contributor Author

@achingbrain achingbrain Jun 10, 2024

Choose a reason for hiding this comment

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

How about passing a LocalDescriptionInit struct that contains the optional iceUfrag/icePwd as a second optional argument to setLocalDescription, similar to the init arg to createDataChannel?

It makes implementation a little simpler and it should be non-breaking that way?

struct LocalDescriptionInit {
    optional<string> iceUfrag;
    optional<string> icePwd;
}
void setLocalDescription(Description::Type type = Description::Type::Unspec, LocalDescriptionInit init = {});

Copy link
Owner

Choose a reason for hiding this comment

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

It looks good to me!

Copy link
Contributor Author

@achingbrain achingbrain Jun 10, 2024

Choose a reason for hiding this comment

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

I've been trying to implement this but it's not straightforward without getting deeply into the internal state of the peer connection.

The general flow is to create a peer connection, open a datachannel, then exchange offers/answers, etc.

I need to be able to set the ufrag/pwd during the initialisation of the ICE transport, which is done as a side-effect of calling setLocalDescription when negotiation is needed.

  • Adding a track or opening a datachannel sets the negotiationNeeded flag
  • I only care about datachannels, adding a track to set the negotiaionNeeded flag seems wrong
  • Opening the datachannel sets the local description with no way to pass the init field in.
  • Setting the local description before opening the datachannel fails because negotiation is not needed yet.
  • Setting the local description after opening the datachannel fails because it expects the next interaction to be the remote's answer and anyway will not re-init the ICE transport

I might be able to skip some of it by setting the negotiationNeeded config option to false but then the calling code is responsible for coordinating the two peers which is very complicated and I think will result in something quite fragile.

I'm now pretty far from the stated aim of just being able to set a ufrag/pwd instead of randomly generating them.

Setting it as a config option as per this PR is much simpler? There's no need to worry about state then, and the concern of developers filling in the field without knowing what it is for can be solved with documentation?

Copy link
Owner

Choose a reason for hiding this comment

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

There should be no need for deep modifications, you only need to set disableAutoNegotiation = true in the config. This way, the library won't call setLocalDescription() internally and you can do so yourself with the init field just after creating the data channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha, I was awaiting the wrong promise during the handshake so everything was grinding to a halt. #1207 implements the local description init arg.

Co-authored-by: Paul-Louis Ageneau <[email protected]>
achingbrain added a commit to achingbrain/libdatachannel that referenced this pull request Jun 11, 2024
Adds an optional second argument to `rtc::PeerConnection::setLocalDescription`
that can contain an ICE ufrag and pwd that if passed will be used in place
of the randomly generated versions.

Refs: paullouisageneau#1166
Refs: paullouisageneau#1201 (comment)
achingbrain added a commit to achingbrain/libdatachannel that referenced this pull request Jun 11, 2024
Adds an optional second argument to `rtc::PeerConnection::setLocalDescription`
that can contain an ICE ufrag and pwd that if passed will be used in place
of the randomly generated versions.

Refs: paullouisageneau#1166
Refs: paullouisageneau#1201 (comment)
@achingbrain
Copy link
Contributor Author

Superseded by #1207

@achingbrain achingbrain deleted the feat/allow-overriding-ice-ufrag-and-pwd branch June 11, 2024 11:00
achingbrain added a commit to achingbrain/libdatachannel that referenced this pull request Jun 13, 2024
Adds an optional second argument to `rtc::PeerConnection::setLocalDescription`
that can contain an ICE ufrag and pwd that if passed will be used in place
of the randomly generated versions.

Refs: paullouisageneau#1166
Refs: paullouisageneau#1201 (comment)

Co-authored-by: Paul-Louis Ageneau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants