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

Added `Message-Integrity` attribute to `Role Conflict` message. #187

Merged
merged 4 commits into from Sep 30, 2019

Conversation

@mstyura
Copy link
Contributor

commented Sep 20, 2019

When ICE role conflict happens, ice4j sends Role Conflict message, which is not liked by both Chrome and Firefox.
Firefox seems to drops such STUN error response, producing following logs:

(ice/INFO) ICE-PEER(PC:1568974392290000 (id=40802189545 url=https://127.0.0.1:9980/):default)/CAND-PAIR(pX/O): setting pair to state IN_PROGRESS: pX/O|IP4:10.0.75.1:60420/UDP|IP4:10.0.75.1:10000/UDP(host(IP4:10.0.75.1:60420/UDP)|candidate:1 1 udp 2130706431 10.0.75.1 10000 typ host generation 0)

(stun/INFO) STUN-CLIENT(pX/O|IP4:10.0.75.1:60420/UDP|IP4:10.0.75.1:10000/UDP(host(IP4:10.0.75.1:60420/UDP)|candidate:1 1 udp 2130706431 10.0.75.1 10000 typ host generation 0)): Received response; processing

(stun/WARNING) Missing MESSAGE-INTEGRITY

(stun/WARNING) STUN-CLIENT(pX/O|IP4:10.0.75.1:60420/UDP|IP4:10.0.75.1:10000/UDP(host(IP4:10.0.75.1:60420/UDP)|candidate:1 1 udp 2130706431 10.0.75.1 10000 typ host generation 0)): Error processing response: Operation rejected, stun error code 0.

Chrome seems to do the same, but silenty.

According to Chrome source code Message-Integrity is mandatory attribute for Role Conflict message.

With this fix, Chrome properly handles Role Conflict response and switched it's role.
Firefox for some reason still unable to handle STUN error response to recover from Role Conflict, but have different message in logs:

(ice/INFO) ICE-PEER(PC:1568984401039000 (id=40802189569 url=https://127.0.0.1:9980/):default)/CAND-PAIR(8hTQ): setting pair to state IN_PROGRESS: 8hTQ|IP4:10.0.75.1:52398/UDP|IP4:10.0.75.1:10000/UDP(host(IP4:10.0.75.1:52398/UDP)|candidate:1 1 udp 2130706431 10.0.75.1 10000 typ host generation 0)

(stun/INFO) STUN-CLIENT(8hTQ|IP4:10.0.75.1:52398/UDP|IP4:10.0.75.1:10000/UDP(host(IP4:10.0.75.1:52398/UDP)|candidate:1 1 udp 2130706431 10.0.75.1 10000 typ host generation 0)): Received response; processing

(stun/WARNING) STUN-CLIENT(8hTQ|IP4:10.0.75.1:52398/UDP|IP4:10.0.75.1:10000/UDP(host(IP4:10.0.75.1:52398/UDP)|candidate:1 1 udp 2130706431 10.0.75.1 10000 typ host generation 0)): Error processing response: Retry may be possible, stun error code 487.

(ice/INFO) ICE-PEER(PC:1568984401039000 (id=40802189569 url=https://127.0.0.1:9980/):default)/CAND-PAIR(zpTK): setting pair to state IN_PROGRESS: zpTK|IP4:10.0.75.1:52398/UDP|IP4:172.19.103.121:10000/UDP(host(IP4:10.0.75.1:52398/UDP)|candidate:2 1 udp 2130706431 172.19.103.121 10000 typ host generation 0)

I've inspected Firefox source code responsible for ICE implementation and was failed to find proper role conflict implementation... There are code bits which handles role conflict, but for some reason they were not executed according to logs :(

@mstyura

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2019

Hello @bbaldino!

Could you please review these change?

Thanks a lot!

Copy link
Member

left a comment

This was a bit hard to review, because you unified the code paths for controlling/controlled in the same commit as the addition of the Message Integrity attribute. But other than my nits I think it's correct.

src/main/java/org/ice4j/ice/Agent.java Outdated Show resolved Hide resolved
@bbaldino

This comment has been minimized.

Copy link
Member

commented Sep 23, 2019

@mstyura would it be possible to add a unit test to validate the conflict case?

@mstyura

This comment has been minimized.

Copy link
Contributor Author

commented Sep 23, 2019

@bbaldino I think it should be possible, I'll try to add it tomorrow. Thank you for review!

@mstyura

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@bbaldino I've added test... I'm not sure I like it, because:

  1. I had to create two full agents and run end-to-end test to trigger role conflict message creation.
  2. Original issue with lack of message integrity attribute is actually not discovered, because there is no message integrity validation at all in responses in ice4j:
    //response
    else if(msg instanceof Response)
    {
    TransactionID tid = ev.getTransactionID();
    StunClientTransaction tran = clientTransactions.remove(tid);
    if(tran != null)
    {
    tran.handleResponse(ev);
    }
    else
    {
    //do nothing - just drop the phantom response.
    logger.fine(
    "Dropped response - no matching client tran found for"
    + " tid " + tid + "\n" + "all tids in stock were "
    + clientTransactions.keySet());
    }
    }

    I thinks it's an issue and need to be fixed, but as separate PR.
@bbaldino

This comment has been minimized.

Copy link
Member

commented Sep 24, 2019

thanks @mstyura , it's unfortunate the test has to be so broad but i do think it's useful, nice job on that and thanks for adding it (even with the caveat).

@mstyura

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@bbaldino can you merge this PR if it's ok?

@mstyura

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2019

Hi, @bbaldino,
Do you think there there is anything else need to be fixed in this PR before it can be approved?
Thanks a lot!

@bbaldino

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

deferred to @JonathanLennox on this one. lgth :)

@bbaldino bbaldino merged commit 7989977 into jitsi:master Sep 30, 2019
1 check passed
1 check passed
default 414 tests run, 286 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.