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

twamp: test packet size too small #31

Closed
vvidic opened this Issue Feb 17, 2018 · 6 comments

Comments

Projects
None yet
4 participants
@vvidic
Contributor

vvidic commented Feb 17, 2018

Packet size should be the same in both directions (41 bytes if no padding), but we are sending much less in request packets:

23:00:06.435012 IP 193.198.241.30.20862 > 193.62.143.100.20862: UDP, length 14
23:00:06.471514 IP 193.62.143.100.20862 > 193.198.241.30.20862: UDP, length 41
23:00:06.471593 IP 193.198.241.30.20862 > 193.62.143.100.20862: UDP, length 14
23:00:06.508119 IP 193.62.143.100.20862 > 193.198.241.30.20862: UDP, length 41
23:00:06.564528 IP 193.198.241.30.20862 > 193.62.143.100.20862: UDP, length 14
23:00:06.601050 IP 193.62.143.100.20862 > 193.198.241.30.20862: UDP, length 41
23:00:06.602768 IP 193.198.241.30.20862 > 193.62.143.100.20862: UDP, length 14
23:00:06.639293 IP 193.62.143.100.20862 > 193.198.241.30.20862: UDP, length 41

This is causing the wireshark dissector to report these as malformed packets.

vvidic added a commit that referenced this issue Feb 19, 2018

Padding depends on auth mode, so configure auth before padding #31
Also default to open mode only if no username is set.
@vvidic

This comment has been minimized.

Contributor

vvidic commented Feb 19, 2018

It seems the packets are not the same in the end. Client sents OWAMP-Test (size 14) and the server replies with TWAMP-Test (size 41). However the RFC5357 says the smaller packets may be padded in order to get the same size in both directions:

   Note that the Reflector test packet formats are larger than the
   Sender's formats.  The Session-Sender MAY append sufficient Packet
   Padding to allow the same IP packet payload lengths to be used in
   each direction of transmission (this is usually desirable).  To
   compensate for the Reflector's larger test packet format, the Sender
   appends at least 27 octets of padding in unauthenticated mode, and at
   least 56 octets in authenticated and encrypted modes.

The code for padding was already there but did not work correctly because auth mode was not set correctly.

@vvidic vvidic self-assigned this Feb 19, 2018

@vvidic vvidic added the bug label Feb 19, 2018

@vvidic vvidic added this to the sprint-20180226 milestone Feb 19, 2018

tonin added a commit that referenced this issue Feb 19, 2018

Merge pull request #32 from perfsonar/twamp-packetsize
twamp: padding depends on auth mode, so configure auth before padding #31
@tonin

This comment has been minimized.

Member

tonin commented Feb 21, 2018

This needs to be properly tested with OWAMP deployments too.

@tonin tonin self-assigned this Feb 21, 2018

@tonin tonin added the help wanted label Feb 21, 2018

@vvidic

This comment has been minimized.

Contributor

vvidic commented Feb 22, 2018

From my testing the packet size for owping does not change from 14 by this patch:

12:13:11.753677 IP 161.53.160.238.9747 > 193.198.241.30.9006: UDP, length 14
12:13:11.758480 IP 161.53.160.238.9747 > 193.198.241.30.9006: UDP, length 14
12:13:11.763926 IP 161.53.160.238.9747 > 193.198.241.30.9006: UDP, length 14
12:13:11.849438 IP 161.53.160.238.9747 > 193.198.241.30.9006: UDP, length 14
@tonin

This comment has been minimized.

Member

tonin commented Feb 22, 2018

Ok, thanks for the tests, that is already a good sign. What we also need to check is that the different authentication modes are still working fine. IIRC @erik-geant had added unit tests about that, but we also need to test it between 2 real (O|T)WAMP instances.

@arlake228

This comment has been minimized.

Contributor

arlake228 commented Aug 6, 2018

Can this be closed? It appears to have been merged and we have seen no issues with TWAMP.

@erik-geant

This comment has been minimized.

Contributor

erik-geant commented Aug 7, 2018

yes, the unit test are still passing ... closing

@erik-geant erik-geant closed this Aug 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment