[Openvpn-devel,v2,1/3] unify code path for adding PKCS#11 providers

Message ID 20221211190913.190-1-marc.becker@astos.de
State Accepted
Headers show
Series [Openvpn-devel,v2,1/3] unify code path for adding PKCS#11 providers | expand

Commit Message

Marc Becker Dec. 11, 2022, 7:09 p.m. UTC
Use existing wrapper for pkcs11h_addProvider to have arguments with
"magic values" for pkcs11-helper call in a central place.

Slot event argument to pkcs11h_addProvider has NOT been a boolean for
at least 15 years.
Luckily the default is PKCS11H_SLOTEVENT_METHOD_AUTO=0=FALSE.

Signed-off-by: Marc Becker <marc.becker@astos.de>
---
v2: propper commit message, error message without error code reference
---
 src/openvpn/pkcs11.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

Comments

Frank Lichtenheld Dec. 14, 2022, 3:25 p.m. UTC | #1
On Sun, Dec 11, 2022 at 08:09:13PM +0100, Marc Becker via Openvpn-devel wrote:
> Use existing wrapper for pkcs11h_addProvider to have arguments with
> "magic values" for pkcs11-helper call in a central place.
> 
> Slot event argument to pkcs11h_addProvider has NOT been a boolean for
> at least 15 years.
> Luckily the default is PKCS11H_SLOTEVENT_METHOD_AUTO=0=FALSE.

Change makes sense and should not change behavior AFAICT.

Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

Regards,
Gert Doering Dec. 15, 2022, 7:55 a.m. UTC | #2
I do claim that I have no idea about pkcs11-helper, but this patch 
isn't *that* complex, just using the existing wrapper... but still
thanks to Frank for reviewing.  I have test compiled on Gentoo with 
"pkcs11-helper-1.27.0-r1", and "it compiled".

I *do* find this part a bit nonintuitive...

   pkcs11_addProvider( ..., cert_private ? TRUE : FALSE)

.. we have a bool, pass it to a bool, and evaluate it in tri-state, to
pass true, if the bool is true, and false, if it is not...  wat?


One day, someone should reformat the rest of pkcs11.c, though... this
stuff is outright ugly

    msg(
        M_INFO,
        "PKCS#11: Adding PKCS#11 provider '%s'",
        provider
        );

(but I'm aware that OpenVPN had this phase when every function call
needed to be distributed to the maximum number of lines possible...)


Your patch has been applied to the master and release/2.6 branch.

commit b08f8cbb2b92f3ee0eced39d11665befea3aec87 (master)
commit f1995ccca4c105e71728101bb719d235f5605b33 (release/2.6)
Author: Marc Becker via Openvpn-devel
Date:   Sun Dec 11 20:09:13 2022 +0100

     unify code path for adding PKCS#11 providers

     Signed-off-by: Marc Becker <marc.becker@astos.de>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20221211190913.190-1-marc.becker@astos.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25642.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering Dec. 15, 2022, 8:14 a.m. UTC | #3
Hi,

*sigh* - "Marc Becker via Openvpn-devel", this so messes up my workflow...
(can DMARC please go somewhere and die, alone, with no friends?).

I had to reapply these patches 1/3 and 2/3 to get the author right,
so you'll see new commit IDs.

For 1/3, these are:

On Thu, Dec 15, 2022 at 08:55:19AM +0100, Gert Doering wrote:
> commit b08f8cbb2b92f3ee0eced39d11665befea3aec87 (master)
> commit f1995ccca4c105e71728101bb719d235f5605b33 (release/2.6)

new:

commit 8958a365479348c1500dee44e1a8b27e7e35a96f ( master)
commit bcceded96775cc5a131bb9ab11ba855c7576603d (release/2.6)

Author: Marc Becker <marc.becker@astos.de>
Date:   Sun Dec 11 20:09:13 2022 +0100

    unify code path for adding PKCS#11 providers

nothing in the actual patch has changed, just the author (and that,
of course, changes the commit IDs).

gert

Patch

diff --git a/src/openvpn/pkcs11.c b/src/openvpn/pkcs11.c
index 507af17c..fbc4c472 100644
--- a/src/openvpn/pkcs11.c
+++ b/src/openvpn/pkcs11.c
@@ -853,19 +853,9 @@  show_pkcs11_ids(
         goto cleanup;
     }
 
-    if (
-        (rv = pkcs11h_addProvider(
-             provider,
-             provider,
-             TRUE,
-             0,
-             FALSE,
-             0,
-             cert_private ? TRUE : FALSE
-             )) != CKR_OK
-        )
+    if (!pkcs11_addProvider(provider, TRUE, 0, cert_private ? TRUE : FALSE))
     {
-        msg(M_FATAL, "PKCS#11: Cannot add provider '%s' %ld-'%s'", provider, rv, pkcs11h_getMessage(rv));
+        msg(M_FATAL, "Failed to add PKCS#11 provider '%s", provider);
         goto cleanup;
     }