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 [email protected]
GPG-ID: 8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D
