Re: [Openvpn-devel] [PATCH] Add ability to specify initialize flags for pkcs11 provider

[Selva Nair]

Hi,

On Thu, Jun 23, 2022 at 8:43 AM David Sommerseth <
open...@sf.lists.topphemmelig.net> wrote:

> On 19/6/2022 19:28, Selva Nair wrote:
> > Hi,0
> >
> > On Thu, Sep 30, 2021 at 7:34 AM Petr Mikhalicin via Openvpn-devel
> >  > > wrote:
> >
> > New pkcs11-helper interface allows to setup pkcs11 provider via
> > properties:
> >
> https://github.com/alonbl/pkcs11-helper/commit/b78d21c7e26041746aa4ae3d08b95469e1714a85
> > <
> https://github.com/alonbl/pkcs11-helper/commit/b78d21c7e26041746aa4ae3d08b95469e1714a85
> >
> >
> > Also pkcs11-helper added ability to setup init args for pkcs11
> provider:
> >
> https://github.com/alonbl/pkcs11-helper/commit/133f893e30856eba1de715ecd6fe176722eb3097
> > <
> https://github.com/alonbl/pkcs11-helper/commit/133f893e30856eba1de715ecd6fe176722eb3097
> >
> >
> > Signed-off-by: Petr Mikhalicin  > >
> >
> >
> > Sorry for the long delay in getting back on this. I somehow also missed
> > the related discussion on Trac
> > (https://community.openvpn.net/openvpn/ticket/1453
> > )
> >
> > I don't quite understand the need for exposing "init-args" to the user.
> > The only two supported flags in the cryptoki docs are related to the use
> > of threads. But we are the application and we should know what flags to
> > pass --- not the user --- isn't it? If CKF_OS_LOCKING_OK is required,
> > can't we just set it unconditionally?
> >
> > That said, OpenVPN2 is single threaded, so why is there a "bug in
> > openvpn" related to the use of pkcs11 library from multiple threads
> > referred to in the trac ticket?
>
> I haven't dug too deep into the matter this time; and it depends also on
> the OS you are on.  But there has been some issues with pkcs11-helper on
> hosts with systemd, due to some intricacies with openvpn doing a fork to
> kick off the password query mechanism with systemd colliding with some
> pkcs11-helper implementation details.  For the systemd case, we added a
> workaround which made most people happy.
>
> For more details:
> 
>

This is a different issue from  mutex locking required when  pkcs#11  calls
are made from multiple threads. The rationale for this patch was that we
may need to tell the provider library whether native OS locking methods are
okay or not, which I see no need for in a single threaded program.

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Add ability to specify initialize flags for pkcs11 provider

[David Sommerseth]

On 19/06/2022 19:28, Selva Nair wrote:

Hi,

On Thu, Sep 30, 2021 at 7:34 AM Petr Mikhalicin via Openvpn-devel 
> wrote:


New pkcs11-helper interface allows to setup pkcs11 provider via
properties:

https://github.com/alonbl/pkcs11-helper/commit/b78d21c7e26041746aa4ae3d08b95469e1714a85



Also pkcs11-helper added ability to setup init args for pkcs11 provider:

https://github.com/alonbl/pkcs11-helper/commit/133f893e30856eba1de715ecd6fe176722eb3097



Signed-off-by: Petr Mikhalicin mailto:mkh199...@mail.ru>>


Sorry for the long delay in getting back on this. I somehow also missed 
the related discussion on Trac 
(https://community.openvpn.net/openvpn/ticket/1453 
)


I don't quite understand the need for exposing "init-args" to the user. 
The only two supported flags in the cryptoki docs are related to the use 
of threads. But we are the application and we should know what flags to 
pass --- not the user --- isn't it? If CKF_OS_LOCKING_OK is required, 
can't we just set it unconditionally?


That said, OpenVPN2 is single threaded, so why is there a "bug in 
openvpn" related to the use of pkcs11 library from multiple threads 
referred to in the trac ticket?


