Attention is currently required from: flichtenheld. plaisthos has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/507?usp=email )
Change subject: Implement support for larger packet counter sizes ...................................................................... Patch Set 2: (16 comments) Commit Message: http://gerrit.openvpn.net/c/openvpn/+/507/comment/ad2cfd5e_b853527e : PS2, Line 27: larger packet counters in any scenario since the other scenarios > Maybe nicer "in any other scenario since those are all legacy" Done http://gerrit.openvpn.net/c/openvpn/+/507/comment/d0565850_8921e027 : PS2, Line 38: 2^32 packet ids) forward. But this is an obscure edge that we can > remove second "forward" Done http://gerrit.openvpn.net/c/openvpn/+/507/comment/f6f357ad_2afeb6e6 : PS2, Line 41: Change-Id: I01e258e97351b5aa4b9e561f5b35ddc2318569e2 > Missing sign-off Done File src/openvpn/crypto.h: http://gerrit.openvpn.net/c/openvpn/+/507/comment/aee34092_a4a37691 : PS2, Line 287: /**< Bit-flag indicating that we should use a 64 bit (8 byte) packet > This needs WAY more explanation. […] Done http://gerrit.openvpn.net/c/openvpn/+/507/comment/7ba9075c_7f276f76 : PS2, Line 288: * counter instead of the 32 bit that we normally use. > "normally use" -> "use by default". 32bit will remain the default, but > hopefully not the norm. Done File src/openvpn/crypto.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/7b8ef00b_afc620f9 : PS2, Line 397: const size_t packet_iv_len = iv_len - ctx->implicit_iv_len; > So implicit_iv_len also depends on the longiv flag, but that relationship is > hidden in a completely […] Well we also only store the part of the IV that is needed for filling it to the required 12 bytes. So with short IV we store 8 bytes here and with long IV 4 bytes. I can look into refactoring this or at least writing a better comment. File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/2a9600b7_98e09add : PS2, Line 3304: /* Check if the DCO drivers support the new 64bit packet counter and > missing "TODO"? Currently we do not seem to check this but just assume that > DCO doesn't support it? Yes. There is currently no DCO implementation that has this feature as this PR just adds the feature. But I will will add a stub method that always returns false instead. File src/openvpn/packet_id.h: http://gerrit.openvpn.net/c/openvpn/+/507/comment/fb5fb5ef_e3ecee08 : PS2, Line 250: * Variant of packet_id_read that expect the timestamp first and packet > "expects" Done http://gerrit.openvpn.net/c/openvpn/+/507/comment/b56f9064_b92507d0 : PS2, Line 273: * will always use a variant of the packet id that can just be seens as > "seen" Done http://gerrit.openvpn.net/c/openvpn/+/507/comment/6a732452_444cd0be : PS2, Line 274: * a flat 64 bit counter > add full stop at the end Done http://gerrit.openvpn.net/c/openvpn/+/507/comment/e88332e1_f6640f30 : PS2, Line 277: * @param buf Buffer to write the packet ID too > "too" -> "to" Done http://gerrit.openvpn.net/c/openvpn/+/507/comment/70329f6b_bb92daa0 : PS2, Line 279: * @param prepend If true, prepend to buffer, otherwise append. > Sorry, confusing two different things here. […] Done File src/openvpn/ssl_common.h: http://gerrit.openvpn.net/c/openvpn/+/507/comment/87fa0aeb_65109819 : PS2, Line 363: bool data_v3_features_supported; /**< dco supports new data channel features */ > Why DCO? Doesn't the non-DCO code also support the features? Clarified the doxygen comment File src/openvpn/ssl_ncp.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/0b74fcf6_d3632ce2 : PS2, Line 433: if (session->opt->data_v3_features_supported && (iv_proto_peer & IV_PROTO_DATA_V3)) > since this is based on both the TAG_AT_THE_END and 64_BIT_PKT_ID, maybe would > be better to move this […] The thing here is that I didn't want to introduce two IV_PROTO flags and so the 2nd patch for the two introduces the IV_PROTO_DATA_V3 flag and all the logic for it. File tests/unit_tests/openvpn/test_ssl.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/b11e4b53_74e1e29b : PS2, Line 151: co->flags |= CO_64_BIT_PKT_ID; > why is this required? I don't see any indication that we overwrite co->flags? yeah seems to be old leftover code. Thanks for pointing out. http://gerrit.openvpn.net/c/openvpn/+/507/comment/82c35dbd_ccb22c51 : PS2, Line 494: buf_advance(&buf, 4); > might be nicer to move the this block before the packetid test to avoid the > "+ 4"? Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/507?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I01e258e97351b5aa4b9e561f5b35ddc2318569e2 Gerrit-Change-Number: 507 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arne-open...@rfc2549.org> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Comment-Date: Fri, 09 Feb 2024 14:52:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@lichtenheld.com> Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
