Re: Review of SSH1 code removal

[Andreas Schneider <asn@xxxxxxxxxxxxxx>]

On Friday, 29 June 2018 11:27:11 CEST Jakub Jelen wrote:
> On Fri, 2018-06-29 at 10:09 +0200, Andreas Schneider wrote:
> > On Friday, 29 June 2018 09:41:25 CEST Jakub Jelen wrote:
> > > Changeset:
> > > https://git.libssh.org/users/asn/libssh.git/commit/?h=master-ssh1
> > 
> > Hi Jakub,
> > 
> > 
> > thank you very much for the review.
> > 
> > > client.c:
> > >              /* If SSHv1 is disabled, we can send the banner
> > > 
> > > immedietly
> > > */
> > 
> > Fixed.
> > 
> > >  -- there is no need to leave comments referencing ssh1 anymore.
> > > 
> > > obj/build_make.sh
> > > 
> > >                       OPTIONS="${OPTIONS} -DWITH_SSH1=ON"
> > 
> > Fixed.
> > 
> > >  -- This should not reference SSH1 either
> > > 
> > > buffer.{c,h}
> > > 
> > > struct ssh_string_struct *ssh_buffer_get_mpint(struct
> > > ssh_buffer_struct
> > > *buffer) {
> > 
> > Fixed.
> > 
> > >  -- this function is unused: * @note This function is SSH-1 only.
> > > 
> > > I am wondering what about the SSH_KEYTYPE_RSA1 key format. This is
> > > used
> > > all over the code and (even though it is probably unreachable) it
> > > should go away too. There is no good use of these keys in SSH2.
> > > 
> > > Otherwise the changes look good and running the tests on this
> > > branch
> > > gives no error.
> > 
> > Yes, you're right. I've handled them. We can't remove the enum as
> > this is
> > public API, but we should not handle it and return an error.
> > 
> > 
> > I've pushed an update to the branch. Could you take another look?
> 
> There are still several occurrences of rsa1 string throughout the code:
> 
> pki.{c,h}

Fixed.

>  -- ssh_pki_export_pubkey_rsa1 -- I don't think this one is needed
> anymore. If it is supposed to be left there as an API, it should be
> marked as deprecated.
> 
> 
> pki_{crypto,gcrypt,mbedcrypto}.c

Fixed.

>  -- pki_export_pubkey_rsa1 -- handling rsa1 keys is not needed anymore
> 
> 
> kex.c: ssh-rsa1

Fixed.

>  -- the ssh-rsa1 should no longer be preferred host key type
> 
> 
> known_hosts.c
> 
>  -- the old format parsing is still there, also comments mention this
> key type

Fixed.

> Just one more idea, about the known hosts, the unit tests should check
> if the known_hosts file can be parsed even if it contains the old
> known_hosts record after the parsing will be removed.

This known_hosts code is obsolete anyway. There is a complete new API now. It 
won't parse ssh-rsa1 keys as they are unknown. ssh_key_type_from_name() will 
return SSH_KEYTYPE_UNKNOWN and we will skip it.


Pushed update :-)
 
-- 
Andreas Schneider                 asn@xxxxxxxxxxxxxx
GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D