I haven't dug too deep into the matter this time; and it depends also on 
the OS you are on.  But there has been some issues with pkcs11-helper on 
hosts with systemd, due to some intricacies with openvpn doing a fork to 
kick off the password query mechanism with systemd colliding with some 
pkcs11-helper implementation details.  For the systemd case, we added a 
workaround which made most people happy.


For more details:



--
kind regards,

David Sommerseth
OpenVPN Inc



___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Add ability to specify initialize flags for pkcs11 provider

[Selva Nair]

Hi,

On Thu, Sep 30, 2021 at 7:34 AM Petr Mikhalicin via Openvpn-devel <
openvpn-devel@lists.sourceforge.net> wrote:

> New pkcs11-helper interface allows to setup pkcs11 provider via
> properties:
> https://github.com/alonbl/pkcs11-helper/commit/b78d21c7e26041746aa4ae3d08b95469e1714a85
>
> Also pkcs11-helper added ability to setup init args for pkcs11 provider:
>
> https://github.com/alonbl/pkcs11-helper/commit/133f893e30856eba1de715ecd6fe176722eb3097
>
> Signed-off-by: Petr Mikhalicin 
>

Sorry for the long delay in getting back on this. I somehow also missed the
related discussion on Trac (
https://community.openvpn.net/openvpn/ticket/1453)

I don't quite understand the need for exposing "init-args" to the user. The
only two supported flags in the cryptoki docs are related to the use of
threads. But we are the application and we should know what flags to pass
--- not the user --- isn't it? If CKF_OS_LOCKING_OK is required, can't we
just set it unconditionally?

That said, OpenVPN2 is single threaded, so why is there a "bug in openvpn"
related to the use of pkcs11 library from multiple threads referred to in
the trac ticket?

Selva
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Re: [Openvpn-devel] [PATCH] Add ability to specify initialize flags for pkcs11 provider

[Gert Doering]

Hi,

On Thu, Sep 30, 2021 at 02:33:08PM +0300, Petr Mikhalicin via Openvpn-devel 
wrote:
> New pkcs11-helper interface allows to setup pkcs11 provider via
> properties: 
> https://github.com/alonbl/pkcs11-helper/commit/b78d21c7e26041746aa4ae3d08b95469e1714a85
> 
> Also pkcs11-helper added ability to setup init args for pkcs11 provider:
> https://github.com/alonbl/pkcs11-helper/commit/133f893e30856eba1de715ecd6fe176722eb3097

I can't comment on the PKCS#11 feature (not my field), but I have a few 
comments about required coding style changes:

> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -664,6 +664,11 @@ static const char usage_message[] =
>  "  8   : Use Unwrap.\n"
>  "--pkcs11-cert-private [0|1] ... : Set if login should be performed 
> before\n"
>  "  certificate can be accessed. Set for 
> each provider.\n"
> +"--pkcs11-init-flags hex ... : PKCS#11 init flags.\n"
> +"  It's bitwise OR of some PKCS#11 
> initialize flags.\n"
> +"  Most popular of them is:\n"
> +"  1   : 
> CKF_LIBRARY_CANT_CREATE_OS_THREADS\n"
> +"  2   : CKF_OS_LOCKING_OK\n"

The indent here is not right - did you use TABs here?  Please don't, they
get usually messed up by mail clients.

> @@ -1838,6 +1843,13 @@ show_settings(const struct options *o)
>  SHOW_PARM(pkcs11_cert_private, o->pkcs11_cert_private[i] ? 
> "ENABLED" : "DISABLED", "%s");
>  }
>  }
> +{
> +int i;
> +for (i = 0; i +{
> +SHOW_PARM(pkcs11_init_flags, o->pkcs11_init_flags[i], "%08x");
> +}
> +}

This, we do C99 style nowadays:

> +for (int i=0; i +{
> +SHOW_PARM(pkcs11_init_flags, o->pkcs11_init_flags[i], "%08x");
> +}

(so, no extra brackets, and the "int i" can go right into the for()
clause)

>  SHOW_INT(pkcs11_pin_cache_period);
>  SHOW_STR(pkcs11_id);
>  SHOW_BOOL(pkcs11_id_management);
> @@ -8778,6 +8790,17 @@ add_option(struct options *options,
>  options->pkcs11_cert_private[j-1] = atoi(p[j]) != 0 ? 1 : 0;
>  }
>  }
> +else if (streq(p[0], "pkcs11-init-flags"))
> +{
> +int j;
> +
> +VERIFY_PERMISSION(OPT_P_GENERAL);
> +
> +for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)

Same here: "int j" goes into the loop.

> diff --git a/src/openvpn/pkcs11.c b/src/openvpn/pkcs11.c
> index 02d0f51f..29db7ea4 100644
> --- a/src/openvpn/pkcs11.c
> +++ b/src/openvpn/pkcs11.c
> @@ -374,12 +374,17 @@ pkcs11_terminate(void)
> +if ((rv = pkcs11h_registerProvider(provider)) != CKR_OK) {
> +msg(M_WARN, "PKCS#11: Cannot register provider '%s' %ld-'%s'", 
> provider, rv, pkcs11h_getMessage(rv));
> + success = false;
> + goto exit;
> +}

The "{" always goes to the next line, and indenting is never done with
tabs (the lines above look like a mixture of tabs and spaces, and the
tab being messed up by the mail client).

> +// pkcs11-helper take ownership over this pointer

No C++ comments, please.

> +// pkcs11-helper take ownership over this pointer
> +if ((p_init_args = malloc(sizeof(*p_init_args))) == NULL) {
> +msg(M_FATAL, "PKCS#11: Cannot allocate memory");
> + success = false;
> + goto cleanup;
> +}
> +
> +memset(p_init_args, 0, sizeof(*p_init_args));

Please use calloc() and check_malloc_return() instead.

msg(M_FATAL) never returns, so the "success = false, goto cleanup" bit
is not needed - and all that is done by check_malloc_return() for you :-)


