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

[Andrew Bartlett]

On Sun, 2006-09-24 at 01:54 +0300, Alon Bar-Lev wrote:
> 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.

It is always a challenge.  That's why I wanted to get the review process
started, with some easy stuff ;-)

> > > 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.

That's one approach.  Another is to clean things up really well, and
target the changes well, so that the developer feels that they simply
can't object to the patch :-).

> > 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.

I use ssh-agent without a gui all the time.  Why can't ssh-add prompt
for a pin just like it prompts for passphrases?

> > > 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.

Yeah, I think i wrote that comment before I finished my patch hacking
session.  My apologies.

Still, the less you mention X509 in terms of the SSH end, and present
this in terms of simply smartcards, the less red flags the OpenSSH
developers need to consider.  Fight the X.509 battle another day :-)

> > 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.

What a pity.  This should be looked into very carefully, as it would
drastically limit the usefulness of X.509 certs.

> > 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...

But if (as on Fedora Core 6, and RHEL5 betas) pam_pkcs11 is functional,
can we make use of it?  They seem to have it down to 'tick a box' for
smartcard login...

> > 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.

That's a rather large step, and in any case, the old UI will need to be
preserved.  

> > 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.

Looking at the ssh-agent code, your pkcs_11 mode shares no options in
common with the other smartcard code.  If that code is to be replaced
with yours, then users with scripts etc will break.  If the smartcard
code is not replaced, manpages get bigger to list both, and users become
confused 'which smartcard do I have?'.

> > 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.

It is a very large lump of code, but I'll leave that judgement to the
OpenSSH crew.

> > 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.

Why?  ssh-agent, like many other programs, needs to deal gracefully with
abnormal termination.  What happens if that _terminate isn't called?
(Because the process was killed in a nasty way?).

> > 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.

I'm saying that pkcs11_helper.c and basicly everything outside pkinit.c
feel like they belong elsewhere.  It is just a gut feeling that 'surely
the system should provide this'.

> > 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?

Current Portable CVS.

> 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.

Yes, I needed these headers to compile on Fedora Core 5.

Andrew Bartlett

-- 
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Red Hat Inc.                  http://redhat.com

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.mindrot.org/pipermail/openssh-unix-dev/attachments/20060923/f8658a3d/attachment.bin