Attention is currently required from: plaisthos. flichtenheld 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: (8 comments) Patchset: PS2: Some more comments now that I understand the code a bit better File src/openvpn/crypto.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/373e1697_ed598058 : 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 separate part of the code? IMHO that is very confusing. Wouldn't it be easier if we just fill the implicit_iv fully always and check how much of it to use here? It think that would make the code simpler and easier to understand. File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/db73a5bd_f5a88cfb : 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? File src/openvpn/packet_id.h: http://gerrit.openvpn.net/c/openvpn/+/507/comment/9644908d_23b0dda7 : PS2, Line 279: * @param prepend If true, prepend to buffer, otherwise append. > prepend parameter does not exist in this variant. […] Sorry, confusing two different things here. It does not exist because it would be always false anyway in the places where this is used. Still needs to be removed from documentation, though. File src/openvpn/ssl_common.h: http://gerrit.openvpn.net/c/openvpn/+/507/comment/612bae7e_d66dfc56 : 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? File src/openvpn/ssl_ncp.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/88a16718_ea9da9d3 : 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 to its own patch instead of mixing it into the existing two? File tests/unit_tests/openvpn/test_ssl.c: http://gerrit.openvpn.net/c/openvpn/+/507/comment/c0271a29_2d76ebf0 : 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? http://gerrit.openvpn.net/c/openvpn/+/507/comment/581f83d7_d1986ad4 : PS2, Line 494: buf_advance(&buf, 4); might be nicer to move the this block before the packetid test to avoid the "+ 4"? -- 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: plaisthos <arne-open...@rfc2549.org> Gerrit-Comment-Date: Mon, 05 Feb 2024 12:24:14 +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
