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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

Allow overriding ICE ufrag and pwd fields #1201

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca2a98c
Allow overriding ICE ufrag and pwd fields
achingbrain Jun 5, 2024
6f3a7fb
Remove cast to char *
achingbrain Jun 9, 2024
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
4 changes: 4 additions & 0 deletions include/rtc/configuration.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,10 @@ struct RTC_CPP_EXPORT Configuration {
optional<string> certificatePemFile;
optional<string> keyPemFile;
optional<string> keyPemPass;

// Overriding ICE ufrag/pwd
optional<string> iceUfrag;
optional<string> icePwd;
Comment on lines +97 to +99
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 •
edited
Loading

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 •
edited
Loading

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.

};

#ifdef RTC_ENABLE_WEBSOCKET
Expand Down
10 changes: 8 additions & 2 deletions src/impl/icetransport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,16 @@ IceTransport::IceTransport(const Configuration &config, candidate_callback candi
}

// Create agent
mAgent = decltype(mAgent)(juice_create(&jconfig), juice_destroy);
if (!mAgent)
auto agent = juice_create(&jconfig);
if (!agent)
throw std::runtime_error("Failed to create the ICE agent");

if (config.iceUfrag && config.icePwd) {
juice_set_local_ice_attributes(agent, (char*)config.iceUfrag->c_str(), (char*)config.icePwd->c_str());
achingbrain marked this conversation as resolved.
Show resolved Hide resolved
}

mAgent = decltype(mAgent)(agent, juice_destroy);

// Add TURN servers
for (const auto &server : servers)
if (!server.hostname.empty() && server.type != IceServer::Type::Stun)
Expand Down
Loading