-
Notifications
You must be signed in to change notification settings - Fork 349
openissues
Add compile-time assert commonincludes.h if BOOST_ASSERT is not defined. (Evidently there are older versions of Boost that don't define BOOST_ASSERT?)
Either ship xxd source code or convert the resource compile step to be a perl script. (Who does't have perl installed?)
We never added "unknown attribute" detection, even though we wrote code to handle sending the response back. Not a terrible bug, but something to think about fixing.
If the client request is detected by the server as "legacy rfc 3489" (i.e. magic cookie is not present), we send back the legacy CHANGED-ADDRESS (0x0005) attribute instead of OTHER-ADDRESS (0x802c). But why do we not do the equivalent for SOURCE-ADDRESS (0x0004) and RESPONSE-ORIGIN (0x802b)? Consider adding this for better legacy interop.
For legacy requests, we need to always send back XOR-MAPPED-ADDRESS in addition to plan MAPPED-ADDRESS, even for legacy requests (consistent with Vovida).
Optional-understanding attributes (0x80??) attributes should always follows after the required-understand attributes. ACTUALLY NO! Fingerprint and Message Integrity always come last. I think we got a solution to mirror Vovida behavior.
Seems like a similar handling could be done for legacy "response-address" and the newer "response-port". We could attempt to honor "response-address", by mapping it to anH equivalent "response-port" (but ignore the IP address part of the request)
Got a very ambiguous report from a user that he couldn't get his "sip device" (frizbox) to "log in and register" with the stunserver code base. But when he switched to Vovida, it works. I am deeply confused because he has been very limited in providing details. I suspect he's confusing "sip server" and "stun server". But that doesn't explain why he thinks it works with Vovida's code base. This would be a good opportunity to review how closely our responses mirror Vovida for legacy requests. It would not surprise me if there is stun client code in the wild that expects attributes in a certain order and/or is hardcoded to expect a response formatted in a certain way. Or perhaps there is an auth issue? Consider contacting fritzbox after the holidays.
Now that ITransport thing has been removed, does it make sense for CStunServer to be a refcounted object with a mandatory factory? (Perhaps so...)
RFC 3489 defines all attributes as multiples of 4-bytes. For attributes such as "error-code" that can contain a variable length text string, it explicitly states padding in the attribute string (with spaces) and the attribute length header is always a multiple of 4. But RFC 5389 suggests that attributes not on byte-boundaries are padded, but the attribute header is not adjusted. This creates a compatibility problem with responses such as "error-code". stunserver is sending back back padding without adjusting the attribute length header. This is compliant with 5389, but not compliant with 3489. Fix this such that error-code and attributes with strings send back padded values and length header is always multiple of 4. Ughh....
... this means CStunMessageBuilder::AddAttribute and AddStringAttribute are going to have to be legacy aware. How do we fix this for "nonce", "username, and "realm" attributes? We need to consult RFC 3489 for how to handle this. Some auth types are specific to 5389 and might not matter.
Other attributes that suffer from the four byte alignment inconsistency: UNKNOWN-ATTRIBUTES and RESPONSE-PORT. For Unknown-attributes, RFC 3489 says to repeat one attribute in the list. RESPONSE-PORT did not exist in RFC 3489.
Message-integrity attribute. RFC 3489 says something about padding the message fed to the algorithm to be a multiple of 64 bytes. I don't recall coding anything for this. Probably because it wasn't defined for RFC 5389. Yet another issue to resolve between RFC 3489 and 5389. Might be ok to just say, "auth is not supported for legacy clients".
Man page. At some point, to make this package complete, we're going to have to write a man page. A good suggestion on stackoverflow is to use pandoc. http://stackoverflow.com/questions/7228461/man-page-editor-for-text-screen
Fix csocketaddress::applyxormap to not assume the transaction Id starts with the magic cookie. This is so we can send XOR-mapped-address to legacy clients. Consistent with vovida.
Something very odd about RFC 5780. For evaluating NAT port mapping behavior it describes the 3 tests.
Test 1 - Basic binding request to PP
If not NAT, then "endpoint independent", otherwise run test 2
Test 2 - Binding request to AP
If result is the same as test 1, then "endpoint independent". Otherwise, run test 3
Test 3 - Basic request to AA
If result is the same as test 2, then "address dependent mapping", otherwise, "address and port dependent mapping"
I personally think they got test 2 and test 3 above mixed up. Either that, or they were very deliberate about this order for a reason. Seems like to really confirm true endpoint independent mapping, you'd want test 2 to hit the AA instead of AP. If the result is the same as test 1, then you definitely know that you have endpoint independent mapping instead of some possible type of "dependent mapping".
My intuition tells me this is a better order with an additional test
Test 1 - Basic binding request to PP
If not NAT, then "endpoint independent"
Otherwise run test 2
Test 2 - Binding request to AA (instead of AP)
If result is the same as test 1, then "endpoint independent".
Otherwise run test 3
Test 3 - Binding request to AP (instead of AP)
If the result is completely different from test 1 AND test 2 --> Address and port dependent mapping
If the result is the same as test 2 --> Address dependent mapping
If the result is the same as test 1 --> Port dependent mapping (does this even exist?)
Optional: Run test 4
Test 4 - Binding request to PA
Could be used to further validate which of the dependent mapping schemess are in effect
Perhaps the 4th test isn't warranted. But once you confirm that the NAT does not have endpoint independent mapping, that essentially means that subsequent reflexive candidates obtained through STUN/TURN are unreliable at best for hole punching in P2P with a remote host (who's likely got a completely different IP and port).
I have an email out to the Behave alias about this. I didn't get any responses. Possibly because I botched the subject line and/or because of the holidays. Either way, I'll follow up with them and the RFC authors after the holidays.