patch: isolate a pkcs11 module

[Nikos Mavrogiannopoulos]

On Mon, 2014-11-10 at 11:41 +0100, Stef Walter wrote:
> On 03.11.2014 13:09, Nikos Mavrogiannopoulos wrote:
> > The attached patch allows to use p11-kit to run and use an isolated
> > PKCS #11 module. The performance cost seems to be quite limited.
> > I've tested it with softhsm (isolated) + lighttpd2 and a
> > pseudo-benchmark (run in the same pc) shows:
> 
> This is great! Nice work. I'd like to get this in. Some review below
> that would need to be fixed first. Happy to have discussion about any
> points that aren't clear or where I've misunderstood things.

Attached is an update to the original patch. What is handled is
discussed in the comments inline.

> I think than using '/' as the prefix here is appropriate. Note of the
> other conceived address forms start with '/'.

Done.

> I have some more general purpose code for this, which I've relicensed
> for this purpose. This makes it build on other Unix variants. Should
> we merge this first or would you like to incorporate it into your
> patch set?

Incorporated.

> Because things like like SELinux and AppArmor would want to treat the
> server differently, we should make it run in a separate process. You
> can see how this was done for 'p11-kit remote'.

Done. For some reason the server now defaults in debugging mode and
prints a lot of output. No idea how that happened.

> +#ifdef HAVE_SIGHANDLER_T
> +# define SIGHANDLER_T sighandler_t
> +#elif HAVE_SIG_T
> +# define SIGHANDLER_T sig_t
> +#elif HAVE___SIGHANDLER_T
> +# define SIGHANDLER_T __sighandler_t
> +#else
> +typedef void (*sighandler_t)(int);
> +# define SIGHANDLER_T sighandler_t
> +#endif
> +static unsigned need_children_cleanup = 0;
> +static unsigned terminate = 0;
> +static unsigned children_avail = 0;
> We shouldn't have unprotected globals like this in a library. They're
> also not thread safe. As an alternative, you could choose to move as
> much of the code out of the library and into p11_kit_server() if you want.

Moved everything of the kind to the server.

> +static void fix_info(const char *id, CK_INFO *info)
> +{
> +	unsigned len;
> +	unsigned i;
> +
> +	/* replace description */
> +	snprintf((char*)info->manufacturerID, sizeof(info->manufacturerID),
> "V:%s", id);
> +	len = strlen((char*)info->manufacturerID);
> +
> +	for (i=len;i<sizeof(info->manufacturerID);i++)
> +		 info->manufacturerID[i] = ' ';
> +}
> The reason for this code isn't commented or documented anywhere. Would
> prefer if it was a separate commit with its own commit message, test, etc.

I've added a special test case.

>  	case P11_RPC_CALL_##name: \
>  		ret = rpc_##name (self, &msg); \
>  		break;
> +	#define CASE_CALL_ID(id, name) \
> +	case P11_RPC_CALL_##name: \
> +		ret = rpc_##name (id, self, &msg); \
> +		break;
> 
> Since this is only called once, it probably make sense just to use the
> code directly without a macro.

Done.

> +		if (!p11_rpc_server_handle (name, &virt->funcs, buffer, buffer)) {
> +			p11_message ("unexpected error handling rpc message");
> +			goto out;
> +		}
> 
> This means we cannot handle multi-threading in the PKCS#11 client. Is
> this expected? Is it a limitation of your first round implementation?

Handled through a thread lock which serializes the requests. Once we
have a multi-threaded server a more advanced client will be needed that
can issue multiple requests and wake up the correct thread on a
response.


> +		if (daemon(0,0) == -1) {
> daemon() is unfortunately not portable. We'll get complains about that
> in short order. But here's an implementation which can go into
> compat.[ch]:

If a system doesn't have daemon(), server is not compiled.

> +	bool is_socket; /* in that case sfile should be used */
> +	char sfile[_POSIX_PATH_MAX];
>  } rpc_exec;
> rpc_exec is a struct that is used for the exec style rpc peer. We
> should define another rpc_connect for a peer which we connect() to.

Done.

regards,
Nikos

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-the-ability-to-use-a-PKCS-11-server-process-vi.patch
Type: text/x-patch
Size: 71898 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/p11-glue/attachments/20141202/36052cbe/attachment-0001.bin>