diff mbox series

[Openvpn-devel,v7] ssl_verify_openssl: Clean up extract_x509_extension

Message ID 20260309133236.29732-1-frank@lichtenheld.com
State New
Headers
Series [Openvpn-devel,v7] ssl_verify_openssl: Clean up extract_x509_extension |

Commit Message

Frank Lichtenheld March 9, 2026, 1:32 p.m. UTC 
  * Avoid sign-compare warning when comparing string
  lengths
* Use the nicer alias rfc822Name instead of the general ia5
  from the GENERAL_NAME union.
* Use the official ASN1_STRING_length API instead of accessing
  the struct directly.
* C11 changes

Change-Id: I23cc00aee47aef007ab2e7d50b52c6de299505db
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1507
---

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/+/1507
This mail reflects revision 7 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering March 10, 2026, 8:27 a.m. UTC | #1 
This looked good to me, but "OpenSSL innards" are still foreign territory,
so thanks Arne for looking at the ia5/rfc822Name change (I found it 
surprising since we had an "ia5->length" before and now need to do
ASN1_STRING_LENGTH(), but I'm sure there is a good reason behind this).

Your patch has been applied to the master branch.

commit 66060627a8cf05c8761d75985e76482d20df4f29
Author: Frank Lichtenheld
Date:   Mon Mar 9 14:32:36 2026 +0100

     ssl_verify_openssl: Clean up extract_x509_extension

     Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1507
     Message-Id: <20260309133236.29732-1-frank@lichtenheld.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg35980.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering March 23, 2026, 12:57 p.m. UTC | #2 
Hi,

On Tue, Mar 10, 2026 at 09:27:19AM +0100, Gert Doering wrote:
> This looked good to me, but "OpenSSL innards" are still foreign territory,
> so thanks Arne for looking at the ia5/rfc822Name change (I found it 
> surprising since we had an "ia5->length" before and now need to do
> ASN1_STRING_LENGTH(), but I'm sure there is a good reason behind this).
> 
> Your patch has been applied to the master branch.
> 
> commit 66060627a8cf05c8761d75985e76482d20df4f29
> Author: Frank Lichtenheld
> Date:   Mon Mar 9 14:32:36 2026 +0100
> 
>      ssl_verify_openssl: Clean up extract_x509_extension

As suggested by Rudi Heitbaum in GH PR 1003, I have cherry-picked this
back to release/2.7 - it's "long term compat".  We expect OpenVPN 2.7.x
to be maintained for long enough that OpenSSL4 becomes a thing, and then
we need the patch - so we can do it right away, and be not surprised.

The patch is the same as in the master branch, except for the #pragma
removal - which git does not care much about ("you want it gone, it was
not there in the first place, so the net result is what you want").

Mildly tested via GH actions.

commit 5f0a168311a53b935f776d4c1566c4f2f3cde98b (release/2.7)
Author: Frank Lichtenheld <frank@lichtenheld.com>
Date:   Mon Mar 9 14:32:36 2026 +0100

    ssl_verify_openssl: Clean up extract_x509_extension
...
    (cherry picked from commit 66060627a8cf05c8761d75985e76482d20df4f29)

gert
diff mbox series

Patch

diff --git a/src/openvpn/ssl_verify_openssl.c b/src/openvpn/ssl_verify_openssl.c
index 58f665c..46401cd 100644
--- a/src/openvpn/ssl_verify_openssl.c
+++ b/src/openvpn/ssl_verify_openssl.c
@@ -118,16 +118,10 @@ 
     return nid == NID_subject_alt_name || nid == NID_issuer_alt_name;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic push
-#pragma GCC diagnostic ignored "-Wsign-compare"
-#endif
-
 static bool
 extract_x509_extension(X509 *cert, char *fieldname, char *out, size_t size)
 {
     bool retval = false;
-    char *buf = 0;
 
     if (!x509_username_field_ext_supported(fieldname))
     {
@@ -139,29 +133,28 @@ 
     GENERAL_NAMES *extensions = X509_get_ext_d2i(cert, nid, NULL, NULL);
     if (extensions)
     {
-        int numalts;
-        int i;
         /* get amount of alternatives,
          * RFC2459 claims there MUST be at least
          * one, but we don't depend on it...
          */
 
-        numalts = sk_GENERAL_NAME_num(extensions);
+        int numalts = sk_GENERAL_NAME_num(extensions);
 
         /* loop through all alternatives */
-        for (i = 0; i < numalts; i++)
+        for (int i = 0; i < numalts; i++)
         {
             /* get a handle to alternative name number i */
             const GENERAL_NAME *name = sk_GENERAL_NAME_value(extensions, i);
+            char *buf = NULL;
 
             switch (name->type)
             {
                 case GEN_EMAIL:
-                    if (ASN1_STRING_to_UTF8((unsigned char **)&buf, name->d.ia5) < 0)
+                    if (ASN1_STRING_to_UTF8((unsigned char **)&buf, name->d.rfc822Name) < 0)
                     {
                         continue;
                     }
-                    if (strlen(buf) != name->d.ia5->length)
+                    if ((ssize_t)strlen(buf) != ASN1_STRING_length(name->d.rfc822Name))
                     {
                         msg(D_TLS_ERRORS, "ASN1 ERROR: string contained terminating zero");
                         OPENSSL_free(buf);
@@ -175,7 +168,7 @@ 
                     break;
 
                 default:
-                    msg(D_TLS_DEBUG, "%s: ignoring general name field type %i", __func__,
+                    msg(D_TLS_DEBUG, "%s: ignoring general name field type %d", __func__,
                         name->type);
                     break;
             }
@@ -185,10 +178,6 @@ 
     return retval;
 }
 
-#if defined(__GNUC__) || defined(__clang__)
-#pragma GCC diagnostic pop
-#endif
-
 /*
  * Extract a field from an X509 subject name.
  *