[Nice] Compatibility with MS Office Communicator 2007 R2

[Jakub Adam]

Hi Youness,

 > Thank you for quickly doing this! Good job! I reviewed the code, and *WOW*, this
 > is pretty much the first time I review code that I like and have no
 > comments/fixes to request!! Very good job, thanks!

Thanks! I like how git lets me easily change/amend/split/rebase the commits so I can
improve and clean up the code in iterations until everything is in place, nice and
clean with the ugly initial implementation forgotten :)

 > Humm.. actually, I do have one small thing though, but the rest of the code is
 > very good and clean...
 > in priv_map_reply_to_relay_request, you added :
 > +            if (agent->compatibility == NICE_COMPATIBILITY_OC2007 ||
 > +                agent->compatibility == NICE_COMPATIBILITY_OC2007R2) {
 > +                  nice_turn_socket_set_ms_realm(relay_cand->sockptr,
 > &d->stun_message);
 > +                  nice_turn_socket_set_ms_connection_id(relay_cand->sockptr, resp);
 > +            }
 >
 > I was wondering two things.. first, why use d->stun_message for set_ms_realm,
 > but use resp for set_ms_connection_id ?

TURN server does not put REALM in the response, so we must retrieve it from the
request we sent and stored in d->stun_message. And response contains connection ID
we need to store and send in all subsequent TURN requests. That's why we need to
dig information from both request and response.

 > Secondly, why did you have to put this
 > here, why not add that code directly in the turn_socket_parse_recv directly and
 > remove those two functions from the turn_socket API ?

I capture realm and connection id in the moment when successful allocate response
is received and right after we create new relay candidate (conncheck.c:2323).
Moving this to turn_socket_parse_recv() is useless, because there is no suitable
item in Component's local_candidates list yet when we receive this allocate response
so turn_socket_parse_recv() doesn't get called at agent.c:2210.

When we later call priv_map_reply_to_relay_request(), candidate along with the
turn socket is created. Right after this is done, I have to set realm and connection id
in TurnPriv, so these values are available when libnice starts sending TURN Send
requests.

Another solution would be to add d->stun_message and resp to the arguments of
discovery_add_relay_candidate() and nice_turn_socket_new() but that would mean
breaking API which I wanted to avoid. Also these arguments would be useful only
in this one particular compatibility mode.

 > Is it because the realm is not set in the response, but it's only in the request
 > we send?

Yes, we send realm in the request, but response does not return it back. We receive
the realm only in initial 401 allocate error response. The process is shown at
http://msdn.microsoft.com/en-us/library/dd946184%28office.12%29.aspx which probably
is not very different from other dialects of TURN protocol.

 > in which case, we would already have the realm, so why set it again on
 > the turn_socket ? and if there's a reason for that, maybe that's why it's not in
 > parse_recv? because it needs information that the parse_recv doesn't get ?

We have the realm in our request message, but we lost it when the processing of
server's allocate response is over (I did not find any place in the code where
realm is stored somewhere for longer period of time than one request/response
pair, but I could overlook something). Server doesn't send realm value in any
other message after TURN allocation is done, but we need to know its value when
computing MESSAGE-INTEGRITY for our send, set active destination or other further
requests, so I store it in TurnPriv.

Because you did't like adding anything to stun_agent_t, from where cached realm
could be directly accessed in stun_agent_finish_message(), I append REALM attribute
to messages in stun_message_ensure_ms_realm() before calling
stun_agent_finish_message(), so value gets read at stunagent.c:547.

And it is not in turn_socket_parse_recv() for the same reason connection ID is
not there as I explained before. parse_recv() is not called on the message as
we don't have the candidate and socket created yet. It is not created until
priv_map_reply_to_relay_request() is called.

I don't like having those two methods in turn socket API either, but after I walked
again through the code, I still have no idea how to do it without them. Of course
any ideas for improvement are welcome.

 > Anyways, you either explain this to me with something that makes sense (in which
 > case, it would be good to add a comment there to explain why it's done like
 > that.. or you fix it by moving that code to the parse_recv and removing the
 > set_ms_* API from turn_socket...

Hope this explains enough the reasons why I implemented things this way.

In attached patch I put some more comments to the code and also found a small bug
in socket/turn.c where by mistake I put both STUN_AGENT_USAGE_SHORT_TERM_CREDENTIALS
and STUN_AGENT_USAGE_LONG_TERM_CREDENTIALS as usage flags to stun_agent_init().

Jakub