Re: Review of SSH1 code removal

[Jakub Jelen <jjelen@xxxxxxxxxx>]

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}

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

 -- pki_export_pubkey_rsa1 -- handling rsa1 keys is not needed anymore


kex.c: ssh-rsa1

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

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.

Jakub