ssh_key::Certificate fails to parse certificates created without an expiration date

[rexxnor]

Hi,

Upon trying out the certificate parser to learn about OpenSSH's certificate structure I discovered that certificates generated without an expiry date fail to be parsed with a Time error.

When creating a certificate without expiry OpenSSH decides to set the valid_after to 0 and valid_before to i64::Max which is a lot higher than the limit discussed in ssh-key/src/certificate/unix_time.rs L11-17. While I do understand that absurd values make no sense I think failing to parse a perfectly valid certificate is bad.

Could you maybe add a special case to UnixTime specifically for i64::MAX that denotes the key does not expire?

Steps to reproduce (OpenSSH 9.5):

$ cd $(mktemp -d)
$ ssh-keygen -f ca

$ ssh-keygen -f test

$ ssh-keygen -I foobar -s ca test.pub
Signed user key test-cert.pub: id "foobar" serial 0 valid forever

$ cargo init --name sshkey_test
$ cargo add ssh_key -F ed25519

$ #cat > src/main.rs << EOF
use std::path::Path;
use ssh_key::certificate::Certificate;

fn main() {
    let mut args = std::env::args();
    args.next().unwrap();
    let cert_file_arg = if let Some(arg) = args.next() {
        arg
    } else {
        eprintln!("Must supply a path to a an OpenSSH certificate file");
        std::process::exit(-1);
    };

    let cert_file_path = Path::new(&cert_file_arg);
    let certificate = Certificate::read_file(cert_file_path);

    dbg!(&certificate);
}
EOF

$ cargo run -- test-cert.pub
   Compiling sshkey_test v0.1.0 (/tmp/tmp.umUnQAJI6F)
    Finished dev [unoptimized + debuginfo] target(s) in 0.55s
     Running `target/debug/sshkey_test test-cert.pub`
[src/main.rs:17] &certificate = Err(
    Time,
)

[tarcieri]

It would probably just make sense to get rid of MAX_SECS if this (mis)feature (ab)uses timestamps this way

[rexxnor]

That works as well. I guess the topic should be discussed in the OpenSSH bugtracker if at all because I am not quite sure how I would implement this given the requirement for protocol/format compatibility across versions.

[tarcieri]

MAX_SECS is now i64::MAX as of #175

[rexxnor]

I just tried it with the new 0.6.3 release and the sample certificate still fails to parse.

I think this is due to the max being defined as i64::MAX and not u64::MAX. According to the certificate protocol specification valid_before and valid_after should use uint64 which is u64 and not i64 in Rust.

This function that checks certificate time validity in OpenSSH might be of interest.

[tarcieri]

Note the value is defined in terms of u64: https://github.com/RustCrypto/SSH/pull/175/files#diff-848d070a3feca06736f81c1b69b7b3c5173b241928e64962c65a79898e5edc32R12

However u64::MAX isn't a valid Unix timestamp (or rather, it would be a negative one), since Unix timestamps are signed since they can represent values before the Unix epoch.

I guess I'll need to add a test vector for one of these certificates, but this is absolutely obnoxious, and a unexpiring certificates are a horrible antipattern (which appears to be the default? What were they thinking?)

[tarcieri]

Fixing this will likely require breaking changes, since I guess we'll need to replace every single place a certificate can expire with an Option, since they chose to use a bogus timestamp as a placeholder for unexpiring certificates. We currently support conversions to SystemTime, for example, and u64::MAX will cause those conversions to panic unless special cased.

Will go ahead and reopen this, but I'm not looking forward to solving it. It would seem to create a lot of opportunities for bugs in something that was otherwise straightforward, to support something which is a bad idea to begin with.