[Nice] Compatibility with MS Office Communicator 2007 R2

[Youness Alaoui]

I see, thanks for all the explanations! I think the real issue is mainly that
the nonce/realm are not cached in the stun agent, and yes, I would understand
modifying stun_agent_t to add the nonce/realm to it. However, since the agent
only creates messages, it cannot know what the destination is.. and the
nonce/realm pair is per-destination IP.. so it wouldn't really work, that's why
it's been left out of stun_agent.

Now I understand better what you're doing with that ms_realm/ms_connection_id..
I didn't really understand that they were the equivalent of the realm/nonce in
standard TURN. Maybe it should have been made into a generic 'realm cache'
rather than a specific 'ms_realm cache'. But that's not important right now, it
can always be improved later if the need arises, and without changing the API.
I'll commit your patch, thanks.

Now I'm just waiting for mlundblad to fix his TURN stuff, then i'll review it
and release the new version of libnice!

Thanks for the contributions! :)

Youness.


On 10/03/2010 02:43 PM, Jakub Adam wrote:
> 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
> 
> 
> 
> _______________________________________________
> Nice mailing list
> Nice at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nice


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/nice/attachments/20101004/f81e8fe0/attachment.pgp>