On 10/9/07, Michael Halcrow <[EMAIL PROTECTED]> wrote:
Thank you for reviewing the code.
> > +#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?
In order to allow side-by-side installation of two libraries.
>From my experience if you don't do this at start, later versions
becomes much more complex.
> > + 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.
And leading to the chance that I lose sync between calculation and set...
The serialize() is not called often, so I prefer to play safe.
If it really bothers you I will revert to your implementation at OpenSSL.
>
> > +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?
Not implemented yet... We should discuss how we wish to implement this.
>
> > +
> > + 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).
OK.
>
> > +}
> > +
> > +static
> > +PKCS11H_BOOL
>
> See above.
This is of the pkcs11-helper library...
I understand your position regarding this code, but other people do
prefer to have boolean type.
So I replaced TRUE->1, FALSE->0 but left the type.
> 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".
I disagree.
If I would have implemented native PKCS#11 code it would have been the
same, but as you don't write a code for reuse it will be much uglier.
pkcs11-helper is just a helper library to reduce the code from a
applications, to ease maintenance. It does enable the application to
access pure PKCS#11 providers.
This makes ecryptfs project focus in encrypted file system and not on
re-invent what already been implemented.
> 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.
I don't understand.
I thought we agree I maintain it with you guys.
Supporting smartcards is not simple, I guess many API and
functionality should be added to other modules. I won't maintain such
module externally, I need your commitment that smartcard are well
supported an have this as part of the roadmap and release engineer.
> 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.
This would be sad for both users and me, it would be more difficult to
maintain and support this module this way.
I don't understand what is the difference between the gcrypt, OpenSSL,
TSPI dependencies and the pkcs11-helper dependency. If you like I can
paste pkcs11-helper into this source, but I am sure it is not the
correct approach.
Pluggable engine is not generic enough, as a maintainer of
opencryptoki large complex and none flexible OpenSSL maintainer, I
think you understand that.
pkcs11-helper support multiple providers, supports session expiration,
serialization, offline mode, prompt hooks and much more, all these
features are a requirement for smartcard aware application, OpenSSL
engine nor opencryptoki provide a suitable interface for this.
Much work was invested to work with almost any PKCS#11 provider, as
minimal as it is. So people may actually use PKCS#11 enabled software,
not just hear that it supports PKCS#11 but not /your/ specific one.
> 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.
Most smartcards do not hold public key and cannot do the public cryptography.
Also I think we need to be capable of doing this even if the token is
unavailable.
Also if they support this, they do it very slow... So it does not
worth the effort.
But if you think this will perform better on external device I can do this.
But please understand that as many devices does not support much of
the operations, the OpenSSL dependency cannot be dropped.
BUT!!!
If you wish to do public key acceleration it does not mean you use the
SAME provider that performs private key operation.
For example, smartcards are very slow, but if you have an OpenSSL
engine for public key acceleration it can be used for public operation
while the slow smartcard will be used for private key operation only.
So I think there are different views regarding this matter. If you
wish to do optimization, we really need to separate between the public
and private operations and handle each independently.
> > + char dn[1024] = {0};
> > + char serial[1024] = {0};
>
> Magic numbers.
Just set a limit for these fields (display only), do you wish me to #define it?
X509_NAME_oneline does not return required size, so what do you suggest?
> > +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.
Yes.
I need you to add support for key module wide parameters...
For now I added a new file ~/.ecryptfsrc.pkcs11 but I think there
should be a generic way to pass parameters to key modules via the
~/.ecryptfsrc.
I am sure I did a lot of mistakes in parsing the file, I don't fully
understand your token parsing implementation, but it will do until you
provide more generic implementation.
It is quite funny... I invested more time on understanding the parser
than doing the other modifications... :)
>
> > + 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".
Don't you think it will confuse users?
Will you work on another PKCS#11 implementation (pure), are you sure
you achieve better results than my work?
Not that I care which name you give this module, it is just that if
you wish to work on a different implementation, I won't compete with
you. And if you don't have two implementation, pkcs11 is the name
which will not confuse users.
I wanted to help you guys, as I think this project is important, and
without hardware cryptography you don't achieve your goals.
And supporting dynamic (smartcard) hardware cryptography is something
very complex, and seldom done right.
This is where I come in, I help projects to achieve correct dynamic
hardware cryptography support into their applications using PKCS#11, I
wrote the pkcs11-helper in order to allow me to do just that, and as
you can see, a few hours of work worth and you have a solution.
I don't see any reason for writing the same low level PKCS#11 code
over and over in different projects, and I don't see the need of doing
this in this project.
But it is your decision.
Just tell me how you want to proceed.
> > + pkcs11h_setLogLevel (10);
>
> Another magic number.
Gone, now it is configuration.
> 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.
OK, we can add the spec into the build, so autoconf will add version
and package information. Tell me if you wish to do this.
A new version is available at:
http://alon.barlev.googlepages.com/ecryptfs-utils-23-alon-patches.tar.bz2
Old patches are prefixed with 0000.
Please send me exported trunk so I may resync.
Some notes, maybe I just don't understand eCryptfs well... But anyway...
1. Can you please describe how the usage of keyutils helps in securing
the environment? As far as I understand it is worse than, for example,
using ssh-agent, as credentials may be read anytime by many
applications.
2. Once ecryptfsd fails to decrypt a file, it seems it does not try
again. So if the token was unavailable and now it is, ecryptfsd does
not forward the new request to the key module. If I stop and start the
daemon all resume to work.
3. I think the API of key module should allow storing module scope
state and blob scope state, so that, for example, the RSA (OpenSSL)
may be stored and not recreated every time.
4. ecryptfsd does not unload key modules when it terminates.
5. I think there should be an option to automatically encrypt to other
public keys, keys such as recovery agents. This suggests that the
configuration file should be signed by the user.
6. We should discuss how the key module may conduct a user dialog via
the daemon, so that it can prompt the user for information.
7. It would be nice if a selection link list will be available as a
suggest value, so user may select a value from a list.
Best Regards,
Alon Bar-Lev.
-------------------------------------------------------------------------
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
[email protected]
https://lists.sourceforge.net/lists/listinfo/ecryptfs-devel
