Attention is currently required from: flichtenheld, plaisthos. MaxF has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/804?usp=email )
Change subject: Implement methods to generate and manage OpenVPN Epoch keys ...................................................................... Patch Set 9: Code-Review-1 (13 comments) Patchset: PS9: Needs uncrustification. Code looks good to me, I just have some comments about comments and naming. File src/openvpn/crypto.h: http://gerrit.openvpn.net/c/openvpn/+/804/comment/71ce8066_a53680c4 : PS9, Line 307: This limit for AEAD cipher The limit for AEAD ciphers? File src/openvpn/crypto_epoch.c: http://gerrit.openvpn.net/c/openvpn/+/804/comment/4fc082c6_eb80eb11 : PS9, Line 136: 11 We could use strlen() here. http://gerrit.openvpn.net/c/openvpn/+/804/comment/8fdb0547_fcd70ff2 : PS9, Line 157: 8 strlen? http://gerrit.openvpn.net/c/openvpn/+/804/comment/403bd9f7_0bc3e36d : PS9, Line 163: 7 strlen? http://gerrit.openvpn.net/c/openvpn/+/804/comment/769c056a_676d4fc5 : PS9, Line 210: uint16_t current_epoch_recv = co->key_ctx_bi.decrypt.epoch; The variable name makes this look like it should be co->epoch_key_recv.epoch. After reading the function, I think the code is correct, but together with the ASSERT below, this caused me a lot of confusion. I thought that the assert was saying that you can only generate new future keys if you used them all up. http://gerrit.openvpn.net/c/openvpn/+/804/comment/dcaea617_c7f7124b : PS9, Line 423: is in? File src/openvpn/packet_id.h: http://gerrit.openvpn.net/c/openvpn/+/804/comment/7be792f3_fe78b9b2 : PS9, Line 214: */ Incomplete File src/openvpn/packet_id.c: http://gerrit.openvpn.net/c/openvpn/+/804/comment/bf06e613_bc17423a : PS9, Line 81: packet_id_init_recv(struct packet_id_rec *rec, int seq_backtrack, int time_backtrack, const char *name, int unit) You could make this a static function. File tests/unit_tests/openvpn/test_crypto.c: http://gerrit.openvpn.net/c/openvpn/+/804/comment/7799a129_e434234f : PS9, Line 728: is uses uses http://gerrit.openvpn.net/c/openvpn/+/804/comment/e365ffa5_5d07eb01 : PS9, Line 830: 1-5 2-6 http://gerrit.openvpn.net/c/openvpn/+/804/comment/c9e49297_c1a18bdd : PS9, Line 835: assert_int_equal(epoch_lookup_decrypt_key(co, 1)->epoch, 1); Could we make sure that it really is the retiring key by comparing it with &co->epoch_retiring_data_receive_key? Similar for epoch 7 and &co->key_ctx_bi.decrypt http://gerrit.openvpn.net/c/openvpn/+/804/comment/a1b07fcd_530357a4 : PS9, Line 899: assert_null(epoch_lookup_decrypt_key(co, UINT16_MAX - 32)); This hard-codes the assumption that this function is called with a state that has num_future_keys == 32. Can't we get that number from co instead? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/804?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: Id7d6a576ca8c9560cb2dfae82fc62175820e9b80 Gerrit-Change-Number: 804 Gerrit-PatchSet: 9 Gerrit-Owner: plaisthos <arne-open...@rfc2549.org> Gerrit-Reviewer: MaxF <m...@max-fillinger.net> Gerrit-Reviewer: flichtenheld <fr...@lichtenheld.com> Gerrit-CC: openvpn-devel <openvpn-devel@lists.sourceforge.net> Gerrit-Attention: plaisthos <arne-open...@rfc2549.org> Gerrit-Attention: flichtenheld <fr...@lichtenheld.com> Gerrit-Comment-Date: Sun, 29 Dec 2024 00:42:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
