[PATCH] Re: PKCS#11 support in OpenSSH 4.3p2

[Alon Bar-Lev]

On Sunday 24 September 2006 01:16, Andrew Bartlett wrote:
> On Sat, 2006-05-27 at 17:26 +0300, Alon Bar-Lev wrote:
> > Hello,
> >
> > The version 0.11 of "PKCS#11 support in OpenSSH" is published.
>
> I cam across this patch recently, and I thought I might give you
> some feedback, because I would like to see it integrated into
> OpenSSH (I think it looks very useful in combination with
> pam_pkcs11 system logins).

Great!
I am happy to receive any feed-back.
I've been waiting to Damien Miller to have some free time.

> > A valid X.509 certificate should exist on the token, without
> > X.509 support it is exported as regular RSA key. There is a
> > simple utility Timo Felbinger wrote
> > (http://www.timof.qipc.org/x509toOpenSSH.c) that extracts
> > ssh public key from X.509 certificate.
>
> Is there a reason that this isn't integrated into the patch?

One reason is that I am not the author of this code.
Another reason is that my OpenSSH developers did not comment on this 
patch, and since they did not, I don't know what to pack yet.

> or does the ssh-keygen -D command 'do the right thing'?

When the PKCS#11 support will be integrated, one of the command-line 
utilities will have this functionality, also none daemon 
implementation will be added. And I hope that the ssh-agent protocol 
will be modified to allow using the agent in none-gui environment.

> > If you like X.509 support apply the X.509 (>=5.4) patch
> > *AFTER* the PKCS#11 patch.
>
> I'm puzzled by how much X.509 code there is in this patch.  It
> seems that this could be broken into two patches, one initial
> patch, and another to be applied with the X.509 patch.  (I've
> removed the X509 hook from the attached patch)

This is the only difference:
#if defined(SSH_PKCS11_X509_DISABLED)
                (*sshkey)->type = KEY_RSA;
#else
                (*sshkey)->type = KEY_X509_RSA;
                (*sshkey)->x509 = x509;
                x509 = NULL;
#endif

There is no reason to remove anything. The patch will work with or 
without X.509 as expected.

> That said, I think the handling of X.509 or ssh-rsa should be
> better arranged.  The documentation implies that I must set '-o
> PubkeyAlgorithms=ssh-rsa' if I want to do 'normal' RSA
> authentication if the X.509 patch is supported in the client. 
> Surely both should be offered, and it should be for the server to
> decline?

Not exactly.
Roumen Petrov explained to me that there is somekind of limit in the 
negotiation stage. So the user should specify if he wishes downgrade.

> Have you looked at how this integrates with pam_pkcs11?  Is it
> practical to have some of this inherit the fact that PAM has chosen
> a certificate to log in with, and have this be the default cert?

I think pam_pkcs11 is much too complex for users... And I don't like 
defaults... They tend to not work for most of people.
I have a script that add my identities into the agent... It runs on 
logon,  it is very nice...

> Also, it seems to duplicate a lot of the smatcard code:  Should
> this be configured under SMARTCARD instead, and provide the
> sc_open() etc interface?

I think that all smartcard related code (opensc and javacard) be 
considered to be removed after a standard PKCS#11 implementation is 
added.

> The same would apply for the user interface:  I know we might need
> to specify the provider, but should the user see the gory details,
> when they just want to put in their smartcard?  At least share that
> with the existing smartcard support, please.

I don't understand.

> On matters of code, I note:
>
> The patch patches autogenerated files (ususally I would just advise
> users that they need to re-run autoconf and autoheader).

Most of the users do not know how to use these tools... Since the auto 
stuff is located at the formal package, I patch it.

> The use of _DISABLED macros seems odd.  I've tried to change this
> to be in line with the existing SMARTCARD support as much as
> possible.

Since this code adds no external dependency, such as opensc, there is 
no need for default disabled.

> In ssh-agent.c, the pkcs11_terminate() call is right after the
> while(1) without a break, and just below the /* NOTREACHED */
> comment.  I presume this call belongs in the cleanup_handler() and
> cleanup_socket().

True...
Well... I kind of hope that a cleaner exit will be applied in the 
future into ssh-agent.

> The code seems to include a lot of what I would have expected to be
> system library code.  Is there a suitable, widely
> deployed/available system library that can (at least optionally)
> handle some of this?

I don't understand... Please explain.

> On licencing, I note the licence on cryptoki.h has the advertising
> clause.  You did the right thing by adding it to LICENCE, but I'm
> not sure if it is accepted by OpenSSH in general.  (I understand
> the other advertising clause, in the Regents of UC licence of
> loginrec.c is inoperative, having been repealed by the regents,
> just not updated in code).

We will wait and see... The answer should be provided by OpenSSH 
developers.

> I've built the patch against current OpenSSH (updated patch
> attached, for most of my comments above), and as soon as I get a
> soft token working I'll give it a spin.

Thanks.
It is not applied cleanly. Which version did you use?
But as I said... the PKCS#11 should be default on, the modification of 
the pre-compiler constant will be decided when merging occurs, 
Why did you add all these string.h, errno.h #include directive? Did 
you have some problem? Nobody reported such... yet.

Thanks for the feed-back!
Best Regards,
Alon Bar-Lev.