Attention is currently required from: MaxF, flichtenheld. plaisthos 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 4: (5 comments) Patchset: PS4: Thanks for the review. File src/openvpn/crypto_epoch.h: http://gerrit.openvpn.net/c/openvpn/+/798/comment/d8777d7c_f373c906 : PS4, Line 32: * - salt is always assumed to be zero length (ie not supported) : * - IKM (secret) is assumed to be always 32 bytes > salt and IKM are arguments for HKDF-Extract, not HKDF-Expand. You are right. I will remove those as they are not even parameters and we don't use HDKF-Extract at all. File src/openvpn/crypto_epoch.c: http://gerrit.openvpn.net/c/openvpn/+/798/comment/496fe9e5_57694e82 : PS4, Line 67: hmac_ctx_cleanup(hmac_ctx); : hmac_ctx_free(hmac_ctx); > Not exactly about this commit itself, but looking at these functions, it's > not clear to me why we ne […] mbed TLS needs both: void md_ctx_cleanup(mbedtls_md_context_t *ctx) { mbedtls_md_free(ctx); } void hmac_ctx_free(mbedtls_md_context_t *ctx) { free(ctx); } They do different things and you need both otherwise you leave memory behind. http://gerrit.openvpn.net/c/openvpn/+/798/comment/38a2fb96_8e7628d3 : PS4, Line 83: } > We could check label_len and context_len too: […] Acknowledged http://gerrit.openvpn.net/c/openvpn/+/798/comment/5ded05ce_c4d2d7a5 : 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); : } > That's not exactly how this works. […] Hm, I know what you are saying, at the same time, TLS 1.3 also doesn't add that extra byte for the context length: https://datatracker.ietf.org/doc/html/rfc8446#section-7.1 I think the label and context is rather theorectical since we a) always have the same label and b) don't use context at all. So I can just remove the possibility to use context from this function if that function if that makes things better. This was intended to do the same as TLS 1.3 just with "ovpn " instead of "tls13 " I think what TLS RFC fails to mention and I also fail to mention is that you this label construction is a bit brittle in the sense that you have described, ie. you need to ensure that all the labels you are using this function with are not prefixes of each other. -- 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: 4 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: flichtenheld <fr...@lichtenheld.com> Gerrit-Attention: MaxF <m...@max-fillinger.net> Gerrit-Comment-Date: Wed, 20 Nov 2024 19:56:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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
