On Mon, Oct 08, 2007 at 11:02:19PM +0200, Alon Bar-Lev wrote: > I have a newer PKCS#11 module with configuration now. > I will send it to you, along with some of my thoughts regarding my > experience.
Some inline comments below.
> +#include <pkcs11-helper-1.0/pkcs11h-certificate.h>
> +#include <pkcs11-helper-1.0/pkcs11h-openssl.h>
It is odd to put a version number right in the #include path. Can't a
version check be done from data inside the .h files?
> + if (blob) blob[i] = serialized_id_length % 256;
> + i++;
> + if (blob) blob[i] = serialized_id_length >> 8;
> + i++;
> + if (blob) memcpy(&blob[i], pkcs11_data->serialized_id,
> serialized_id_length);
> + i += serialized_id_length;
> + if (blob) blob[i] = pkcs11_data->certificate_blob_size % 256;
> + i++;
> + if (blob) blob[i] = pkcs11_data->certificate_blob_size >> 8;
> + i++;
> + if (blob) memcpy(&blob[i], pkcs11_data->certificate_blob,
> pkcs11_data->certificate_blob_size);
> + i += pkcs11_data->certificate_blob_size;
> + passphrase_length = strlen(pkcs11_data->passphrase) + 1; /* + '\0' */
> + if (blob) blob[i] = passphrase_length % 256;
> + i++;
> + if (blob) blob[i] = passphrase_length >> 8;
> + i++;
> + if (blob) memcpy(&blob[i], pkcs11_data->passphrase, passphrase_length);
> + i += passphrase_length;
I don't know how well gcc optimizes these if's, but it might make more
sense to consolidate the code under a single scope dominated by a
single conditional, even if that means having two segments of code
that look much the same.
> +static
> +PKCS11H_BOOL
> +pkcs11_token_prompt (
> + void * const global_data,
> + void * const user_data,
> + const pkcs11h_token_id_t token,
> + const unsigned retry
> +) {
> + (void)global_data;
> + (void)user_data;
> + (void)retry;
What's going on here?
> +
> + return FALSE;
return 0 is fine. Even though this is not kernel code, I try not to do
anything that goes against the kernel coding conventions (e.g.,
#defining TRUE and FALSE).
> +}
> +
> +static
> +PKCS11H_BOOL
See above.
> +static int ecryptfs_pkcs11_read_key(RSA **rsa, unsigned char *blob)
> +{
> + struct pkcs11_data _pkcs11_data;
> + struct pkcs11_data *pkcs11_data = &_pkcs11_data;
> + pkcs11h_certificate_id_t certificate_id = NULL;
> + pkcs11h_certificate_t certificate = NULL;
> + pkcs11h_openssl_session_t openssl_session = NULL;
> + CK_RV rv = CKR_OK;
> + int rc;
> +
> + if ((rc = ecryptfs_pkcs11_deserialize(pkcs11_data, blob)) != 0) {
> + goto out;
> + }
> +
> + if (
> + (rv = pkcs11h_certificate_deserializeCertificateId (
> + &certificate_id,
> + pkcs11_data->serialized_id
> + )) != CKR_OK
> + ) {
Okay; I see what's going on with pkcs11-helper now.
I can understand the desire to simplify the interaction with a PKCS#11
implementation in this manner, but then this is not a pure "PKCS#11"
key module. This is a key module that uses some intermediate generic
interface to some PKCS#11 backend. As such, this should not be labeled
as a "pkcs11" key module; it should be something like "pkcs11-helper".
The feature in the eCryptfs game plan is a key module that *only* uses
the PKCS#11 API calls, without any additional dependencies (OpenSSL,
helper library, etc.). The eCryptfs maintainers really cannot
officially support anything else.
That said, I am not against the existence of the pkcs11-helper key
module (that is the whole reason why there is a pluggable key module
API, much like OpenSSL has a pluggable engine API). I will probably
use it myself at some point. I just cannot promise you that it will
wind up getting approved to be packaged and supported in the official
ecryptfs-utils tarball; it might need to be shipped as an out-of-tree
add-on, for which you are the official maintainer, and have the
documentation point users to where they can download the key module
and get support for it. I will have to check on this.
> + if ((rc = ecryptfs_pkcs11_read_key(&rsa, blob))) {
> + rc = -(int)ERR_get_error();
> + syslog(LOG_ERR, "Error attempting to read RSA key from file;"
> + " rc = [%d]\n", rc);
> + goto out;
> + }
> + (*to_size) = RSA_size(rsa);
> + if (to) {
> + if ((rc = RSA_public_encrypt(from_size, from, to, rsa,
> + RSA_PKCS1_PADDING)) ==
> -1) {
I have not specified what I am looking for in a PKCS#11 key module,
but it will certainly involve making PKCS#11 calls to do the
encryption and decryption. This will allow, for instance, hardware
acceleration already provided by ICA OpenCryptoki token to take
effect. The key module should have as much flexibility as possible,
given what mechanisms and types are supported by the PKCS#11 spec.
> + char dn[1024] = {0};
> + char serial[1024] = {0};
Magic numbers.
> +static int ecryptfs_pkcs11_init(char **alias)
> +{
> + char *provider = "/usr/lib/pkcs11/libasepkcs.so";
I assume you are doing this just for convenience in the first pass and
plan on doing it properly in the next iteration.
> + if (asprintf(alias, "pkcs11") == -1) {
This needs to be "pkcs11-helper". Only a key module that makes
strictly PKCS#11 API calls should be aliased "pkcs11".
> + pkcs11h_setLogLevel (10);
Another magic number.
I also need to more fully test the build changes across the various
platforms (s390x, ppc64, etc.) on at least RHEL and SLES, and I should
at least make the SPEC file changes in Fedora before I release these
updates in the ecryptfs-utils, since I am the Fedora package
maintainer.
Thanks,
Mike
pgpMpYVGOHHph.pgp
Description: PGP signature
------------------------------------------------------------------------- This SF.net email is sponsored by: Splunk Inc. Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________ eCryptfs-devel mailing list eCryptfs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ecryptfs-devel
