Re: [PATCH] Support for private keys in OpenSSH format

[Andreas Schneider <asn@xxxxxxxxxxxxxx>]

On Thursday, 6 September 2018 18:30:11 CEST Jakub Jelen wrote:
> Hello all,

Hi Jakub,

> the attached patch set provides support and tests for all the supported
> key types in new OpenSSH format, which is default since OpenSSH 7.8. It
> is also available in gitlab mirror:
> 
> https://gitlab.com/jjelen/libssh-mirror/tree/openssh-keys

thank you very much for your contribution. I've added comments on gitlab.

> Applying this patch set will allow us to revert commit 100c9c98, which
> modified the pkd test to generate keys in old format, but it is not a
> requirement.

I think we should revert it, as you've already done.

> The code is generally functional, but in crypto backend, I did not
> implement calculation of additional RSA parameters, that are not part
> of the key format, but can be calculated from others. OpenSSL can work
> without these parameters, but might be slower.
> 
> We can inspire in OpenSSH code, which calculates these parameters the
> following way:
> 
> https://github.com/openssh/openssh-portable/blob/master/ssh-rsa.c#L107
> 
> I will be on vacation until next Tuesday, so I will address potential
> review comments later.

I will be on vacation from Wednesday till Friday next week :-)

If possible it would be great if you could check that we can load public keys 
from the new container type e.g. with ssh_pki_import_pubkey_file(). That 
should be possible as the new container includes them.
I think we need some tests for this.

Also we have a lot of code duplication in the tests. Often the tests for rsa, 
dsa, ecda and ed25519 are completely the same. So it would be nice to abstract 
some of this in a function the tests could call with different parameters.


Thanks,


	Andreas

-- 
Andreas Schneider                 asn@xxxxxxxxxxxxxx
GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D