OpenSSH PKCS#11merge

[Peter Stuge]

Some comments from a quick read of the patch. I did not look at what
the code does in great detail.


On Sat, Sep 29, 2007 at 03:36:28PM +0200, Alon Bar-Lev wrote:
> https://bugzilla.mindrot.org/show_bug.cgi?id=1371

* Upstream

I don't know what OpenBSD thinks about PKCS#11 but unless they
fiercely reject it, I think the patch should be worked into the
OpenBSD version of OpenSSH first, instead of the portable tree.


* X.509

I agree with your reasoning that PKCS#11 goes well with X.509.

However, since the X.509 patch is not included in OpenSSH, I don't
think there should be anything X.509-related in the p11 patch.

--8<-- config.h.in
+/* Define if you want to use X509 with PKCS11 */
+#undef ENABLE_PKCS11_X509
-->8--

I think this and everything else about X.509 should go into Roumen's
X.509 patch instead.


* Related changes

--8<-- pkcs11.c
+        * WARNING!!!
+        * There is no way to get log level,
+        * so set to minimum.
+        * After fix in log.c it can be fixed.
-->8--

What fix is needed in log.c? Maybe you already have a patch?


--8<-- ssh-add.c
+        * TEMP TEMP TEMP TEMP
+        *
+        * This should be fixed if another mechanism
+        * will be propsed.
-->8--

I for one would like to avoid temporary solutions that are known to
be incorrect but work well enough. You mentioned extending the agent
protocol. Can you expand on that a little? Is it really neccessary?
Can't the agent figure out when it needs help from the user just from
how it is being used without actually being instructed by anyone?


If, in your opinion, the p11 patch can do everything that the current
OpenSC support can, I think the current code should be removed and
replaced with a compatibility layer that uses p11 to do what direct
OpenSC use does now, and maybe even removed completely some time long
in the future.


* Coding style

The patch doesn't follow the OpenSSH coding style. pkcs11_show_ids()
is pretty crazy.


* Copyright

> + * The ssh_from_x509 is dirived of Tatu and Markus work.

Please put all copyright notices in a file at the top of the file.


* do_pkcs11()

Implementing a (file-)local getopt_long() surely can't be a good
idea?


//Peter