For our coding style guidelines, see also here:

  https://community.openvpn.net/openvpn/wiki/CodeStyle

and in the openvpn repo there is a "dev-tools/uncrustify.conf" config
which can be used with the "uncrustify" program to format your code
according to the whitespace rules.  Won't do the "for (int i=0; ...)"
C99 changes, though.

gert
-- 
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
 Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany g...@greenie.muc.de


signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

[Openvpn-devel] [PATCH] Add ability to specify initialize flags for pkcs11 provider

[Petr Mikhalicin via Openvpn-devel]

New pkcs11-helper interface allows to setup pkcs11 provider via
properties: 
https://github.com/alonbl/pkcs11-helper/commit/b78d21c7e26041746aa4ae3d08b95469e1714a85

Also pkcs11-helper added ability to setup init args for pkcs11 provider:
https://github.com/alonbl/pkcs11-helper/commit/133f893e30856eba1de715ecd6fe176722eb3097

Signed-off-by: Petr Mikhalicin 
---
 src/openvpn/init.c|  3 +-
 src/openvpn/options.c | 23 
 src/openvpn/options.h |  1 +
 src/openvpn/pkcs11.c  | 82 ---
 src/openvpn/pkcs11.h  |  3 +-
 5 files changed, 90 insertions(+), 22 deletions(-)

diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 6d09e566..6af585ac 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -682,7 +682,8 @@ context_init_1(struct context *c)
 for (i = 0; ioptions.pkcs11_providers[i] != NULL; i++)
 {
 pkcs11_addProvider(c->options.pkcs11_providers[i], 
c->options.pkcs11_protected_authentication[i],
-   c->options.pkcs11_private_mode[i], 
c->options.pkcs11_cert_private[i]);
+   c->options.pkcs11_private_mode[i], 
c->options.pkcs11_cert_private[i],
+   c->options.pkcs11_init_flags[i]);
 }
 }
 #endif
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index b3a83aa1..0939ee86 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -664,6 +664,11 @@ static const char usage_message[] =
 "  8   : Use Unwrap.\n"
 "--pkcs11-cert-private [0|1] ... : Set if login should be performed 
