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,
--
Antonio Quartulli
OpenVPN Inc.
