Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

Fix Net::SSH::Perl::Key->extract_public() #12

Merged
merged 2 commits into from
Apr 21, 2016

Conversation

reyjrar
Copy link
Contributor

@reyjrar reyjrar commented Feb 5, 2016

The docs incorrectly specified the function took 2 parameters, the type
and the base64 encoded string. This might be due to confusion on lvalue
list/context, vs rvalure list Array context;

my @array = qw(a b c);
my ($x) = @array;      # $x = 'a'
my ($x) = qw(a b c);   # $x = 'a'
my $x   = @array;      # $x = 3
my $x   = qw(a b c);   # $x = 'c'

The new behavior correctly handles:

my $k = Net::SSH::Perl::Key->extract_public('DSA', $key_string);

and:

my $k = Net::SSH::Perl::Key->extract_public($key_string);

The only difference is when 2 args are sent and the first arg differs
from the detected key type, a warning is thrown.

The reason for supporting the single argument in this way is due the
interface being documented incorrectly. This single argument version is
the only version that actually worked in the past, despite being
undocumented. This patch maintains the hidden backwards compatibility
for others who may have code depending on this module.

Patch the extract_public() and new() to add the ability to extract the
comment field too. Patch DSA/RSA dump_public() to write the comment if
it exists.

Create a t/07-keys.t which reads t/authorized_keys and makes sure that
extract_public() is working correctly. The data for the tests comes
from the comment fields of the keys. These can be add by:

ssh-keygen -t <type> -b <bits> -C "type=RSA|DSA,size=<bits>"

For keys that you know should fail, adding a "FAIL" to the comment will
pass the test.

The docs incorrectly specified the function took 2 parameters, the type
and the base64 encoded string.  This might be due to confusion on lvalue
list/context, vs rvalure list **Array** context;

    my @array = qw(a b c);
    my ($x) = @array;      # $x = 'a'
    my ($x) = qw(a b c);   # $x = 'a'
    my $x   = @array;      # $x = 3
    my $x   = qw(a b c);   # $x = 'c'

The new behavior correctly handles:

    my $k = Net::SSH::Perl::Key->extract_public('DSA', $key_string);

and:

    my $k = Net::SSH::Perl::Key->extract_public($key_string);

The only difference is when 2 args are sent and the first arg differs
from the detected key type, a warning is thrown.

The reason for supporting the single argument in this way is due the
interface being documented incorrectly.  This single argument version is
the only version that actually worked in the past, despite being
undocumented.  This patch maintains the hidden backwards compatibility
for others who may have code depending on this module.

Patch the extract_public() and new() to add the ability to extract the
comment field too.  Patch DSA/RSA dump_public() to write the comment if
it exists.

Create a t/07-keys.t which reads t/authorized_keys and makes sure that
extract_public() is working correctly.  The data for the tests comes
from the comment fields of the keys.  These can be add by:

    ssh-keygen -t <type> -b <bits> -C "type=RSA|DSA,size=<bits>"

For keys that you know should fail, adding a "FAIL" to the comment will
pass the test.
@renormalist renormalist self-assigned this Feb 8, 2016
* Fix a bug in the Net::SSH::Perl::Key::RSA.
* Handle parsing SSH authorized_keys file options string prefixing.

This patch looks explicitly for the placement of the key typ
designations in each line it's passed.  It then uses that offset to
split the line into the options string and the keystring.  Currently,
the options string is just silently discarded, but it could be added to
the key object to allow for output like this in the dump_public()
routine:

    from="*.example.com" ssh-rsa <key> <comment>

Updated the tests to handle this as well.
@renormalist renormalist merged commit 36b7d44 into renormalist:master Apr 21, 2016
@renormalist
Copy link
Owner

Thanks. Sorry for the late response. I think that patch makes sense. And if there really should be an issue with backwards compatibility then now is a good time as I try to release a major version number for the AES stuff.

lkinley pushed a commit to lkinley/Net-SSH-Perl that referenced this pull request May 11, 2016
- Add ECDSA key support
- Improve extract_public() in Key.pm inspired by
  renormalist/Net-SSH-Perl#12
  but implement comment with backwards compat with RSA/DSA datafellows
- Fix XS from being loaded more than once (warnings from Net::SFTP)
renormalist pushed a commit that referenced this pull request Mar 12, 2017
 - Add ECDSA key support
 - Improve extract_public() in Key.pm
   inspired by #12
   but implement comment with backwards compat with RSA/DSA datafellows
 - Fix XS from being loaded more than once (warnings from Net::SFTP)
renormalist added a commit that referenced this pull request Mar 12, 2017
Sorry all, that I messed up the master branch
with merges I tested too late and pushed too
fast to github.

This commit here is in fact a simple reverse
patch of the differences that piled up
between the v2 and the broken master so that
the v2 can become master again.

