[Nice] Compatibility with MS Office Communicator 2007 R2

[Youness Alaoui]

On 08/22/2010 07:07 PM, Jakub Adam wrote:
> Hi Youness,
> 
> thanks for your comments on MSOC compatibility patch and sorry for my late
> reply. I'm attaching new version of the patch, reflecting most of the
> issues
> you complained about.

Thanks!

> 
>> - Does OC2007 also have the same bug in its CRC32 function as WLM2009 ?
> 
> Yes, I made few tests and when the WLM2009 fix is not active, 'bad
> fingerprint'
> errors appear in debug log. This supports your theory that MSOC uses the
> very
> same library as WLM.

hehe, yes, I knew it was the same code! Consider yourself lucky I found that
stupid CRC32 error before you hit that error (had to disassemble the dll and
reverse engineer their crc32 function to find that!).

> 
>> - Why not name the compatibility OC2007? Is that 'R2' meaningful in
>> any way?
>> What if there's an R3 release that is also compatible with this (and
>> probably
>> will be for interop) ?
> 
> I think name OC2007R2 is meaningful, because OC2007 (release 1) uses ICEv6
> (IETFDRAFT-ICENAT-06) which is an older version of ICE draft, that, as
> far as
> I know, is not supported by libnice. Please correct me if I'm wrong.

hehe, actually, it is! the "MSN" compatiblity is a derivative of ICE draft 6,
the WLM2009 is the derivative from ICE draft 19.. so basically, the older MSN
version uses the dll from OC2007, and the new WLM2009 used the dll from OC2007R2!


> 
>> - Is there some changes required with regards to TURN? Do you know what
>> specification is supported? Right now, you seem to let it use draft9.
> 
> For now our plugin does not support TURN. MSOC should use Office
> Communications
> Server component called A/V Edge Server that, among other things,
> implements
> TURN server. I was not successful in deploying this component and testing
> the compatibility yet, also after looking in the available documentation it
> seems we will need to implement some nonstandard authentication mechanism
> (in SIPE plugin, not in libnice).
> 
> In the code I changed TURN compatibility to
> STUN_USAGE_TURN_COMPATIBILITY_MSN, as it
> is more likely MSOC uses this dialect and chance is higher no other
> change in libnice
> will be required after we implement all needed parts into our plugin.

I see, actually there are two TURN specifications, one using long term and one
using short term credentials.. I only reverse engineered the old TURN protocol
used by MSN 8.x, and that worked fine.. then I was feeling lazy and didn't want
to reverse engineer/implement the newer TURN specifications used by
WLM2009/OC2007R2, so I just made it use the old servers and use the old
specifications.. it still worked, so I was happy. but I'm pretty sure it's the
same specs, so we'd still need to add support for that sometime in the future
for OC2007R2 and WLM2009 to have the proper TURN support that goes with their
specific protocol revisions.

> 
>> - I don't like the fact that you break the API by adding the
>> candidate_identifier argument to stun_usage_ice_connecheck_create...
>> I'd like to
>> see some other way to do it without breaking the API, although the
>> only solution
>> I see for now is to duplicate the function, which I don't really like
>> either...
>> This will break the API, but I'm afraid we don't really have a choice.
>> Let me
>> know if you have a better idea!
> 
> The doc on stun_usage_ice_conncheck_create is saying that there already are
> some optional arguments (used only with
> STUN_USAGE_ICE_COMPATIBILITY_RFC5245), so
> adding another one seems logical, of course not ideal solution. With
> current
> implementation it seems API has to be broken anytime some compatibility
> mode adds
> a new argument. Idea that came to my mind is to use a (NULL-terminated)
> array of GParameter
> (http://library.gnome.org/devel/gobject/stable/gobject-The-Base-Object-Type.html#GParameter)
> 
> to pass the optional arguments, so the function would change to
> 
> size_t
> stun_usage_ice_conncheck_create (StunAgent *agent, StunMessage *msg,
>     uint8_t *buffer, size_t buffer_len,
>     GParameter* params, StunUsageIceCompatibility compatibility);
> 
> this solution would allow adding another parameters without more API
> breakages.
> 
> Maybe this should be a separate patch.
> 
> Please tell me your opinion, for now I'm leaving the current
> implementation as it is.

Humm, GParameter would make it harder to use the API. You'd need to build the
GParameter everytime before calling the function, I'm not sure it's such a great
idea.. Also, if you noticed, the stun part of the library is completely
independent from glib, there is no glib/gobject use in that library, there's a
reason for that (which I'm not even sure is still valid), so we can't use glib
in 'libstun'.
I think for now, it's safe to break the API, mainly because we have no choice,
secondly because I don't really think that there is much more different ICE
implementations out there, so I don't think there will be more compatibility
modes added that might break the API again.. and finally, because noone (besides
libnice itself) uses stun_usage_ice_conncheck_create! :)


There is just one more thing that I'm thinking about now.. considering that the
OC2007R2 compatibility is the same as the WLM2009 (since you delegate the STUN
compat to WLM2009 even in OC2007R2 mode), and that the only difference is adding
the candidate-identifier (which is harmless to have) and since you're not using
it yourself when receiving data, then I'd like to suggest the following :
1 - add the candidate-identifier in WLM2009 mode (it might require it in the
future if they update their dll)
2 - do a NICE_COMPATIBILITY_OC2007R2 = NICE_COMPATIBILITY_WLM2009
This way you get your compatiblity mode, but less code/paths to maintain, and
it's harmless.
What do you think?


Youness.

-------------- 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/20100823/c0d7a40e/attachment-0001.pgp>