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. I've also included some links to unfinished work that I did, which may be a help. Stef ------ 8< ----------- 8< ------- 8<---------- > When the remote option is specified with a file starting with '@' > a unix socket will be used for PKCS #11 operations. I think than using '/' as the prefix here is appropriate. Note of the other conceived address forms start with '/'. #if !defined(__cplusplus) && (__GNUC__ > 2) #define GNUC_PRINTF(x, y) __attribute__((__format__(__printf__, x, y))) #else diff --git a/common/unix-peer.c b/common/unix-peer.c new file mode 100644 index 0000000..689159d --- /dev/null +++ b/common/unix-peer.c 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? http://cgit.freedesktop.org/p11-glue/p11-kit/tree/p11-kit/unix-credentials.c?h=wip/rpc-layer http://cgit.freedesktop.org/p11-glue/p11-kit/tree/p11-kit/unix-credentials.h?h=wip/rpc-layer +int p11_kit_server (int argc, + char *argv[]); 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'. + case opt_socket: + socket_file = strdup(optarg); Nit pick: In p11-kit function have spaces after them. +#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. +static +SIGHANDLER_T p11_signal(int signum, SIGHANDLER_T handler) +{ + struct sigaction new_action, old_action; + + new_action.sa_handler = handler; + sigemptyset (&new_action.sa_mask); + new_action.sa_flags = 0; + + sigaction (signum, &new_action, &old_action); + return old_action.sa_handler; +} This sorta thing should go into common/compat.[ch]. Ideally it would be broken out as a separate commit. I'm uncomfortable with libraries changing signal handlers like this. It would be better to put all such logic into the actual binary of the server. What is missing from p11_kit_remote_serve_module() to enable that? +static void fix_info(const char *id, CK_INFO *info) Nit pick, in p11-kit we have the return type on one line, function and first argument on the next, and all arguments wrapped. Like this: 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. bool -p11_rpc_server_handle (CK_X_FUNCTION_LIST *self, +p11_rpc_server_handle (const char *name, + CK_X_FUNCTION_LIST *self, p11_buffer *request, p11_buffer *response) { @@ -1796,9 +1851,13 @@ p11_rpc_server_handle (CK_X_FUNCTION_LIST *self, 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. + case 1: + if (version != 1) { + p11_message ("unspported version received: %d", (int)version); Typo. + goto out; + } + break; + default: + p11_message_err (errno, "couldn't read credential byte"); + goto out; + } + + version = 0; + pid = getpid(); + + iov[0].iov_base = &version; + iov[0].iov_len = 1; + + iov[1].iov_base = &pid; + iov[1].iov_len = 4; Why are we sending the pid, and what are we doing with this? I see it being assigned to an otherwise unused field in rpc_socket_file_connect(). + + switch (writev (fd, iov, 2)) { + case 5: + break; + default: + p11_message_err (errno, "couldn't write credential bytes"); This can fail with EAGAIN. + 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? Do you plan to address it later? I think that 'p11-kit remote' has a similar issue outstanding. Any thoughts here? +static void cleanup_children(void) +{ +int status; +pid_t pid; Indentation. + while ((pid = waitpid(-1, &status, WNOHANG)) > 0) { + if (children_avail > 0) + children_avail--; + if (WIFSIGNALED(status)) { + if (WTERMSIG(status) == SIGSEGV) + p11_message("child %u died with sigsegv\n", (unsigned)pid); + else + p11_message("child %u died with signal %d\n", (unsigned)pid, (int)WTERMSIG(status)); + } + } + need_children_cleanup = 0; +} Again, see above, this code in a library is a no go. +int +p11_kit_remote_isolate_module (CK_FUNCTION_LIST *module, + const char *socket_file, + mode_t mode, + uid_t uid, + gid_t gid, + uid_t run_as_uid, + gid_t run_as_gid, + unsigned foreground, + unsigned timeout) ... + umask(066); This is not thread safe in a library, see above. + if (setgid(run_as_gid) == -1) { + if (setgroups(1, &run_as_gid) == -1) { + if (setuid(run_as_uid) == -1) { + sigprocmask(SIG_BLOCK, &blockset, NULL); Again, it's better not to do this in a library. + 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]: http://cgit.freedesktop.org/p11-glue/p11-kit/tree/common/compat.c?h=wip/rpc-layer#n277 #ifdef OS_UNIX +#ifdef __linux__ +# include <sys/prctl.h> +#endif What is this header used for? typedef struct { /* Never changes */ int fd; + pid_t pid; I don't see where this is used or what it is used for. + 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. static void @@ -679,7 +737,7 @@ rpc_exec_wait_or_terminate (pid_t pid) } static void -rpc_exec_disconnect (p11_rpc_client_vtable *vtable, +rpc_disconnect (p11_rpc_client_vtable *vtable, Ditto, see above. #endif /* OS_UNIX */ + } else if (remote[0] == '@') { + rpc = rpc_socket_file_init (remote + 1, name); See above about '/' as the prefix. -bool p11_rpc_server_handle (CK_X_FUNCTION_LIST *funcs, +bool p11_rpc_server_handle (const char *name, Module 'name' has a specific meaning in p11-kit. Perhaps 'description' would be better here? Unless you are indeed passing a module name. Module names are the things passed to functions like this: http://p11-glue.freedesktop.org/doc/p11-kit/p11-kit-Modules.html#p11-kit-module-for-name static void -test_mock_add_tests (const char *prefix) +test_mock_add_tests (const char *prefix, unsigned no_mid) Should document what the extra argument means in a comment. diff --git a/p11-kit/test-proxy.c b/p11-kit/test-proxy.c index e4998be..63ffa4f 100644 --- a/p11-kit/test-proxy.c +++ b/p11-kit/test-proxy.c @@ -189,7 +189,7 @@ main (int argc, p11_test (test_initialize_finalize, "/proxy/initialize-finalize"); p11_test (test_initialize_multiple, "/proxy/initialize-multiple"); - test_mock_add_tests ("/proxy"); + test_mock_add_tests ("/proxy", 0); Can we use a flag instead of 0/1? Otherwise this is hard to read. If you use a descriptive flag name, then this would also fix the previous review point. _______________________________________________ p11-glue mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/p11-glue
