[L] Change in openvpn[master]: oob: Add control message TLV encoding (P_CONTROL_OOB_V1)

Attention is currently required from: stipa.

plaisthos has posted comments on this change by stipa. ( http://gerrit.openvpn.net/c/openvpn/+/1741?usp=email )

The change is no longer submittable: Code-Review is unsatisfied now.

Change subject: oob: Add control message TLV encoding (P_CONTROL_OOB_V1)
......................................................................


Patch Set 8: Code-Review-2

(7 comments)

File src/openvpn/oob.c:

http://gerrit.openvpn.net/c/openvpn/+/1741/comment/b45eab83_cea5e37e?usp=email :
PS8, Line 32:  * Consume @p value_len bytes of TLV value, of which @p consumed have already
What is @p? That is not used anywhere else.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/87eaa606_951f189c?usp=email :
PS8, Line 37:  * @param value_len  the TLV's declared value length
declared as in the length in the header or declared as in the spec or as in expected.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/c82b36ce_c84f3b09?usp=email :
PS8, Line 38:  * @param consumed   number of value bytes already read from @p buf
We normally do this by buf_advance instead of this way.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/d7e80cce_f9965cee?usp=email :
PS8, Line 52: bool
Why just have a method that wraps buf_write_u16? That seems uncessary.


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/c74e3f34_d9a3d0be?usp=email :
PS8, Line 91:     *value_len = (uint16_t)len;
I feel instead of passing the same values with pointers again and again we should instead use proper structs


http://gerrit.openvpn.net/c/openvpn/+/1741/comment/a5fbae94_48df0998?usp=email :
PS8, Line 168: }
The formatting of this method is not really great.


File src/openvpn/ssl_pkt.h:

http://gerrit.openvpn.net/c/openvpn/+/1741/comment/34640017_fe9a3866?usp=email :
PS8, Line 70: #define P_CONTROL_OOB_V1 12
That feels like a very hacky way of doing this and also will already create problem with the spec that we written as CONTROL_DATA_V1 (=14) as we then need to change LAST_OPCODE to 14 and then this falls apart.

Also the assumption here that P_CONTROL_OOB_V1 needs to treat specially should be explained here better. What is the harm we consider if we parsed and respond to such packet during a session? I don't see an obvious reason why that is bad.



-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1741?usp=email
To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings?usp=email

Gerrit-MessageType: comment
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I1c8d302ac57c5603d622a7be14be369437388268
Gerrit-Change-Number: 1741
Gerrit-PatchSet: 8
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: stipa <[email protected]>
Gerrit-Comment-Date: Sun, 26 Jul 2026 00:58:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel