Menu
▾
▴
openvpn-devel
|
From: plaisthos (C. Review) <ge...@op...> - 2025-04-09 11:23:56
|
Attention is currently required from: flichtenheld.
Hello flichtenheld,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/938?usp=email
to review the following change.
Change subject: Also print key agreement when printing negotiated details
......................................................................
Also print key agreement when printing negotiated details
With TLS 1.0 to 1.2, the used key agreement was depended on the certificates
themselves. With TLS 1.3 is no longer the case but basically always X25519
was used. So this information has been very interesting so far. But with
OpenSSL 3.5.0 and the new X25519MLKEM768 hybrid key agreement, the used
key agreement group actually becomes interesting information.
This commit adds printing this information for OpenSSL 3.0.0+ and uses
a compat version for OpenSSL 3.0-3.1 to avoid an additional ifdef in the
code itself.
Example output with ML-DSA-65 certificates on the server (client output):
Control Channel: TLSv1.3, cipher
TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 15616
bits ML-DSA-65, signature: id-ml-dsa-65, peer signing
digest/type: mldsa65 id-ml-dsa-65,
key agreement: X25519MLKEM768
with an secp384r1 certificate:
Control Channel: TLSv1.3, cipher
TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 384
bits ECsecp384r1, signature: ecdsa-with-SHA256, peer signing
digest/type: ecdsa_secp384r1_sha384 ECDSA,
key agreement: X25519MLKEM768
Change-Id: I90d54853fe1b1d820661cc2c099e07ec5d31ed05
Signed-off-by: Arne Schwabe <ar...@rf...>
---
M src/openvpn/openssl_compat.h
M src/openvpn/ssl_openssl.c
2 files changed, 29 insertions(+), 4 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/38/938/1
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index e2bd9bf..bd6f09c 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -197,6 +197,13 @@
}
#endif /* if OPENSSL_VERSION_NUMBER < 0x30500000 && (!defined(LIBRESSL_VERSION_NUMBER) || LIBRESSL_VERSION_NUMBER > 0x3050400fL) */
-
+#if OPENSSL_VERSION_NUMBER < 0x30200000L && OPENSSL_VERSION_NUMBER >= 0x30000000L
+static inline const char *
+SSL_get0_group_name(SSL *s)
+{
+ int nid = SSL_get_negotiated_group(s);
+ return SSL_group_to_name(s, nid);
+}
+#endif
#endif /* OPENSSL_COMPAT_H_ */
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 23b0266..d1d5d3e 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -2486,7 +2486,21 @@
peer_sig, peer_sig_type);
}
-
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+void
+print_tls_key_agreement_group(SSL *ssl, char *buf, size_t buflen)
+{
+ const char *groupname = SSL_get0_group_name(ssl);
+ if (!groupname)
+ {
+ snprintf(buf, buflen, ", key agreement: (error fetching group)");
+ }
+ else
+ {
+ snprintf(buf, buflen, ", key agreement: %s", groupname);
+ }
+}
+#endif
/* **************************************
*
@@ -2503,8 +2517,9 @@
char s2[256];
char s3[256];
char s4[256];
+ char s5[256];
- s1[0] = s2[0] = s3[0] = s4[0] = 0;
+ s1[0] = s2[0] = s3[0] = s4[0] = s5[0] = 0;
ciph = SSL_get_current_cipher(ks_ssl->ssl);
snprintf(s1, sizeof(s1), "%s %s, cipher %s %s",
prefix,
@@ -2520,8 +2535,11 @@
}
print_server_tempkey(ks_ssl->ssl, s3, sizeof(s3));
print_peer_signature(ks_ssl->ssl, s4, sizeof(s4));
+#if OPENSSL_VERSION_NUMBER >= 0x30000000L
+ print_tls_key_agreement_group(ks_ssl->ssl, s5, sizeof(s5));
+#endif
- msg(D_HANDSHAKE, "%s%s%s%s", s1, s2, s3, s4);
+ msg(D_HANDSHAKE, "%s%s%s%s%s", s1, s2, s3, s4, s5);
}
void
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/938?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: I90d54853fe1b1d820661cc2c099e07ec5d31ed05
Gerrit-Change-Number: 938
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <arn...@rf...>
Gerrit-Reviewer: flichtenheld <fr...@li...>
Gerrit-CC: openvpn-devel <ope...@li...>
Gerrit-Attention: flichtenheld <fr...@li...>
Gerrit-MessageType: newchange
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-04-09 11:52:04
|
Attention is currently required from: flichtenheld, plaisthos. cron2 has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/938?usp=email ) Change subject: Also print key agreement when printing negotiated details ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: Useful. Code matches what the OpenSSL online docs say how to use the function & the compat functions. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/938?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: I90d54853fe1b1d820661cc2c099e07ec5d31ed05 Gerrit-Change-Number: 938 Gerrit-PatchSet: 1 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Wed, 09 Apr 2025 11:51:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
|
From: Gert D. <ge...@gr...> - 2025-04-09 12:24:44
|
From: Arne Schwabe <ar...@rf...> With TLS 1.0 to 1.2, the used key agreement was depended on the certificates themselves. With TLS 1.3 is no longer the case but basically always X25519 was used. So this information has been very interesting so far. But with OpenSSL 3.5.0 and the new X25519MLKEM768 hybrid key agreement, the used key agreement group actually becomes interesting information. This commit adds printing this information for OpenSSL 3.0.0+ and uses a compat version for OpenSSL 3.0-3.1 to avoid an additional ifdef in the code itself. Example output with ML-DSA-65 certificates on the server (client output): Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 15616 bits ML-DSA-65, signature: id-ml-dsa-65, peer signing digest/type: mldsa65 id-ml-dsa-65, key agreement: X25519MLKEM768 with an secp384r1 certificate: Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 384 bits ECsecp384r1, signature: ecdsa-with-SHA256, peer signing digest/type: ecdsa_secp384r1_sha384 ECDSA, key agreement: X25519MLKEM768 Change-Id: I90d54853fe1b1d820661cc2c099e07ec5d31ed05 Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/938 This mail reflects revision 1 of this Change. Acked-by according to Gerrit (reflected above): Gert Doering <ge...@gr...> diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index e2bd9bf..bd6f09c 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -197,6 +197,13 @@ } #endif /* if OPENSSL_VERSION_NUMBER < 0x30500000 && (!defined(LIBRESSL_VERSION_NUMBER) || LIBRESSL_VERSION_NUMBER > 0x3050400fL) */ - +#if OPENSSL_VERSION_NUMBER < 0x30200000L && OPENSSL_VERSION_NUMBER >= 0x30000000L +static inline const char * +SSL_get0_group_name(SSL *s) +{ + int nid = SSL_get_negotiated_group(s); + return SSL_group_to_name(s, nid); +} +#endif #endif /* OPENSSL_COMPAT_H_ */ diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 23b0266..d1d5d3e 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -2486,7 +2486,21 @@ peer_sig, peer_sig_type); } - +#if OPENSSL_VERSION_NUMBER >= 0x30000000L +void +print_tls_key_agreement_group(SSL *ssl, char *buf, size_t buflen) +{ + const char *groupname = SSL_get0_group_name(ssl); + if (!groupname) + { + snprintf(buf, buflen, ", key agreement: (error fetching group)"); + } + else + { + snprintf(buf, buflen, ", key agreement: %s", groupname); + } +} +#endif /* ************************************** * @@ -2503,8 +2517,9 @@ char s2[256]; char s3[256]; char s4[256]; + char s5[256]; - s1[0] = s2[0] = s3[0] = s4[0] = 0; + s1[0] = s2[0] = s3[0] = s4[0] = s5[0] = 0; ciph = SSL_get_current_cipher(ks_ssl->ssl); snprintf(s1, sizeof(s1), "%s %s, cipher %s %s", prefix, @@ -2520,8 +2535,11 @@ } print_server_tempkey(ks_ssl->ssl, s3, sizeof(s3)); print_peer_signature(ks_ssl->ssl, s4, sizeof(s4)); +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + print_tls_key_agreement_group(ks_ssl->ssl, s5, sizeof(s5)); +#endif - msg(D_HANDSHAKE, "%s%s%s%s", s1, s2, s3, s4); + msg(D_HANDSHAKE, "%s%s%s%s%s", s1, s2, s3, s4, s5); } void |
|
From: Gert D. <ge...@gr...> - 2025-04-09 12:48:08
|
If everything works fine, nobody needs to look at this - but if things
break, extra information is always good to have. I have stared at the
code, and compared code & compat code with the OpenSSL documentation,
and this all makes sense. The buildbots have tested it (t_client
tests actually excercising this code, with various OpenSSL and mbedTLS
versions) and nothing breaks.
Your patch has been applied to the master branch.
Commit message adjusted ("not" inserted") as discussed.
commit 5b7a1bc34c8a3f86dfcc91b7a1aa520b1c549390
Author: Arne Schwabe
Date: Wed Apr 9 14:24:03 2025 +0200
Also print key agreement when printing negotiated details
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@gr...>
URL: https://www.mail-archive.com/ope...@li.../msg31393.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: cron2 (C. Review) <ge...@op...> - 2025-04-09 12:49:04
|
cron2 has uploaded a new patch set (#2) to the change originally created by plaisthos. ( http://gerrit.openvpn.net/c/openvpn/+/938?usp=email ) The following approvals got outdated and were removed: Code-Review+2 by cron2 Change subject: Also print key agreement when printing negotiated details ...................................................................... Also print key agreement when printing negotiated details With TLS 1.0 to 1.2, the used key agreement was depended on the certificates themselves. With TLS 1.3 this is no longer the case but basically always X25519 was used. So this information has not been very interesting so far. With OpenSSL 3.5.0 and the new X25519MLKEM768 hybrid key agreement, the used key agreement group actually becomes interesting information. This commit adds printing this information for OpenSSL 3.0.0+ and uses a compat version for OpenSSL 3.0-3.1 to avoid an additional ifdef in the code itself. Example output with ML-DSA-65 certificates on the server (client output): Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 15616 bits ML-DSA-65, signature: id-ml-dsa-65, peer signing digest/type: mldsa65 id-ml-dsa-65, key agreement: X25519MLKEM768 with an secp384r1 certificate: Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 384 bits ECsecp384r1, signature: ecdsa-with-SHA256, peer signing digest/type: ecdsa_secp384r1_sha384 ECDSA, key agreement: X25519MLKEM768 Change-Id: I90d54853fe1b1d820661cc2c099e07ec5d31ed05 Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg31393.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/openssl_compat.h M src/openvpn/ssl_openssl.c 2 files changed, 29 insertions(+), 4 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/38/938/2 diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index e2bd9bf..bd6f09c 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -197,6 +197,13 @@ } #endif /* if OPENSSL_VERSION_NUMBER < 0x30500000 && (!defined(LIBRESSL_VERSION_NUMBER) || LIBRESSL_VERSION_NUMBER > 0x3050400fL) */ - +#if OPENSSL_VERSION_NUMBER < 0x30200000L && OPENSSL_VERSION_NUMBER >= 0x30000000L +static inline const char * +SSL_get0_group_name(SSL *s) +{ + int nid = SSL_get_negotiated_group(s); + return SSL_group_to_name(s, nid); +} +#endif #endif /* OPENSSL_COMPAT_H_ */ diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 23b0266..d1d5d3e 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -2486,7 +2486,21 @@ peer_sig, peer_sig_type); } - +#if OPENSSL_VERSION_NUMBER >= 0x30000000L +void +print_tls_key_agreement_group(SSL *ssl, char *buf, size_t buflen) +{ + const char *groupname = SSL_get0_group_name(ssl); + if (!groupname) + { + snprintf(buf, buflen, ", key agreement: (error fetching group)"); + } + else + { + snprintf(buf, buflen, ", key agreement: %s", groupname); + } +} +#endif /* ************************************** * @@ -2503,8 +2517,9 @@ char s2[256]; char s3[256]; char s4[256]; + char s5[256]; - s1[0] = s2[0] = s3[0] = s4[0] = 0; + s1[0] = s2[0] = s3[0] = s4[0] = s5[0] = 0; ciph = SSL_get_current_cipher(ks_ssl->ssl); snprintf(s1, sizeof(s1), "%s %s, cipher %s %s", prefix, @@ -2520,8 +2535,11 @@ } print_server_tempkey(ks_ssl->ssl, s3, sizeof(s3)); print_peer_signature(ks_ssl->ssl, s4, sizeof(s4)); +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + print_tls_key_agreement_group(ks_ssl->ssl, s5, sizeof(s5)); +#endif - msg(D_HANDSHAKE, "%s%s%s%s", s1, s2, s3, s4); + msg(D_HANDSHAKE, "%s%s%s%s%s", s1, s2, s3, s4, s5); } void -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/938?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: I90d54853fe1b1d820661cc2c099e07ec5d31ed05 Gerrit-Change-Number: 938 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: newpatchset |
|
From: cron2 (C. Review) <ge...@op...> - 2025-04-09 12:49:05
|
cron2 has submitted this change. ( http://gerrit.openvpn.net/c/openvpn/+/938?usp=email ) Change subject: Also print key agreement when printing negotiated details ...................................................................... Also print key agreement when printing negotiated details With TLS 1.0 to 1.2, the used key agreement was depended on the certificates themselves. With TLS 1.3 this is no longer the case but basically always X25519 was used. So this information has not been very interesting so far. With OpenSSL 3.5.0 and the new X25519MLKEM768 hybrid key agreement, the used key agreement group actually becomes interesting information. This commit adds printing this information for OpenSSL 3.0.0+ and uses a compat version for OpenSSL 3.0-3.1 to avoid an additional ifdef in the code itself. Example output with ML-DSA-65 certificates on the server (client output): Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 15616 bits ML-DSA-65, signature: id-ml-dsa-65, peer signing digest/type: mldsa65 id-ml-dsa-65, key agreement: X25519MLKEM768 with an secp384r1 certificate: Control Channel: TLSv1.3, cipher TLSv1.3 TLS_AES_256_GCM_SHA384, peer certificate: 384 bits ECsecp384r1, signature: ecdsa-with-SHA256, peer signing digest/type: ecdsa_secp384r1_sha384 ECDSA, key agreement: X25519MLKEM768 Change-Id: I90d54853fe1b1d820661cc2c099e07ec5d31ed05 Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@gr...> URL: https://www.mail-archive.com/ope...@li.../msg31393.html Signed-off-by: Gert Doering <ge...@gr...> --- M src/openvpn/openssl_compat.h M src/openvpn/ssl_openssl.c 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h index e2bd9bf..bd6f09c 100644 --- a/src/openvpn/openssl_compat.h +++ b/src/openvpn/openssl_compat.h @@ -197,6 +197,13 @@ } #endif /* if OPENSSL_VERSION_NUMBER < 0x30500000 && (!defined(LIBRESSL_VERSION_NUMBER) || LIBRESSL_VERSION_NUMBER > 0x3050400fL) */ - +#if OPENSSL_VERSION_NUMBER < 0x30200000L && OPENSSL_VERSION_NUMBER >= 0x30000000L +static inline const char * +SSL_get0_group_name(SSL *s) +{ + int nid = SSL_get_negotiated_group(s); + return SSL_group_to_name(s, nid); +} +#endif #endif /* OPENSSL_COMPAT_H_ */ diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c index 23b0266..d1d5d3e 100644 --- a/src/openvpn/ssl_openssl.c +++ b/src/openvpn/ssl_openssl.c @@ -2486,7 +2486,21 @@ peer_sig, peer_sig_type); } - +#if OPENSSL_VERSION_NUMBER >= 0x30000000L +void +print_tls_key_agreement_group(SSL *ssl, char *buf, size_t buflen) +{ + const char *groupname = SSL_get0_group_name(ssl); + if (!groupname) + { + snprintf(buf, buflen, ", key agreement: (error fetching group)"); + } + else + { + snprintf(buf, buflen, ", key agreement: %s", groupname); + } +} +#endif /* ************************************** * @@ -2503,8 +2517,9 @@ char s2[256]; char s3[256]; char s4[256]; + char s5[256]; - s1[0] = s2[0] = s3[0] = s4[0] = 0; + s1[0] = s2[0] = s3[0] = s4[0] = s5[0] = 0; ciph = SSL_get_current_cipher(ks_ssl->ssl); snprintf(s1, sizeof(s1), "%s %s, cipher %s %s", prefix, @@ -2520,8 +2535,11 @@ } print_server_tempkey(ks_ssl->ssl, s3, sizeof(s3)); print_peer_signature(ks_ssl->ssl, s4, sizeof(s4)); +#if OPENSSL_VERSION_NUMBER >= 0x30000000L + print_tls_key_agreement_group(ks_ssl->ssl, s5, sizeof(s5)); +#endif - msg(D_HANDSHAKE, "%s%s%s%s", s1, s2, s3, s4); + msg(D_HANDSHAKE, "%s%s%s%s%s", s1, s2, s3, s4, s5); } void -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/938?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: I90d54853fe1b1d820661cc2c099e07ec5d31ed05 Gerrit-Change-Number: 938 Gerrit-PatchSet: 2 Gerrit-Owner: plaisthos <arn...@rf...> Gerrit-Reviewer: cron2 <ge...@gr...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: merged |
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X