patch: isolate a pkcs11 module
Stef Walter
stefw at redhat.com
Mon Nov 10 02:41:12 PST 2014
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.
More information about the p11-glue
mailing list