OpenSSH PKCS#11merge

[Alon Bar-Lev]

Hello Peter,

I will be happy to continue working with you on this one... I hope you
did not give up :)

The major issue I need to know:
a. Do you think the agent protocol should be modified, as per my explanation?
b. Do you think the ssh-agent may read ssh_config file for options?
c. Do you think the utility that shows available PKCS#11 ids be part
of ssh-add or separate utility?
d. I need allocation of options (short parameter names) for PKCS#11 options.

Best Regard,
Alon Bar-Lev.

On 9/29/07, Alon Bar-Lev <alon.barlev at gmail.com> wrote:
> On 9/29/07, Peter Stuge <stuge-openssh-unix-dev at cdy.org> wrote:
> > Some comments from a quick read of the patch. I did not look at what
> > the code does in great detail.
>
> Thank you so much for your time!
>
> > * 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.
>
> Oh... I will be glad to do so, but I don't have a test environment for this...
> I don't think I've done anything that prevent it to work with the
> OpenBSD version... :)
> If this is a requirement to proceed I will setup my first OpenBSD machine :)
>
> > * 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.
> > I think this and everything else about X.509 should go into Roumen's
> > X.509 patch instead.
>
> I agree.
> I've talked to Roumen in the past regarding this, if PKCS#11 support
> is merged then the X.509 patch will enable PKCS#11 X.509 features.
> But as long as this is an external patch, it is comfortable to users
> to have this packaged this way.
>
> But regardless the above, a self-signed X.509 certificate is needed in
> the public area of the token, in order to extract the public key. This
> is required as many providers stores only X.509 certificate and
> private key objects.
>
> > * 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?
>
> Oh...
> Simple...
> Just to be able to return current log level... :)
>
> Just return log_level from log.c, something like:
>
> LogLevel
> get_log_level ()
> {
>     return log_level;
> }
>
> > --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?
>
> This is the major change all smartcard components requires, there are
> some opened issues in bugzilla regarding this.
>
> Smartcards are dynamic in nature unlike file based keys, smartcards
> can be removed and inserted by the users, also the session between the
> application and the smartcard is sometime time limited.
>
> When smartcards are also used in order to open the door for your
> office, it tends to become even more dynamic.
>
> There are two kinds of user prompts that an smartcard enabled
> application needs to have:
>
> 1. Token prompt - When key should be used but the hardware device is
> not available the application should prompt the user to insert his
> token. This is very important when re-negotiation is performed, as
> users don't expect session disconnect because their token is not
> available.
>
> 2. Passphrase prompt - Private key is used first time on session, may
> be triggered when:
> a. A new session is created.
> b. Card was removed and inserted, this actually forces application to
> create a new session.
> c. Provider forces session duration timeout, this also forces
> application to create a new session.
>
> Because I did not wanted to touch more than I needed, I currently
> implemented these callbacks using external program the ssh-agent
> execute when needed.
>
> But a much cleaner solution would be modifying the ssh-agent protocol
> so that it inform the forground application to perform the prompt.
>
> So basically SSH2_AGENTC_SIGN_REQUEST and  SSH_AGENTC_PKCS11_ADD_ID
> may return an error with indication that a user interactive prompt is
> needed with the details for the prompt (for example a string and type
> of required prompt), so that application may handle the user
> interaction.
>
> There should be added some more command, such as introduce key
> passphrase and optionally wait for token.
>
> > 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.
>
> Yes, it does anything and much more, many OpenSC users already using
> this patch as it support dynamic smartcard availability and adding
> identities into the agent without card available.
>
> I think OpenSSH should support only PKCS#11 for hardware cryptography,
> making the code much cleaner.
>
> I suggested this in the past but it was rejected by some OpenSC users,
> as I only updated the agent, while current OpenSC support does not
> require agent configuration.
> I have implemented agent only configuration to minimize my
> modification, but if we proceed in merging I will add this support.
> One thing we need to discuss is if we add all keys (as in OpenSC case)
> or only user requested ones. I don't like adding all keys... :)
>
> BTW: I don't understood why ssh does not execute ssh-agent internally
> if one is not available... GnuPG does this and it seems to minimize
> code duplication...
>
> > * Coding style
> >
> > The patch doesn't follow the OpenSSH coding style. pkcs11_show_ids()
> > is pretty crazy.
>
> Is there any document I can read?
> I will convert it to whatever style needed.
> The pkcs11_show_ids is crazy as we need to discuss where you want to
> put this one... It can be exported to a standalone executable... Or
> stay within this one... Also we should discuss the output format that
> looks right for you...
>
> > * 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.
>
> Will do.
>
> > * do_pkcs11()
> >
> > Implementing a (file-)local getopt_long() surely can't be a good
> > idea?
>
> I don't follow you... Can you please explain...
>
> In the past I asked to allocate some BSD style short option letters
> for this one... :)
>
> But I think most options should go into configuration file, but the
> ssh-agent does not currently have configuration file, so in order to
> minimize my changes I've added these long options. Maybe we can make
> ssh-agent read the ssh_config and take some options from there?
>
> The ssh-add will only requires --pkcs11-add-id, --pkcs11-remove-id and
> optionally --pkcs11-show-ids.
>
> Thanks for quick reviewing,
> Alon Bar-Lev.
>