Later I can retry to resolve the pull requests
(PR #11 and #12) that I actually wanted to have
in here.
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Apr 7, 2017
--------------------------------------
2.09 2016.10.26
    - Fix creation of keys in ecdsa, ed25519 key classes
    - Update eg/pssh-keygen to create ecdsa, ed25519 keys
    - Handle hostkeys-00@openssh.com global requests
    - Add support for 'CheckHostIP' and 'UpdateHostKeys' config options
    - Refactor handling of '+' syntax in options
    - Key fingerprints now output sha256-base64 by default.
      (md5 can be specified with FingerprintHash config option)
    - Add id_ed25519, id_ecdsa to default identity files
    - Documentation updates in Perl.pm to reflect new functionality in 2.XX

2.08 2016.10.14
    - Use sha512 instead of md5 in Net::SSH::Perl::Cipher->new_from_key_str()
      to provide ChachaPoly with enough key material
      Tests in t/05-cipher.t should now pass on all platforms [ CPAN bug #114077 ]
    - Add AES128_CBC to cipher tests
    - Info on using features not enabled by default added to README

2.07 2016.10.13

    - Fix blowfish compilation on SunOS [CPAN bug #116323]
    - Fix bug in Packet [CPAN bug #118335]
    - Add support for '+' syntax in MACs option
    - Remove hmac-sha1 from default MACs. It can re-enabled
      by passing the option: 'MACs +hmac-sha1'

2.06 2016.10.04

    - Add support for additional fixed Diffie-Hellman 2K, 4K and 8K groups
      from OpenSSH 7.3 (draft-ietf-curdle-ssh-kex-sha2-03)
    - Kex defaults now updated to draft-ietf-curdle-ssh-kex-sha2-03
      recommendations (diffie-hellman-group-exchange-sha1 removed)
      It can re-enabled by passing the option:
      'KexAlgorithms +diffie-hellman-group-exchange-sha1'

2.05 2016.10.03

    - Add support for '+' syntax in Ciphers, KexAlgorithms, HostKeyAlgorithms
      options as in OpenSSH

2.04 2016.05.11

    - Add ECDSA key support
    - Improve extract_public() in Key.pm inspired by
      renormalist/Net-SSH-Perl#12
      but implement comment with backwards compat with RSA/DSA datafellows
    - Fix XS from being loaded more than once (warnings from Net::SFTP)

2.03 2016.05.06

    - Fixes so that "make test" passes

2.02 2016.05.04

    - Use CryptX to further reduce module depedencies
      This eliminates the need for:
        Math::Pari
        Crypt::DH
        Crypt::RSA
        Crypt::DSA
        Crypt::DES
        Crypt::Blowfish
        MIME::Base64
    - Add support for rsa-sha2-512,rsa-sha2-256 signing with RSA keys
    - Implement HashKnownHosts, KexAlgorithms, MACs config directives
    - Add XS code for Chacha20, BSD Blowfish, Ed25519 routines
    - Properly handle and create known_hosts entries when port is specified
    - Remove obsolete ciphers, MACs, Kex from default list to duplicate
      upcoming OpenSSH behavior
    - Bug fixes

2.01 2016.02.19

    - Use CryptX to reduce module depedencies
      This eliminates the need for:
        BSD::arc4random
        Digest::MD5
        Digest::SHA
        Digest::HMAC_MD5
        Crypt::OpenSSL::AES

2.00 2015.12.07

    - Add Chacha20-Poly1305 cipher support for best security
      (Requires Crypt::OpenSSH::ChachaPoly, see README)
    - Add AES Cipher support in CTR mode (CBC mode supported in Ed25519
      keys only)
    - Add Group Exchange (RFC4523) Diffie-Hellman Key Exchange
    - Add Curve25519 (curve25519-sha256@libssh.org) Key Exchange support
      (Requires Crypt::Curve25519)
    - Add hmac-sha2-256,hmac-sha2-512 MAC support
    - Add hmac-sha2-256-etm@openssh.com,hmac-sha2-512-etm@openssh.com
      Encrypt-then-MAC (ETM) MAC support
    - Use BSD::arc4random for encrypted packet padding
    - Add support for Ed25519 ssh/host keys (Requires Crypt::Ed25519)
      Encrypted Ed25519 key support requires Crypt::OpenBSD::Blowfish
      (See README for info)
    - Default ciphers order is now chacha,aes,3des,blowfish,arcfour
    - Default KEX order is now Curve25519, DHGEXSHA256, DHGEXSHA1, DH14, DH1
    - Default MAC order is now hmac-sha2-512-etm@openssh.com,
      hmac-sha2-256-etm@openssh.com, sha2-512, sha2-256, sha1, md5
    - SSH Keys can now be in DOS format (no need to remove CR/LF)
    - SOCKS proxy support via sub class Net::SSH:Perl::Proxy
    - Now does not abort due to OpenSSH 6.8+ server
      SSH2_MSG_GLOBAL_REQUEST messages for host key rotation

(pkgsrc changes)
    - Adjust DEPENDS base upon above note (p5-CryptX related)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants