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
