On 14/11/2024 23:57, Sergey Ryazanov wrote:
On 14.11.2024 10:07, Antonio Quartulli wrote:
On 12/11/2024 17:47, Sabrina Dubroca wrote:
2024-11-09, 03:01:21 +0200, Sergey Ryazanov wrote:
On 29.10.2024 12:47, Antonio Quartulli wrote:
+/* When the OpenVPN protocol is ran in AEAD mode, use
- the OpenVPN packet ID as the AEAD nonce:
- * 00000005 521c3b01 4308c041
- * [seq # ] [ nonce_tail ]
- * [ 12-byte full IV ] -> NONCE_SIZE
- * [4-bytes -> NONCE_WIRE_SIZE
- * on wire]
- */
Nice diagram! Can we go futher and define the OpenVPN packet header as a stucture? Referencing the structure instead of using magic sizes and offsets can greatly improve the code readability. Especially when it comes to header construction/parsing in the encryption/decryption code.
E.g. define a structures like this:
struct ovpn_pkt_hdr { __be32 op; __be32 pktid; u8 auth[]; } __attribute__((packed));
struct ovpn_aead_iv { __be32 pktid; u8 nonce[OVPN_NONCE_TAIL_SIZE]; } __attribute__((packed));
__attribute__((packed)) should not be needed here as the fields in both structs look properly aligned, and IIRC using packed can cause the compiler to generate worse code.
Agreed. Using packed will make certain architecture read every field byte by byte (I remember David M. biting us on this in batman-adv :))
Still curious to see an example of that strange architecture/compiler combination. Anyway, as Sabrina mentioned, the header is already pretty aligned. So it's up to you how to document the structure.
IIRC MIPS was one of those, but don't take my word for granted.
This said, I like the idea of using a struct, but I don't feel confident enough to change the code now that we are hitting v12. This kind of change will be better implemented later and tested carefully. (and patches are always welcome! :))
The main reason behind the structure introduction is to improve the code readability. To reduce a shadow, where bugs can reside. I wonder how many people have invested their time to dig through the encryption preparation function?
As for risk of breaking something I should say that it can be addressed by connecting the kernel implementation to pure usespace implementation, which can be assumed the reference. And, I believe, it worth the benefit of merging easy to understand code.
I understand your point, but this is something I need to spend time on because the openvpn packet format is not "very stable", as in "it can vary depending on negotiated features".
When implementing ovpn I decided what was the supported set of features so to create a stable packet header, but this may change moving forward (there is already some work going on in userspace regarding new features that ovpn will have to support). Therefore I want to take some time thinking about what's best.
Regards,