OpenSSH PKCS#11merge

[Alon Bar-Lev]

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.