Add addProvider function which takes initialize flags

[lo1ol]

This patch resolves this issue. I decide to add new function because it's doesn't broke existing interface.

[lgtm-com[bot]]

This pull request introduces 1 alert when merging 18a483535a43d146e42a0eec8bfa065c1f46eaf0 into 88077c988afce13fa02c865f017c3e8e65dae7b9 - view on LGTM.com

new alerts:

• 1 for Missing return statement

[alonbl]

Thanks!

I thought for a while how to add this without breaking backward compatibility and without having to go over this again in future.

Obviously, the current interface is incorrect...

If we are doing this via code, I would prefer to add a different interface, something like:

pkcs11h_registerProvider(reference);
pkcs11h_setProviderProperty(reference, PROPERTY1, value1);
pkcs11h_setProviderProperty(reference, PROPERTY2, value2);
pkcs11h_setProviderProperty(reference, PROPERTY,3 value3);
pkcs11h_initializeProvider(reference);

Notice that it is best to pass the entire CK_C_INITIALIZE_ARGS if we already setting values within.

Another approach is to have a configuration file loaded by the library instead/in-addition of requiring application to be modified each time a new configuration is available. In order to preserve backward compatibility maybe we can have a syntax in provider_location which will reference a configuration file instead of shared object, for example config:/path/to/config.

I believe the configuration file approach is simpler and more flexible.

What do you think?

[lo1ol]

Thank you for your answer and thoghts!

I have thought about config-approach too, but sorry: code-way is simplier for me) If we will be using config files we have to write a parcer and resolve some OS-specific problems.

Any way, adding of config files may lead to adding pkcs11h*Provider functions, so that can implements both approaches.

Do you know any case when config approach is more flexiable?)

[alonbl]

using config file will make it possible to add new features without breaking/changing the application, for example openvpn will not be modified to respect the new setting.

[lo1ol]

What do you think about adding pkcs11h*Provider functions and mark pkcs11haddProvider as deprecated, with saving of this? This way also doesn't break current interface.

[lo1ol]

Oh. I got your idea about passing config using config:/path/to/config syntax. It's realy doesn't break compability.

BUT I think config file has to be specified by application, not by a user of this application: user may doesn't know anything about theads inside app. So, spicifieng of the config inside app is potentially a code breaking change anyway

[lo1ol]

Also, some application may expect that provider string is a path and try to make it canonical. config:/path/to/config is not a path and cannot be canonised.

[alonbl]

A path in Unix can contain any character, in windows path validation may fail due to ‘:’, however, I do not expect application to modify path. Anyway, this was an example and any magic string will be do the trick.

Most of the pkcs11 tweaks are local to providers, I found it coupled with provider implementation more than application.

Anyway, if you want to go ahead the first option, go ahead and create a patch.

On Tue, 25 May 2021 at 11:53 Petr Mikhalitsyn @.***> wrote:

Also, some application may expect that provider string is a path and try to make it canonical. config:/path/to/config is not a path and cannot be canonised.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/OpenSC/pkcs11-helper/pull/37#issuecomment-847683597, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJURLOPZU6AK3V7TPGZSQDTPNQP3ANCNFSM45NBGGLQ .

[lo1ol]

What do you think about updated MR?

Most of the pkcs11 tweaks are local to providers, I found it coupled with provider implementation more than application

Any way, an application needs be able to control setting of CKF_OS_LOCKING flag by its own. Therefore, the code way approach has to be implemented.

If do you want, I can implement config approach, but based on the *ProviderProperty functions.

[lo1ol]

Hi,

Thank you for your review. I'm rewrite my MR:

1. I split MR by adding new interface and adding init args
2. I decide not to change addProvider function, to doesn't remove PKCS11H_INIT_ARGS_RESERVED. But I still think that using an environment variables is bad idea.
3. I add static assertion function to check filling provider_property_name array. Also I think it's may be usefull in future. Notice it raises C standard to C11.
4. I've made other fixes code according your review

[lo1ol]

What do you think?

[alonbl]

Merged with changes via #44 Thanks for your help.

[lo1ol]

Inside your patch you doesn't take ownership over init_args. I think it's not convenient to use, because init_args are used mostly inside pkcs11helper.

I think that will be better to take it and frees inside removeProvider function.

What do you think about that?

[alonbl]

What do you think about that?

I think it is not wise, as the structure itself contains more pointers and we require deep copy, while we do not know what is there as we get no sequence nor size for pReserved. So I decided that the caller must preserve the structure and the relevant pointers.

[lo1ol]

I thinking for a lot time how to make destruction of init_flags simpler. What do you think about adding property PKCS11H_PROVIDER_PROPERTY_INIT_ARGS_DESTRUCTOR? :)

If it's not set the implementation may be like that:

void init_args_destructor(CK_C_INITIALIZE_ARGS_PTR init_args)
{
    if (init_args)
        free(args);
}