Attention is currently required from: flichtenheld, plaisthos. MaxF has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/798?usp=email )
Change subject: Implement HKDF expand function based on RFC 8446 ...................................................................... Patch Set 5: -Code-Review (5 comments) Patchset: PS5: The ovpn_expand_label() function looks good to me now. I have some nitpicks but it should work correctly. Also, I really like that you added a test to see that it produces the same result as OpenSSL! File src/openvpn/crypto_epoch.c: http://gerrit.openvpn.net/c/openvpn/+/798/comment/4f1de082_54f44df5 : PS4, Line 67: hmac_ctx_cleanup(hmac_ctx); : hmac_ctx_free(hmac_ctx); > mbed TLS needs both: […] Makes sense! http://gerrit.openvpn.net/c/openvpn/+/798/comment/a592ddf7_ad5c9cd2 : PS4, Line 86: /* 2 byte for the outlen encoded as uint16, 5 bytes for "ovpn " */ : int hkdf_label_len = 2 + 5 + label_len + context_len; : struct buffer hkdf_label = alloc_buf_gc(hkdf_label_len, &gc); : : : buf_write_u16(&hkdf_label, out_len); : buf_write(&hkdf_label, "ovpn ", 5); : buf_write(&hkdf_label, label, label_len); : if (context_len > 0) : { : buf_write(&hkdf_label, context, context_len); : } > Yeah, you are right. […] I agree, it's a good idea to keep it close to the TLS standard. Also makes it easier if someone else is going to review it in the future. The current implementation looks good to me! File src/openvpn/crypto_epoch.c: http://gerrit.openvpn.net/c/openvpn/+/798/comment/2574664f_9fe72c38 : PS5, Line 89: int hkdf_label_len = 2 + 5 + 1 + label_len + 1 + context_len; : struct buffer hkdf_label = alloc_buf_gc(hkdf_label_len, &gc); : : const uint8_t *label_prefix = (const uint8_t *) ("ovpn "); : int prefix_len = 5; You could move the prefix_len declaration up and replace the "5" in hkdf_label_len with it. http://gerrit.openvpn.net/c/openvpn/+/798/comment/effb4ac6_eb381832 : PS5, Line 101: if (context_len > 0) : { : buf_write(&hkdf_label, context, context_len); : } Why do we need this check? buf_write() doesn't do anything if context_len is 0 right? (And if it's needed, why is it not needed for label?) -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/798?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: I3a1c6561f4d9a69e2a441d49dff620b4258a1bcc Gerrit-Change-Number: 798 Gerrit-PatchSet: 5 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: Sat, 23 Nov 2024 00:00:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: plaisthos <arne-open...@rfc2549.org> Comment-In-Reply-To: MaxF <m...@max-fillinger.net> Gerrit-MessageType: comment
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
