Mostly misc cleanups, wireguard dissecting and network packet testing #16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Always store port numbers in n2n_sock_t, since they are basically opaque strings - the only time this needs to change is for presentation to a human. This should fix some observed errors with the data contained within network packets.
This reverts commit e0f506c.
This reverts commit 1edb03b.
This reverts commit e938e78. It appears that this breaks federation with n2n supernode. More investigation required.
The wire encode_* functions are intended to build the packets that will be sent on the wire. These uses of encode_mac() were for structures that would then be passed to another encode_* function, thus they are not building wire structs. Additionally, the encode_mac() degenerates into a string copy, so just do that.
Since these 64bit writes are entirely unstructured random data, they have no difference between their in memory or on the wire encoding. They are also not used in the decode step, so are just acting as a nonce. Removing their use allows cleaner separation of the wire protocol definitions.
Add a function to the random number generator to reseed with a fixed and known seed - this allows us to run any tests that consume random numbers with a known and expected result. Specifically, the transform functions use a prefix of randomness that is basically serving as a nonce. This was previously stable because we didnt seed the random number generator, but adding any new transform functions in the middle would have perturbed the following results - so we use the stable default before each transform. This came to light as the result of changing to use native host ordering for the random nonce bytes - which disturbed the encrypted output.
This reverts commit d6bca21. The github ubuntu builder doesnt have the right packages installed - even though there is a `mk-build-deps -i` command run before the `make dpkg` - this is probably because it is cross-compiling and only the native packages are installed. ``` env -u CFLAGS dpkg-buildpackage -rfakeroot -us -uc --host-type i686-linux-gnu dpkg-buildpackage: info: source package n3n dpkg-buildpackage: info: source version 3.1.1-815-g70a011d-1 dpkg-buildpackage: info: source distribution UNRELEASED dpkg-buildpackage: info: source changed by <builder@example.com> dpkg-source --before-build . dpkg-buildpackage: info: host architecture i386 dpkg-checkbuilddeps: error: Unmet build dependencies: libcap-dev libzstd-dev python3 uncrustify dpkg-buildpackage: warning: build dependencies/conflicts unsatisfied; aborting dpkg-buildpackage: warning: (Use -d flag to override.) ```
The speck code has been generating warnings at compile time for as long
as I can remember. Whilst it may be a valid warning, it does not appear
to have random uninitialised data in the Y[2] as the unit tests are
producing stable results.
Ideally, this warning would be fixed. The simple fix of initialising
the variable could slow down the encryption routine. So - before trying
that - there should be an automated encryption performance test added.
There is also a plan to make more accessible optimised versions of the
crypto routines, which would require refactoring this code, so it may
unearth more warnings or shift the message in a way that helps to
diagnose things.
For the moment, take the simple path of ignoring that warning for this
one file.
```
In file included from /usr/lib/gcc/x86_64-linux-gnu/12/include/xmmintrin.h:1316,
from /usr/lib/gcc/x86_64-linux-gnu/12/include/immintrin.h:31,
from /home/hamish/r/n3n/include/speck.h:75,
from src/speck.c:25:
In function ‘_mm_unpackhi_epi64’,
inlined from ‘speck_encrypt_xor.isra’ at src/speck.c:587:9:
/usr/lib/gcc/x86_64-linux-gnu/12/include/emmintrin.h:1024:19: warning: ‘Y[2]’ may be used uninitialized [-Wmaybe-uninitialized]
1024 | return (__m128i)__builtin_ia32_punpckhqdq128 ((__v2di)__A, (__v2di)__B);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
src/speck.c: In function ‘speck_encrypt_xor.isra’:
src/speck.c:540:16: note: ‘Y[2]’ was declared here
540 | u128 X[4], Y[4], Z[4]; \
| ^
src/speck.c:587:9: note: in expansion of macro ‘Encrypt_Dispatcher’
587 | Encrypt_Dispatcher(128);
| ^~~~~~~~~~~~~~~~~~
```
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
==========================================
- Coverage 46.32% 43.82% -2.50%
==========================================
Files 40 53 +13
Lines 6048 9254 +3206
==========================================
+ Hits 2802 4056 +1254
- Misses 3246 5198 +1952 ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.