Handle full parsing of authorized_keys file

[Rocco83]

Hi,

as by #21 would be wonderful to handle full parsing of authorized_keys files, including empty and commented-out lines.

I will try to manage in the future a PR, if it will not happens before.

Thanks, Daniele

[rarylson]

Hi @Rocco83, handling authorized_keys files is a very good enhancement. But if you make a PR in a future to implement this, please remember that authorized keys can have options prefixing a key, like in:

command="/usr/bin/tinyfugue" ssh-rsa AAAAB3NzaC1yc2EAAA...OFy5Lwc8Lo+Jk=

This is why not all public key library handle authorized_keys files, since it is not a trivial task (only ignoring comments and empty lines).

Full parsing of authorized keys should not return a list of keys, but a list of AuthorizedKeyLine (or AuthorizedKeyItem, for example), and each authorized_keys line/item must have a key (required) and an optional list of options.

The full syntax is described at the sshd man: http://man.openbsd.org/sshd.8#AUTHORIZED_KEYS_FILE_FORMAT

[ojarva]

@rarylson the library already parses full lines, including command, and other options. The only missing thing is excluding comments and empty lines.

[rarylson]

Hi @ojarva, i did not realize that SSHKeys already parse options. Thanks for the AuthorizedKeysFile feature. It's a very cool feature.

Some points:

1) Second to the specification, starting a line with spaces is not allowed. However, OpenSSH implementation strips spaces (spaces at line start, or multiple spaces between options and keys), so you are right when striping spaces in parse method!

static int
check_authkeys_file(struct ssh *ssh, struct passwd *pw, FILE *f,
    char *file, struct sshkey *key, struct sshauthopt **authoptsp)
{
        [...]
        /* Skip leading whitespace, empty and comment lines. */
        cp = line;
        skip_space(&cp);
        if (!*cp || *cp == '\n' || *cp == '#')
            continue;
        snprintf(loc, sizeof(loc), "%.200s:%lu", file, linenum);
        [...]
}

[...]
       if (advance_past_options(&cp) != 0) {
            reason = "invalid key option string";
            goto fail_reason;
        }
        skip_space(&cp);
        if (sshkey_read(found, &cp) != 0) {
[...]

2) OpenSSH implementation keeps sshkey and sshauthopt as two different concepts (the methods check_authkey_line and check_authkey_file as examples - check_authkey_line gets sshkey and sshauthopt for each line, and not an sshkey which contains a list of sshauthopt).

However, in some specifications that I read the authorized_keys options are mentioned as properties of the public SSH key. So, in this case, your implementation (where options are parsed inside the 'SSHKey' class, and a SSHKey object having options) seems to be ok.

For other side, I have an argument that may indicate SSHKey and AuthorizedKeyFileOption should be two different classes (see point 3).

3) An authorized_keys can have only options in a line (without an associated key)

From the IBM ZOS docs: https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.3.0/com.ibm.zos.v2r3.foto100/authkeyf.htm

Protocol version 2 public keys that are in a key ring only consist of options, one of which must be the zos-key-ring-label option.

From OpenSSH source code, in check_authkey_line, after processing options, if there is no key, it just go to next line (so lines with only options are allowed):

    if (sshkey_read(found, &cp) != 0) {
        /* no key?  check for options */
        debug2("%s: check options: '%s'", loc, cp);
        key_options = cp;
        if (advance_past_options(&cp) != 0) {
            reason = "invalid key option string";
            goto fail_reason;
        }
        skip_space(&cp);
        if (sshkey_read(found, &cp) != 0) {
            /* still no key?  advance to next line*/
            debug2("%s: advance: '%s'", loc, cp);
            goto out;
        }
    }

If you want to allow authorized_keys lines with only options (without a key, just with options), maybe would should move the option parsing to an AuthorizedKeyFileOption object or to create a new class that inherit from SSHKey class (in addition to RSA, DSA, and others) that only accepts options (example: OnlyOptions). So, sshpubkeys.SSHKey('zos-key-ring-label').parse() should return an OnlyOptions instance.

However, I only heard about lines with only options (without keys) in IBM ZOS. So, skip parsing this very specific cases will be ok for almost all cases.

[ojarva]

@rarylson very good points, thanks for being so thorough with this! As you noted, the current implementation throws an error for any lines with options and without valid key data.

For IBM ZOS, it is very nice to have sources and details about various implementations. However, I don't think it makes sense to try to accomodate every possible implementation - in this case I think IBM ZOS specific options (for example, zos-key-ring-label) will not be supported by this library.

I'm not sure what should be done with this. OTOH if OpenSSH just ignores option-only lines, it makes sense to do the same. OTOH if that is more or less incorrect data, maybe it makes more sense to fail in that case.

Also, tectia ssh seems to have a separate authorizations (not to be confused with authorized_keys) file with a bit different file format: https://www.ssh.com/ssh/authorized_keys/ - that is definitely out of scope for this library.

If you think option-only lines should be parsed in authorized_keys, could you please open a new issue regarding that, and let's move discussion there?

[rarylson]

For now, I think the currently implementation is very good. It will work very well in almost all cases.

If someday some IBM ZOS user (or someone else using only-option lines) asks for this, when this occur, so you worry about it (if you remember this discussion, I hope it can help you thinking about the best solution). But may not be worth to worry about it now.

Again, thank you for implementing AuthorizedKeysFile parsing.