before\n"
 "  certificate can be accessed. Set for 
each provider.\n"
+"--pkcs11-init-flags hex ... : PKCS#11 init flags.\n"
+"  It's bitwise OR of some PKCS#11 initialize 
flags.\n"
+"  Most popular of them is:\n"
+"  1   : 
CKF_LIBRARY_CANT_CREATE_OS_THREADS\n"
+"  2   : CKF_OS_LOCKING_OK\n"
 "--pkcs11-pin-cache seconds  : Number of seconds to cache PIN. The 
default is -1\n"
 "  cache until token is removed.\n"
 "--pkcs11-id-management  : Acquire identity from management 
interface.\n"
@@ -1838,6 +1843,13 @@ show_settings(const struct options *o)
 SHOW_PARM(pkcs11_cert_private, o->pkcs11_cert_private[i] ? 
"ENABLED" : "DISABLED", "%s");
 }
 }
+{
+int i;
+for (i = 0; ipkcs11_init_flags[i], "%08x");
+}
+}
 SHOW_INT(pkcs11_pin_cache_period);
 SHOW_STR(pkcs11_id);
 SHOW_BOOL(pkcs11_id_management);
@@ -8778,6 +8790,17 @@ add_option(struct options *options,
 options->pkcs11_cert_private[j-1] = atoi(p[j]) != 0 ? 1 : 0;
 }
 }
+else if (streq(p[0], "pkcs11-init-flags"))
+{
+int j;
+
+VERIFY_PERMISSION(OPT_P_GENERAL);
+
+for (j = 1; j < MAX_PARMS && p[j] != NULL; ++j)
+{
+sscanf(p[j], "%x", &(options->pkcs11_init_flags[j-1]));
+}
+}
 else if (streq(p[0], "pkcs11-pin-cache") && p[1] && !p[2])
 {
 VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 98c21a2a..2317528e 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -573,6 +573,7 @@ struct options
 unsigned pkcs11_private_mode[MAX_PARMS];
 bool pkcs11_protected_authentication[MAX_PARMS];
 bool pkcs11_cert_private[MAX_PARMS];
+unsigned pkcs11_init_flags[MAX_PARMS];
 int pkcs11_pin_cache_period;
 const char *pkcs11_id;
 bool pkcs11_id_management;
diff --git a/src/openvpn/pkcs11.c b/src/openvpn/pkcs11.c
index 02d0f51f..29db7ea4 100644
--- a/src/openvpn/pkcs11.c
+++ b/src/openvpn/pkcs11.c
@@ -374,12 +374,17 @@ pkcs11_terminate(void)
 bool
 pkcs11_addProvider(
 const char *const provider,
-const bool protected_auth,
+const bool _protected_auth,
 const unsigned private_mode,
-const bool cert_private
+const bool _cert_private,
+const unsigned init_flags
 )
 {
 CK_RV rv = CKR_OK;
+int success = true;
+PKCS11H_BOOL protected_auth = _protected_auth;
+PKCS11H_BOOL cert_private = _cert_private;
+CK_C_INITIALIZE_ARGS_PTR p_init_args;
 
 ASSERT(provider!=NULL);
 
@@ -396,29 +401,66 @@ pkcs11_addProvider(
 provider
 );
 
-if (
-(rv = pkcs11h_addProvider(
- provider,
- provider,
- protected_auth,
- private_mode,
- PKCS11H_SLOTEVENT_METHOD_AUTO,
- 0,
- cert_private
- )) != CKR_OK
-)
-{
-msg(M_WARN, "PKCS#11: Cannot initialize provider '%s' %ld-'%s'", 
provider, rv, pkcs11h_getMessage(rv));
+if ((rv = pkcs11h_registerProvider(provider)) != CKR_OK) {
+msg(M_WARN, "PKCS#11: Cannot