From 0f86e71bf64a26e35775c2cf6be4e00361d22b0d Mon Sep 17 00:00:00 2001 From: Duc Thanh Nguyen Date: Sun, 27 Sep 2026 13:22:57 -0400 Subject: [PATCH 1/2] fix(ssh): write allowed signers in the format OpenSSH accepts AllowedSigner's Display output was rejected by OpenSSH and by this crate's own parser. Options were space-separated and unquoted, and timestamps were written as raw UNIX epochs. Display now writes comma-separated options with quoted values, timestamps as YYYYMMDDHHMMSSZ clamped to the range OpenSSH accepts, escapes quotes in namespaces, and quotes principals containing whitespace or '#'. The parser now follows the ssh-keygen(1) ALLOWED SIGNERS format: options are a single comma-separated field, so space-separated options are rejected. It also accepts tabs between fields and a case-insensitive Z or UTC timestamp suffix, and rejects unknown options, empty options, cert-authority with a value, and stray quotes. --- src/ssh/allowed_signer.rs | 173 +++++++++++++++------ tests/allowed-signer.rs | 160 ++++++++++++++++--- tests/allowed_signers/good_allowed_signers | 4 +- 3 files changed, 260 insertions(+), 77 deletions(-) diff --git a/src/ssh/allowed_signer.rs b/src/ssh/allowed_signer.rs index 40d6f9a..554161e 100644 --- a/src/ssh/allowed_signer.rs +++ b/src/ssh/allowed_signer.rs @@ -4,7 +4,7 @@ use std::io::Read; use std::path::Path; use chrono::prelude::Local; -use chrono::{Duration, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, TimeZone}; +use chrono::{DateTime, Duration, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, TimeZone}; use super::pubkey::PublicKey; use crate::{error::Error, Result}; @@ -102,6 +102,9 @@ impl AllowedSigner { let principals = tokenizer.next(true)? .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingPrincipals))?; let principals = principals.trim_matches('"'); + if principals.contains('"') { + return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); + } let principals: Vec = principals.split(',') .map(|s| s.to_string()) .collect(); @@ -114,10 +117,18 @@ impl AllowedSigner { let mut valid_after = None; let mut valid_before = None; - let kt = loop { - let option = tokenizer.next(false)? - .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingKey))?; + let field = tokenizer.next(false)? + .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingKey))?; + + // The options field is optional. Key types never contain '=' or ',', and the only option + // without either is cert-authority. + let options = if field.contains(['=', ',']) || field.eq_ignore_ascii_case("cert-authority") { + split_options(&field) + } else { + Vec::new() + }; + for option in &options { let (option_key, option_value) = match option.split_once('=') { Some(v) => v, None => (option.as_str(), ""), @@ -128,7 +139,14 @@ impl AllowedSigner { } match option_key.to_lowercase().as_str() { - "cert-authority" => cert_authority = true, + "cert-authority" => { + if option.contains('=') { + return Err( + Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption("cert-authority".to_string())) + ); + } + cert_authority = true; + }, "namespaces" => { if namespaces.is_some() { return Err( @@ -167,9 +185,25 @@ impl AllowedSigner { |_| Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption("valid-before".to_string())))? ); }, - // If option_key does not match any valid option, we test if it's the key data - _ => break option, + // An empty option comes from a stray comma, so report the whole field + "" => { + return Err( + Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(field.clone())) + ); + }, + _ => { + return Err( + Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(option_key.to_string())) + ); + }, }; + } + + let kt = if options.is_empty() { + field + } else { + tokenizer.next(false)? + .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingKey))? }; let key_data = tokenizer.next(false)? @@ -209,22 +243,34 @@ impl fmt::Display for AllowedSigner { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let mut output = String::new(); - output.push_str(&self.principals.join(",")); + let principals = self.principals.join(","); + if principals.contains([' ', '\t', '#']) { + output.push_str(&format!("\"{}\"", principals)); + } else { + output.push_str(&principals); + } + // OpenSSH requires comma-separated options with quoted values + let mut options = Vec::new(); + if self.cert_authority { - output.push_str(" cert-authority"); + options.push("cert-authority".to_string()); } if let Some(ref namespaces) = self.namespaces { - output.push_str(&format!(" namespaces={}", namespaces.join(","))); + options.push(format!("namespaces=\"{}\"", namespaces.join(",").replace('"', "\\\""))); } - if let Some(ref valid_after) = self.valid_after { - output.push_str(&format!(" valid-after={}", valid_after)); + if let Some(valid_after) = self.valid_after { + options.push(format!("valid-after=\"{}\"", format_timestamp(valid_after))); } - if let Some(ref valid_before) = self.valid_before { - output.push_str(&format!(" valid-before={}", valid_before)); + if let Some(valid_before) = self.valid_before { + options.push(format!("valid-before=\"{}\"", format_timestamp(valid_before))); + } + + if !options.is_empty() { + output.push_str(&format!(" {}", options.join(","))); } output.push_str(&format!(" {}", self.key)); @@ -297,8 +343,8 @@ impl AllowedSigners { /// The splitter is highly aware of the allowed_signer format and will catch certain invalid /// formats. /// -/// For example: "principals option1=\"value1 value2\" option2 option3=value kt key_data" is split into -/// ["principals", "option1=\"value1 value2\"", "option2", "option3=value", "kt", "key_data"] +/// For example: "principals option1=\"value1 value2\",option2,option3=value kt key_data" is split into +/// ["principals", "option1=\"value1 value2\",option2,option3=value", "kt", "key_data"] struct AllowedSignerSplitter { /// A buffer of remaining tokens in reverse order. buffer: Vec, @@ -310,7 +356,7 @@ impl AllowedSignerSplitter { let mut buffer = Vec::new(); let mut last = 0; - for (index, matched) in s.match_indices([' ', '"', '#']) { + for (index, matched) in s.match_indices([' ', '\t', '"', '#']) { // Push the new text before the delimiter if last != index { buffer.push(s[last..index].to_owned()); @@ -336,7 +382,7 @@ impl AllowedSignerSplitter { return self.buffer.is_empty(); } - /// Get the next part that is not an option (principals, key) + /// Get the next whitespace-separated field (principals, options, key type, key data) /// If opening_quotes_allowed is set to false, we reject the next token if it starts with ". fn next(&mut self, opening_quotes_allowed: bool) -> Result> { if self.is_empty_after_trim() { @@ -353,32 +399,28 @@ impl AllowedSignerSplitter { } } - // If the next token doesn't start with a double quote, the token can represent an option. - // Only an option token can contain double quotes in the middle (e.g. a="b c"). - // If we don't see any double quote in the token, we greedily parse the token until the - // next whitespace. + // If the next token doesn't start with a double quote, the token can represent the options. + // Only the options token can contain double quotes in the middle (e.g. a="b c",d="e"). + // Otherwise, we greedily parse the token until the next whitespace. let mut s = String::new(); while let Some(last) = self.buffer.pop() { - if [" ", "\"", "#"].contains(&last.as_str()) { - self.buffer.push(last); - break; - } - - s.push_str(&last); - } - - // This should only apply to options - if let Some(last) = self.buffer.last() { - if last == "\"" { - s.push_str(self.split_quote()?.as_str()); - - // After the double quotes in the option token, there can only be nothing, a - // whitespace, or a pound - if let Some(last) = self.buffer.last() { - if ![" ", "#"].contains(&last.as_str()) { - return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); + match last.as_str() { + " " | "\t" | "#" => { + self.buffer.push(last); + break; + }, + "\"" => { + self.buffer.push(last); + s.push_str(self.split_quote()?.as_str()); + + // A closing double quote must end the token or the option + if let Some(last) = self.buffer.last() { + if ![" ", "\t", "#"].contains(&last.as_str()) && !last.starts_with(',') { + return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); + } } - } + }, + _ => s.push_str(&last), } } @@ -389,7 +431,7 @@ impl AllowedSignerSplitter { fn trim(&mut self) { while let Some(last) = self.buffer.last(){ match last.as_str() { - " " => { + " " | "\t" => { self.buffer.pop(); }, // Comment detected @@ -427,18 +469,47 @@ impl AllowedSignerSplitter { } } +/// Split the options field on commas that are not inside double quotes. +fn split_options(field: &str) -> Vec { + let mut options = Vec::new(); + let mut option = String::new(); + let mut in_quotes = false; + + for c in field.chars() { + match c { + '"' => { + in_quotes = !in_quotes; + option.push(c); + }, + ',' if !in_quotes => options.push(std::mem::take(&mut option)), + _ => option.push(c), + } + } + options.push(option); + + options +} + +/// Format a UNIX timestamp as YYYYMMDDHHMMSSZ. +/// OpenSSH rejects timestamps at or before the epoch and needs a four-digit year, so this clamps +/// the timestamp between 19700101000001Z and 99991231235959Z. +fn format_timestamp(timestamp: i64) -> String { + let timestamp = timestamp.clamp(1, 253402300799); + DateTime::from_timestamp(timestamp, 0) + .expect("clamped timestamp should be in range") + .format("%Y%m%d%H%M%SZ") + .to_string() +} + /// Parse a string into a u64 representing a timestamp. -/// The timestamp has format YYYYMMDD[HHMM[SS]][Z] +/// The timestamp has format YYYYMMDD[HHMM[SS]][Z|UTC], with a case-insensitive suffix. /// The timestamp can be enclosed by quotation marks. fn parse_timestamp(s: &str) -> Result { - let mut s = s.trim_matches('"'); - let is_utc = s.ends_with('Z'); - if s.len() % 2 == 1 && !is_utc { - return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidTimestamp)); - } - if is_utc { - s = s.trim_end_matches('Z'); - } + let s = s.trim_matches('"').to_ascii_lowercase(); + let (s, is_utc) = match s.strip_suffix('z').or_else(|| s.strip_suffix("utc")) { + Some(s) => (s, true), + None => (s.as_str(), false), + }; let datetime = match s.len() { 8 => { let date = NaiveDate::parse_from_str(s, "%Y%m%d") diff --git a/tests/allowed-signer.rs b/tests/allowed-signer.rs index 6a23ce2..1b685c2 100644 --- a/tests/allowed-signer.rs +++ b/tests/allowed-signer.rs @@ -46,7 +46,7 @@ fn parse_good_allowed_signer_with_quoted_principals() { #[test] fn parse_good_allowed_signer_with_options() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=\"thanh,mitchell\" valid-before=\"20240505Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-before=\"20240505Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -67,7 +67,7 @@ fn parse_good_allowed_signer_with_options() { #[test] fn parse_good_allowed_signer_with_utc_timestamp() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=\"thanh,mitchell\" valid-after=20240505Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=20240505Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -88,7 +88,7 @@ fn parse_good_allowed_signer_with_utc_timestamp() { #[test] fn parse_good_allowed_signer_with_hm_timestamp() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=\"thanh,mitchell\" valid-after=202405050102Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=202405050102Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -109,7 +109,7 @@ fn parse_good_allowed_signer_with_hm_timestamp() { #[test] fn parse_good_allowed_signer_with_hms_timestamp() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=\"thanh,mitchell\" valid-after=20240505010230Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=20240505010230Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -130,7 +130,7 @@ fn parse_good_allowed_signer_with_hms_timestamp() { #[test] fn parse_good_allowed_signer_with_consecutive_spaces() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=\"thanh,#mitchell\" valid-before=\"20240505Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 "; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,#mitchell\",valid-before=\"20240505Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 "; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -151,7 +151,7 @@ fn parse_good_allowed_signer_with_consecutive_spaces() { #[test] fn parse_good_allowed_signer_with_empty_namespaces() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=\"thanh,,mitchell\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,,mitchell\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -172,7 +172,7 @@ fn parse_good_allowed_signer_with_empty_namespaces() { #[test] fn parse_good_allowed_signer_with_space_in_namespaces() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=\"thanh,mitchell tech\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell tech\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -191,24 +191,26 @@ fn parse_good_allowed_signer_with_space_in_namespaces() { } #[test] -fn parse_good_allowed_signer_with_unquoted_namespaces() { +fn parse_bad_allowed_signer_with_unquoted_namespaces() { + // Options are comma-separated, so the parser reads an unquoted list as namespaces=thanh plus an option named mitchell let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces=thanh,mitchell ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io cert-authority,namespaces=thanh,mitchell ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); - assert!(allowed_signer.is_ok()); - let allowed_signer = allowed_signer.unwrap(); - assert_eq!(allowed_signer.key.fingerprint().to_string(), "SHA256:QAtqtvvCePelMMUNPP7madH2zNa1ATxX1nt9L/0C5+M"); - assert_eq!( - allowed_signer.principals, - vec!["mitchell@confurious.io".to_string(), "mitchel2@confurious.io".to_string()], - ); - assert!(allowed_signer.cert_authority); - assert_eq!( - allowed_signer.namespaces, - Some(vec!["thanh".to_string(), "mitchell".to_string()]) + assert!( + matches!( + allowed_signer, + Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(ref v))) if v == "mitchell", + ) ); - assert!(allowed_signer.valid_after.is_none()); - assert!(allowed_signer.valid_before.is_none()); +} + +#[test] +fn parse_bad_allowed_signer_with_space_separated_options() { + let allowed_signer = + "mitchell@confurious.io cert-authority namespaces=\"thanh\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + let allowed_signer = AllowedSigner::from_string(allowed_signer); + // Like OpenSSH, the options field ends at the space, so the parser reads namespaces=... as the key type + assert!(matches!(allowed_signer, Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidKey)))); } #[test] @@ -267,7 +269,7 @@ fn parse_bad_allowed_signer_with_timestamp_option() { #[test] fn parse_bad_allowed_signer_with_conflicting_timestamps() { let allowed_signer = - "mitchell@confurious.io valid-before=20240505 valid-after=20240505 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io valid-before=20240505,valid-after=20240505 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_err()); assert!(matches!(allowed_signer, Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidTimestamps)))); @@ -276,7 +278,7 @@ fn parse_bad_allowed_signer_with_conflicting_timestamps() { #[test] fn parse_bad_allowed_signer_with_duplicate_option() { let allowed_signer = - "mitchell@confurious.io namespaces=thanh namespaces=mitchell ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io namespaces=thanh,namespaces=mitchell ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_err()); assert!( @@ -350,3 +352,113 @@ fn parse_bad_allowed_signer_with_invalid_timestamp() { let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_err()); } + +#[test] +fn display_allowed_signer_round_trips() { + let allowed_signer = + "mitchell@confurious.io cert-authority,namespaces=\"git,file\",valid-after=20240101Z,valid-before=\"20240505123045Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + let allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + let formatted = allowed_signer.to_string(); + assert_eq!( + formatted, + "mitchell@confurious.io cert-authority,namespaces=\"git,file\",valid-after=\"20240101000000Z\",valid-before=\"20240505123045Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ); + assert_eq!(AllowedSigner::from_string(&formatted).unwrap(), allowed_signer); +} + +#[test] +fn display_allowed_signer_quotes_principals() { + for principals in ["\"mitchell confurious\"", "\"mitchell#confurious,thanh\""] { + let allowed_signer = format!("{} ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", principals); + let allowed_signer = AllowedSigner::from_string(&allowed_signer).unwrap(); + let formatted = allowed_signer.to_string(); + assert!(formatted.starts_with(principals)); + assert_eq!(AllowedSigner::from_string(&formatted).unwrap(), allowed_signer); + } +} + +#[test] +fn display_allowed_signer_clamps_timestamps() { + let allowed_signer = + "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + let mut allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + allowed_signer.valid_after = Some(i64::MIN); + allowed_signer.valid_before = Some(i64::MAX); + assert_eq!( + allowed_signer.to_string(), + "mitchell@confurious.io valid-after=\"19700101000001Z\",valid-before=\"99991231235959Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ); +} + +#[test] +fn display_allowed_signer_escapes_quotes_in_namespaces() { + let allowed_signer = + "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + let mut allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + allowed_signer.namespaces = Some(vec!["git".to_string(), "a\"b".to_string()]); + assert_eq!( + allowed_signer.to_string(), + "mitchell@confurious.io namespaces=\"git,a\\\"b\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ); +} + +#[test] +fn parse_good_allowed_signer_with_tabs() { + let allowed_signer = + "mitchell@confurious.io\tcert-authority,namespaces=\"git\"\tssh-ed25519\tAAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5\t"; + let allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + assert_eq!(allowed_signer.principals, vec!["mitchell@confurious.io".to_string()]); + assert!(allowed_signer.cert_authority); + assert_eq!(allowed_signer.namespaces, Some(vec!["git".to_string()])); +} + +#[test] +fn parse_bad_allowed_signer_with_stray_quotes() { + for allowed_signer in [ + "a\"b\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + "a\"b c\"d ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + "mitchell@confurious.io namespaces=\"\"a\"\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + "mitchell@confurious.io namespaces=\"a\"b ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ] { + let allowed_signer = AllowedSigner::from_string(allowed_signer); + assert!( + matches!(allowed_signer, Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes))), + "{:?}", allowed_signer, + ); + } +} + +#[test] +fn parse_bad_allowed_signer_reports_invalid_option() { + for (options, expected) in [ + ("namespace=\"git\"", "namespace"), + ("option=test", "option"), + ("cert-authority=\"x\"", "cert-authority"), + ("cert-authority,", "cert-authority,"), + ("namespaces=\"git\",,cert-authority", "namespaces=\"git\",,cert-authority"), + ] { + let allowed_signer = format!("mitchell@confurious.io {} ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", options); + let allowed_signer = AllowedSigner::from_string(&allowed_signer); + assert!( + matches!( + allowed_signer, + Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(ref v))) if v == expected, + ), + "{}: {:?}", options, allowed_signer, + ); + } +} + +#[test] +fn parse_good_allowed_signer_with_utc_suffix() { + for timestamp in ["20240505UTC", "20240505utc", "20240505z", "\"20240505000000UTC\""] { + let allowed_signer = format!("mitchell@confurious.io valid-after={} ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); + let allowed_signer = AllowedSigner::from_string(&allowed_signer).unwrap(); + assert_eq!(allowed_signer.valid_after, Some(1714867200i64), "{}", timestamp); + } + + for timestamp in ["20240505ZZ", "20240505UTCZ", "2024050UTC"] { + let allowed_signer = format!("mitchell@confurious.io valid-after={} ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); + assert!(AllowedSigner::from_string(&allowed_signer).is_err(), "{}", timestamp); + } +} diff --git a/tests/allowed_signers/good_allowed_signers b/tests/allowed_signers/good_allowed_signers index 14e9848..f65a03e 100644 --- a/tests/allowed_signers/good_allowed_signers +++ b/tests/allowed_signers/good_allowed_signers @@ -2,5 +2,5 @@ mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 # Empty lines are also ignored -mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces="thanh,#mitchell" valid-before="20240505Z" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 # End of line comment is ignored -mitchell@confurious.io,mitchel2@confurious.io cert-authority namespaces="thanh, ,mitchell mitchell, andrew andrew" valid-before="20240505Z" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 +mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces="thanh,#mitchell",valid-before="20240505Z" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 # End of line comment is ignored +mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces="thanh, ,mitchell mitchell, andrew andrew",valid-before="20240505Z" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 From c67581abedfd6b7b783938e7c8e1d4bac2a27d1f Mon Sep 17 00:00:00 2001 From: Duc Thanh Nguyen Date: Sun, 27 Sep 2026 18:55:17 -0400 Subject: [PATCH 2/2] fix(ssh): parse allowed signers the way OpenSSH does Port OpenSSH's allowed signers parser so that a line means the same thing here as it does to ssh-keygen, and make Display fail instead of writing a line that OpenSSH would read back differently. - Read \" in a quoted option value as a literal quote, so namespaces containing quotes round-trip. - Replace the splitter, whose quote check looked at the last token of the line instead of the next one. - Stop clamping timestamps. Display now fails for values OpenSSH can't read back, including pairs that clamping collapsed together. - Fail in Display for principals or namespaces that are empty or contain commas, quotes (principals), or line breaks. - Add test cases from OpenSSH's test_strdelim.c, test_convtime.c, and sshsig.sh. --- src/ssh/allowed_signer.rs | 450 +++++++++++++++++------------------- tests/allowed-signer-dst.rs | 4 +- tests/allowed-signer.rs | 239 ++++++++++++++++--- 3 files changed, 415 insertions(+), 278 deletions(-) diff --git a/src/ssh/allowed_signer.rs b/src/ssh/allowed_signer.rs index 554161e..5e5114e 100644 --- a/src/ssh/allowed_signer.rs +++ b/src/ssh/allowed_signer.rs @@ -4,7 +4,7 @@ use std::io::Read; use std::path::Path; use chrono::prelude::Local; -use chrono::{DateTime, Duration, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, TimeZone}; +use chrono::{Datelike, DateTime, Duration, LocalResult, NaiveDate, NaiveDateTime, NaiveTime, TimeZone}; use super::pubkey::PublicKey; use crate::{error::Error, Result}; @@ -97,14 +97,17 @@ impl AllowedSigner { /// println!("{:?}", allowed_signer); /// ``` pub fn from_string(s: &str) -> Result { - let mut tokenizer = AllowedSignerSplitter::new(s); - - let principals = tokenizer.next(true)? - .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingPrincipals))?; - let principals = principals.trim_matches('"'); - if principals.contains('"') { - return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); + // This follows OpenSSH's parse_principals_key_and_options() and sshsigopt_parse() so that + // a line means the same thing here as it does to ssh-keygen. + let line = s.trim_start_matches([' ', '\t', '\r', '\n']); + if line.is_empty() || line.starts_with('#') { + return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingPrincipals)); } + + // Format: identity[,identity...] [option[,option...]] key + let (principals, rest) = strdelimw(line) + .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes))?; + let principals: Vec = principals.split(',') .map(|s| s.to_string()) .collect(); @@ -112,105 +115,80 @@ impl AllowedSigner { return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidPrincipals)); } + // The options field is optional, so first try to read the key + let (options, key) = match read_key(rest) { + Some(key) => ("", key), + None => { + let end = advance_past_options(rest) + .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes))?; + let (options, key) = rest.split_at(end); + if key.is_empty() { + return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingKey)); + } + let key = read_key(key[1..].trim_start_matches([' ', '\t'])) + .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidKey))?; + (options, key) + }, + }; + let mut cert_authority = false; let mut namespaces = None; let mut valid_after = None; let mut valid_before = None; - let field = tokenizer.next(false)? - .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingKey))?; - - // The options field is optional. Key types never contain '=' or ',', and the only option - // without either is cert-authority. - let options = if field.contains(['=', ',']) || field.eq_ignore_ascii_case("cert-authority") { - split_options(&field) - } else { - Vec::new() - }; + let mut opts = options; + while !opts.is_empty() { + let option = opts; - for option in &options { - let (option_key, option_value) = match option.split_once('=') { - Some(v) => v, - None => (option.as_str(), ""), - }; - let option_value = option_value.trim_matches('"'); - if option_value.contains("\"") { - return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); - } - - match option_key.to_lowercase().as_str() { - "cert-authority" => { - if option.contains('=') { - return Err( - Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption("cert-authority".to_string())) - ); - } - cert_authority = true; - }, - "namespaces" => { - if namespaces.is_some() { - return Err( - Error::InvalidAllowedSigner(AllowedSignerParsingError::DuplicateOptions("namespaces".to_string())) - ); - } - - let namespaces_value: Vec<&str> = option_value.split(',') - .filter(|e| !e.is_empty()) - .collect(); - namespaces = Some( - namespaces_value.iter() - .map(|s| s.to_string()) - .collect() + if opt_flag(&mut opts, "cert-authority") { + cert_authority = true; + } else if opt_match(&mut opts, "namespaces") { + if namespaces.is_some() { + return Err( + Error::InvalidAllowedSigner(AllowedSignerParsingError::DuplicateOptions("namespaces".to_string())) ); - }, - "valid-after" => { - if valid_after.is_some() { - return Err( - Error::InvalidAllowedSigner(AllowedSignerParsingError::DuplicateOptions("valid-after".to_string())) - ); - } - valid_after = Some(parse_timestamp(option_value) - .map_err( - |_| Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption("valid-after".to_string())))? - ); - }, - "valid-before" => { - if valid_before.is_some() { - return Err( - Error::InvalidAllowedSigner(AllowedSignerParsingError::DuplicateOptions("valid-before".to_string())) - ); - } - valid_before = Some(parse_timestamp(option_value) - .map_err( - |_| Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption("valid-before".to_string())))? - ); - }, - // An empty option comes from a stray comma, so report the whole field - "" => { + } + namespaces = Some( + opt_dequote(&mut opts)? + .split(',') + .filter(|e| !e.is_empty()) + .map(|s| s.to_string()) + .collect() + ); + } else if opt_match(&mut opts, "valid-after") { + if valid_after.is_some() { return Err( - Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(field.clone())) + Error::InvalidAllowedSigner(AllowedSignerParsingError::DuplicateOptions("valid-after".to_string())) ); - }, - _ => { + } + valid_after = Some(opt_timestamp(&mut opts, "valid-after")?); + } else if opt_match(&mut opts, "valid-before") { + if valid_before.is_some() { return Err( - Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(option_key.to_string())) + Error::InvalidAllowedSigner(AllowedSignerParsingError::DuplicateOptions("valid-before".to_string())) ); - }, - }; - } - - let kt = if options.is_empty() { - field - } else { - tokenizer.next(false)? - .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingKey))? - }; - - let key_data = tokenizer.next(false)? - .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidKey))?; + } + valid_before = Some(opt_timestamp(&mut opts, "valid-before")?); + } - let key = PublicKey::from_string(format!("{} {}", kt, key_data).as_str()) - .map_err(|_| Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidKey))?; + if opts.is_empty() { + break; + } + // Anything other than a comma means an unknown option. Like OpenSSH, skip an empty + // option between two commas. + if !opts.starts_with(',') { + let name = option.split(['=', ',']).next().unwrap_or(option); + return Err( + Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(name.to_string())) + ); + } + opts = &opts[1..]; + if opts.is_empty() { + return Err( + Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(options.to_string())) + ); + } + } // Timestamp sanity check if let (Some(valid_before), Some(valid_after)) = (&valid_before, &valid_after) { @@ -221,13 +199,6 @@ impl AllowedSigner { } } - // After key data, there must be only comment or nothing - if !tokenizer.is_empty_after_trim() { - return Err( - Error::InvalidAllowedSigner(AllowedSignerParsingError::UnexpectedEnd), - ); - } - Ok(AllowedSigner{ principals, cert_authority, @@ -240,9 +211,17 @@ impl AllowedSigner { } impl fmt::Display for AllowedSigner { + /// Fails if OpenSSH wouldn't read a field back as the same value. fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + // OpenSSH doesn't escape commas in lists. A line break would start a new entry. + let is_list_item = |s: &String| !s.is_empty() && !s.contains([',', '\r', '\n']); + let mut output = String::new(); + // A quoted principals field ends at the next double quote + if self.principals.is_empty() || !self.principals.iter().all(|p| is_list_item(p) && !p.contains('"')) { + return Err(fmt::Error); + } let principals = self.principals.join(","); if principals.contains([' ', '\t', '#']) { output.push_str(&format!("\"{}\"", principals)); @@ -258,15 +237,25 @@ impl fmt::Display for AllowedSigner { } if let Some(ref namespaces) = self.namespaces { + // A trailing backslash would escape the closing quote + if !namespaces.iter().all(is_list_item) || namespaces.last().is_some_and(|n| n.ends_with('\\')) { + return Err(fmt::Error); + } options.push(format!("namespaces=\"{}\"", namespaces.join(",").replace('"', "\\\""))); } + if let (Some(valid_before), Some(valid_after)) = (self.valid_before, self.valid_after) { + if valid_before <= valid_after { + return Err(fmt::Error); + } + } + if let Some(valid_after) = self.valid_after { - options.push(format!("valid-after=\"{}\"", format_timestamp(valid_after))); + options.push(format!("valid-after=\"{}\"", format_timestamp(valid_after).ok_or(fmt::Error)?)); } if let Some(valid_before) = self.valid_before { - options.push(format!("valid-before=\"{}\"", format_timestamp(valid_before))); + options.push(format!("valid-before=\"{}\"", format_timestamp(valid_before).ok_or(fmt::Error)?)); } if !options.is_empty() { @@ -339,173 +328,120 @@ impl AllowedSigners { } } -/// A type used to split the allowed signer segments, abstracting out the handling of double quotes. -/// The splitter is highly aware of the allowed_signer format and will catch certain invalid -/// formats. -/// -/// For example: "principals option1=\"value1 value2\",option2,option3=value kt key_data" is split into -/// ["principals", "option1=\"value1 value2\",option2,option3=value", "kt", "key_data"] -struct AllowedSignerSplitter { - /// A buffer of remaining tokens in reverse order. - buffer: Vec, -} - -impl AllowedSignerSplitter { - /// Split the string by delimiters but keep the delimiters. - fn new(s: &str) -> Self { - let mut buffer = Vec::new(); - let mut last = 0; - - for (index, matched) in s.match_indices([' ', '\t', '"', '#']) { - // Push the new text before the delimiter - if last != index { - buffer.push(s[last..index].to_owned()); - } - // Push the delimiter - buffer.push(matched.to_owned()); - last = index + matched.len(); - } - - // Push the remaining text - if last < s.len() { - buffer.push(s[last..].to_owned()); - } +/// Port of OpenSSH's strdelimw(). Split off the first token and skip the whitespace after it. The +/// token ends at whitespace or at the closing quote of a quoted section. Returns None if a quote is +/// left open. +fn strdelimw(s: &str) -> Option<(String, &str)> { + const WHITESPACE: [char; 4] = [' ', '\t', '\r', '\n']; - // We parse from left to right so reversing allow us to use Vec's last() and pop() - buffer.reverse(); + let Some(index) = s.find(|c| WHITESPACE.contains(&c) || c == '"') else { + return Some((s.to_string(), "")); + }; - Self { buffer } + if s[index..].starts_with('"') { + let (quoted, rest) = s[index + 1..].split_once('"')?; + return Some((format!("{}{}", &s[..index], quoted), rest.trim_start_matches(WHITESPACE))); } - fn is_empty_after_trim(&mut self) -> bool { - self.trim(); - return self.buffer.is_empty(); - } + Some((s[..index].to_string(), s[index + 1..].trim_start_matches(WHITESPACE))) +} - /// Get the next whitespace-separated field (principals, options, key type, key data) - /// If opening_quotes_allowed is set to false, we reject the next token if it starts with ". - fn next(&mut self, opening_quotes_allowed: bool) -> Result> { - if self.is_empty_after_trim() { - return Ok(None); - } +/// Port of OpenSSH's sshkey_read(). Ignores anything after the key data, which is a comment. +fn read_key(s: &str) -> Option { + let mut fields = s.split([' ', '\t']).filter(|f| !f.is_empty()); + let key = format!("{} {}", fields.next()?, fields.next()?); + PublicKey::from_string(&key).ok() +} - // If the next token starts with a double quote, then the closing double quote is also - // the end of the token - if self.buffer[0] == "\"" { - if opening_quotes_allowed { - return self.split_quote().map(|v| Some(v)); - } else { - return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); - } +/// Port of OpenSSH's sshkey_advance_past_options(). Returns the length of the options field, which +/// ends at the first whitespace outside double quotes, or None if a quote is left open. +fn advance_past_options(s: &str) -> Option { + let bytes = s.as_bytes(); + let mut quoted = false; + let mut index = 0; + + while index < bytes.len() && (quoted || (bytes[index] != b' ' && bytes[index] != b'\t')) { + if bytes[index] == b'\\' && bytes.get(index + 1) == Some(&b'"') { + index += 1; + } else if bytes[index] == b'"' { + quoted = !quoted; } + index += 1; + } - // If the next token doesn't start with a double quote, the token can represent the options. - // Only the options token can contain double quotes in the middle (e.g. a="b c",d="e"). - // Otherwise, we greedily parse the token until the next whitespace. - let mut s = String::new(); - while let Some(last) = self.buffer.pop() { - match last.as_str() { - " " | "\t" | "#" => { - self.buffer.push(last); - break; - }, - "\"" => { - self.buffer.push(last); - s.push_str(self.split_quote()?.as_str()); - - // A closing double quote must end the token or the option - if let Some(last) = self.buffer.last() { - if ![" ", "\t", "#"].contains(&last.as_str()) && !last.starts_with(',') { - return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); - } - } - }, - _ => s.push_str(&last), - } - } + // A quote left open runs to the end of the string + (!quoted).then_some(index) +} - Ok(Some(s)) +/// Port of OpenSSH's opt_flag(). Consume a case-insensitive flag option. +fn opt_flag(opts: &mut &str, flag: &str) -> bool { + match opts.get(..flag.len()) { + Some(prefix) if prefix.eq_ignore_ascii_case(flag) => { + *opts = &opts[flag.len()..]; + true + }, + _ => false, } +} - /// Trim comment and whitespaces - fn trim(&mut self) { - while let Some(last) = self.buffer.last(){ - match last.as_str() { - " " | "\t" => { - self.buffer.pop(); - }, - // Comment detected - "#" => { - self.buffer.clear() - }, - _ => break, - }; - } +/// Port of OpenSSH's opt_match(). Consume a case-insensitive option name and the '=' after it. +fn opt_match(opts: &mut &str, name: &str) -> bool { + match opts.get(..name.len() + 1) { + Some(prefix) if prefix[..name.len()].eq_ignore_ascii_case(name) && prefix.ends_with('=') => { + *opts = &opts[name.len() + 1..]; + true + }, + _ => false, } +} - /// Extract content inside the double quotes. - /// This function assumes buffer starst with a ". - fn split_quote(&mut self) -> Result { - match self.buffer.pop() { - Some(v) => { - if v != "\"" { - return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)); - } +/// Port of OpenSSH's opt_dequote(). Consume a double-quoted value, in which \" is a literal quote. +fn opt_dequote(opts: &mut &str) -> Result { + let mut chars = opts.strip_prefix('"') + .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes))? + .chars(); + + let mut value = String::new(); + loop { + match chars.next() { + Some('"') => break, + Some('\\') if chars.as_str().starts_with('"') => { + chars.next(); + value.push('"'); }, + Some(c) => value.push(c), None => return Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)), } + } - let mut s = String::from("\""); - loop { - let token = self.buffer.pop() - .ok_or(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes))?; - s.push_str(&token); - if token == "\"" { - break; - } - } + *opts = chars.as_str(); + Ok(value) +} - Ok(s) +/// Consume a quoted valid-after or valid-before value. Like OpenSSH, reject the epoch itself. +fn opt_timestamp(opts: &mut &str, name: &str) -> Result { + match parse_timestamp(&opt_dequote(opts)?) { + Ok(timestamp) if timestamp != 0 => Ok(timestamp), + _ => Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(name.to_string()))), } } -/// Split the options field on commas that are not inside double quotes. -fn split_options(field: &str) -> Vec { - let mut options = Vec::new(); - let mut option = String::new(); - let mut in_quotes = false; - - for c in field.chars() { - match c { - '"' => { - in_quotes = !in_quotes; - option.push(c); - }, - ',' if !in_quotes => options.push(std::mem::take(&mut option)), - _ => option.push(c), - } +/// Format a UNIX timestamp as YYYYMMDDHHMMSSZ. Returns None if OpenSSH can't read the timestamp +/// back, since it rejects the epoch itself and needs a four-digit year. +fn format_timestamp(timestamp: i64) -> Option { + if timestamp == 0 { + return None; } - options.push(option); - options -} - -/// Format a UNIX timestamp as YYYYMMDDHHMMSSZ. -/// OpenSSH rejects timestamps at or before the epoch and needs a four-digit year, so this clamps -/// the timestamp between 19700101000001Z and 99991231235959Z. -fn format_timestamp(timestamp: i64) -> String { - let timestamp = timestamp.clamp(1, 253402300799); DateTime::from_timestamp(timestamp, 0) - .expect("clamped timestamp should be in range") - .format("%Y%m%d%H%M%SZ") - .to_string() + .filter(|datetime| (0..=9999).contains(&datetime.year())) + .map(|datetime| datetime.format("%Y%m%d%H%M%SZ").to_string()) } /// Parse a string into a u64 representing a timestamp. /// The timestamp has format YYYYMMDD[HHMM[SS]][Z|UTC], with a case-insensitive suffix. -/// The timestamp can be enclosed by quotation marks. fn parse_timestamp(s: &str) -> Result { - let s = s.trim_matches('"').to_ascii_lowercase(); + let s = s.to_ascii_lowercase(); let (s, is_utc) = match s.strip_suffix('z').or_else(|| s.strip_suffix("utc")) { Some(s) => (s, true), None => (s.as_str(), false), @@ -545,3 +481,33 @@ fn parse_timestamp(s: &str) -> Result { Ok(timestamp) } + +#[cfg(test)] +mod tests { + use super::*; + + // Cases from OpenSSH's regress/unittests/misc/test_strdelim.c. strdelimw() doesn't split on + // '=', so this leaves out the strdelim() cases that do. + #[test] + fn strdelimw_matches_openssh() { + for (s, expected) in [ + ("", Some(("", ""))), + ("\t", Some(("", ""))), + ("blob", Some(("blob", ""))), + ("blob ", Some(("blob", ""))), + ("blob1 blob2", Some(("blob1", "blob2"))), + ("blob1\t \tblob2 \t \t", Some(("blob1", "blob2 \t \t"))), + ("blob1=blob2", Some(("blob1=blob2", ""))), + ("\"blob\"", Some(("blob", ""))), + ("\"blob1\" blob2", Some(("blob1", "blob2"))), + ("blob1 \"blob2\"", Some(("blob1", "\"blob2\""))), + ("\"blob2\"", Some(("blob2", ""))), + ("\"blob2\" blob3", Some(("blob2", "blob3"))), + ("\"blob", None), + ("\"blob\\\"", Some(("blob\\", ""))), + ] { + let actual = strdelimw(s); + assert_eq!(actual.as_ref().map(|(token, rest)| (token.as_str(), *rest)), expected, "{:?}", s); + } + } +} diff --git a/tests/allowed-signer-dst.rs b/tests/allowed-signer-dst.rs index ea463e2..2976d21 100644 --- a/tests/allowed-signer-dst.rs +++ b/tests/allowed-signer-dst.rs @@ -7,10 +7,10 @@ fn parse_allowed_signer_dst_local_times() { let key = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; // 01:30 happens twice on 2024-11-03. Expect the earlier one, in EDT. - let allowed_signer = AllowedSigner::from_string(&format!("mitchell@confurious.io valid-after=202411030130 {key}")); + let allowed_signer = AllowedSigner::from_string(&format!("mitchell@confurious.io valid-after=\"202411030130\" {key}")); assert_eq!(allowed_signer.unwrap().valid_after, Some(1730611800)); // 02:30 does not exist on 2024-03-10. Like mktime, expect the EST offset from before the gap. - let allowed_signer = AllowedSigner::from_string(&format!("mitchell@confurious.io valid-after=202403100230 {key}")); + let allowed_signer = AllowedSigner::from_string(&format!("mitchell@confurious.io valid-after=\"202403100230\" {key}")); assert_eq!(allowed_signer.unwrap().valid_after, Some(1710055800)); } diff --git a/tests/allowed-signer.rs b/tests/allowed-signer.rs index 1b685c2..ba824b3 100644 --- a/tests/allowed-signer.rs +++ b/tests/allowed-signer.rs @@ -1,3 +1,5 @@ +use std::fmt::Write; + use sshcerts::error::Error; use sshcerts::ssh::{AllowedSigner, AllowedSignerParsingError}; @@ -67,7 +69,7 @@ fn parse_good_allowed_signer_with_options() { #[test] fn parse_good_allowed_signer_with_utc_timestamp() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=20240505Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=\"20240505Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -88,7 +90,7 @@ fn parse_good_allowed_signer_with_utc_timestamp() { #[test] fn parse_good_allowed_signer_with_hm_timestamp() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=202405050102Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=\"202405050102Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -109,7 +111,7 @@ fn parse_good_allowed_signer_with_hm_timestamp() { #[test] fn parse_good_allowed_signer_with_hms_timestamp() { let allowed_signer = - "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=20240505010230Z ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io,mitchel2@confurious.io cert-authority,namespaces=\"thanh,mitchell\",valid-after=\"20240505010230Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_ok()); let allowed_signer = allowed_signer.unwrap(); @@ -192,16 +194,11 @@ fn parse_good_allowed_signer_with_space_in_namespaces() { #[test] fn parse_bad_allowed_signer_with_unquoted_namespaces() { - // Options are comma-separated, so the parser reads an unquoted list as namespaces=thanh plus an option named mitchell + // Like OpenSSH, option values must be quoted let allowed_signer = "mitchell@confurious.io cert-authority,namespaces=thanh,mitchell ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); - assert!( - matches!( - allowed_signer, - Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(ref v))) if v == "mitchell", - ) - ); + assert!(matches!(allowed_signer, Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)))); } #[test] @@ -269,7 +266,7 @@ fn parse_bad_allowed_signer_with_timestamp_option() { #[test] fn parse_bad_allowed_signer_with_conflicting_timestamps() { let allowed_signer = - "mitchell@confurious.io valid-before=20240505,valid-after=20240505 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io valid-before=\"20240505\",valid-after=\"20240505\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_err()); assert!(matches!(allowed_signer, Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidTimestamps)))); @@ -278,7 +275,7 @@ fn parse_bad_allowed_signer_with_conflicting_timestamps() { #[test] fn parse_bad_allowed_signer_with_duplicate_option() { let allowed_signer = - "mitchell@confurious.io namespaces=thanh,namespaces=mitchell ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io namespaces=\"thanh\",namespaces=\"mitchell\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_err()); assert!( @@ -306,10 +303,11 @@ fn parse_bad_allowed_signer_with_quoted_key() { "mitchell@confurious.io \"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5\""; let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!(allowed_signer.is_err()); + // Like OpenSSH, the parser reads the quoted string as an options field with no key after it assert!( matches!( allowed_signer, - Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)), + Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::MissingKey)), ) ); @@ -320,7 +318,7 @@ fn parse_bad_allowed_signer_with_quoted_key() { assert!( matches!( allowed_signer, - Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidQuotes)), + Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidKey)), ) ); } @@ -356,7 +354,7 @@ fn parse_bad_allowed_signer_with_invalid_timestamp() { #[test] fn display_allowed_signer_round_trips() { let allowed_signer = - "mitchell@confurious.io cert-authority,namespaces=\"git,file\",valid-after=20240101Z,valid-before=\"20240505123045Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + "mitchell@confurious.io cert-authority,namespaces=\"git,file\",valid-after=\"20240101Z\",valid-before=\"20240505123045Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); let formatted = allowed_signer.to_string(); assert_eq!( @@ -378,16 +376,37 @@ fn display_allowed_signer_quotes_principals() { } #[test] -fn display_allowed_signer_clamps_timestamps() { +fn display_allowed_signer_keeps_timestamps_before_epoch() { let allowed_signer = - "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; - let mut allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); - allowed_signer.valid_after = Some(i64::MIN); - allowed_signer.valid_before = Some(i64::MAX); + "mitchell@confurious.io valid-after=\"19691230Z\",valid-before=\"19691231Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + let allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + let formatted = allowed_signer.to_string(); assert_eq!( - allowed_signer.to_string(), - "mitchell@confurious.io valid-after=\"19700101000001Z\",valid-before=\"99991231235959Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + formatted, + "mitchell@confurious.io valid-after=\"19691230000000Z\",valid-before=\"19691231000000Z\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", ); + assert_eq!(AllowedSigner::from_string(&formatted).unwrap(), allowed_signer); +} + +#[test] +fn display_allowed_signer_rejects_unwritable_timestamps() { + let allowed_signer = + "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + // OpenSSH rejects the epoch itself and needs a four-digit year + for (valid_after, valid_before) in [ + (Some(0), None), + (None, Some(0)), + (Some(i64::MIN), None), + (None, Some(i64::MAX)), + (None, Some(253402300800)), + (Some(1714867200), Some(1714867200)), + (Some(1714867201), Some(1714867200)), + ] { + let mut allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + allowed_signer.valid_after = valid_after; + allowed_signer.valid_before = valid_before; + assert!(write!(String::new(), "{}", allowed_signer).is_err(), "{:?} {:?}", valid_after, valid_before); + } } #[test] @@ -395,11 +414,35 @@ fn display_allowed_signer_escapes_quotes_in_namespaces() { let allowed_signer = "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; let mut allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); - allowed_signer.namespaces = Some(vec!["git".to_string(), "a\"b".to_string()]); + allowed_signer.namespaces = Some(vec!["git".to_string(), "a\"b".to_string(), "c\\\"d".to_string(), "e f".to_string()]); + let formatted = allowed_signer.to_string(); assert_eq!( - allowed_signer.to_string(), - "mitchell@confurious.io namespaces=\"git,a\\\"b\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + formatted, + "mitchell@confurious.io namespaces=\"git,a\\\"b,c\\\\\"d,e f\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", ); + assert_eq!(AllowedSigner::from_string(&formatted).unwrap(), allowed_signer); +} + +#[test] +fn display_allowed_signer_rejects_unwritable_principals() { + let allowed_signer = + "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + for principals in [vec![], vec![""], vec!["a,b"], vec!["a\"b"], vec!["a\nb"], vec!["a\rb"]] { + let mut allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + allowed_signer.principals = principals.iter().map(|p| p.to_string()).collect(); + assert!(write!(String::new(), "{}", allowed_signer).is_err(), "{:?}", principals); + } +} + +#[test] +fn display_allowed_signer_rejects_unwritable_namespaces() { + let allowed_signer = + "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + for namespaces in [vec![""], vec!["a,b"], vec!["a\nb"], vec!["a\rb"], vec!["a\\"], vec!["git", "a\\"]] { + let mut allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + allowed_signer.namespaces = Some(namespaces.iter().map(|n| n.to_string()).collect()); + assert!(write!(String::new(), "{}", allowed_signer).is_err(), "{:?}", namespaces); + } } #[test] @@ -413,12 +456,32 @@ fn parse_good_allowed_signer_with_tabs() { } #[test] -fn parse_bad_allowed_signer_with_stray_quotes() { +fn parse_allowed_signer_with_stray_quotes() { + // Like OpenSSH's strdelimw(), a quote in the principals field starts a quoted section that + // ends the field at its closing quote + let allowed_signer = + "a\"b\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + let allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + assert_eq!(allowed_signer.principals, vec!["ab".to_string()]); + + for (allowed_signer, expected) in [ + ("a\"b c\"d ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", "d"), + ("mitchell@confurious.io namespaces=\"\"a\"\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", "namespaces"), + ("mitchell@confurious.io namespaces=\"a\"b ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", "namespaces"), + ] { + let allowed_signer = AllowedSigner::from_string(allowed_signer); + assert!( + matches!( + allowed_signer, + Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(ref v))) if v == expected, + ), + "{:?}", allowed_signer, + ); + } + for allowed_signer in [ - "a\"b\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", - "a\"b c\"d ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", - "mitchell@confurious.io namespaces=\"\"a\"\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", - "mitchell@confurious.io namespaces=\"a\"b ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + "\"a b ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + "mitchell@confurious.io namespaces=\"a ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", ] { let allowed_signer = AllowedSigner::from_string(allowed_signer); assert!( @@ -428,6 +491,29 @@ fn parse_bad_allowed_signer_with_stray_quotes() { } } +#[test] +fn parse_good_allowed_signer_with_quotes_in_comment() { + for allowed_signer in [ + "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 # \"quoted comment\"", + "\"mitchell confurious\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 #\"", + // Like OpenSSH, anything after the key data is a comment + "mitchell@confurious.io ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5 comment", + ] { + let allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + assert_eq!(allowed_signer.key.fingerprint().to_string(), "SHA256:QAtqtvvCePelMMUNPP7madH2zNa1ATxX1nt9L/0C5+M"); + } +} + +#[test] +fn parse_good_allowed_signer_with_empty_options() { + // OpenSSH's sshsigopt_parse() skips an empty option between two commas + let allowed_signer = + "mitchell@confurious.io ,namespaces=\"git\",,cert-authority ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5"; + let allowed_signer = AllowedSigner::from_string(allowed_signer).unwrap(); + assert!(allowed_signer.cert_authority); + assert_eq!(allowed_signer.namespaces, Some(vec!["git".to_string()])); +} + #[test] fn parse_bad_allowed_signer_reports_invalid_option() { for (options, expected) in [ @@ -435,7 +521,10 @@ fn parse_bad_allowed_signer_reports_invalid_option() { ("option=test", "option"), ("cert-authority=\"x\"", "cert-authority"), ("cert-authority,", "cert-authority,"), - ("namespaces=\"git\",,cert-authority", "namespaces=\"git\",,cert-authority"), + ("cert-authorityx", "cert-authorityx"), + ("valid-after=\"0\"", "valid-after"), + ("valid-after=\"19700101Z\"", "valid-after"), + ("valid-after=\"\\\"20240505Z\\\"\"", "valid-after"), ] { let allowed_signer = format!("mitchell@confurious.io {} ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", options); let allowed_signer = AllowedSigner::from_string(&allowed_signer); @@ -451,14 +540,96 @@ fn parse_bad_allowed_signer_reports_invalid_option() { #[test] fn parse_good_allowed_signer_with_utc_suffix() { - for timestamp in ["20240505UTC", "20240505utc", "20240505z", "\"20240505000000UTC\""] { - let allowed_signer = format!("mitchell@confurious.io valid-after={} ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); + for timestamp in ["20240505UTC", "20240505utc", "20240505z", "20240505000000UTC"] { + let allowed_signer = format!("mitchell@confurious.io valid-after=\"{}\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); let allowed_signer = AllowedSigner::from_string(&allowed_signer).unwrap(); assert_eq!(allowed_signer.valid_after, Some(1714867200i64), "{}", timestamp); } for timestamp in ["20240505ZZ", "20240505UTCZ", "2024050UTC"] { - let allowed_signer = format!("mitchell@confurious.io valid-after={} ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); + let allowed_signer = format!("mitchell@confurious.io valid-after=\"{}\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); assert!(AllowedSigner::from_string(&allowed_signer).is_err(), "{}", timestamp); } } + +// Cases from OpenSSH's regress/unittests/misc/test_convtime.c +#[test] +fn parse_allowed_signer_timestamps_like_openssh() { + for timestamp in ["20000101", "200001011223", "20000101122345"] { + let allowed_signer = format!("mitchell@confurious.io valid-after=\"{}\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); + assert!(AllowedSigner::from_string(&allowed_signer).is_ok(), "{}", timestamp); + } + + for (timestamp, expected) in [ + ("20000101Z", 946684800), + ("200001011223Z", 946729380), + ("20000101122345Z", 946729425), + ("20000101UTC", 946684800), + ("200001011223UTC", 946729380), + ("20000101122345UTC", 946729425), + ] { + let allowed_signer = format!("mitchell@confurious.io valid-after=\"{}\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); + let allowed_signer = AllowedSigner::from_string(&allowed_signer).unwrap(); + assert_eq!(allowed_signer.valid_after, Some(expected), "{}", timestamp); + } + + for timestamp in [ + // Bad month + "20001301", "20000001", + // Incomplete + "2", "2000", "20000", "200001", "2000010", "200001010", + // Bad day, hour, minute, second + "20000199", "200001019900", "200001010099", "20000101000099", + // Invalid TZ specifier + "20000101ZZ", "20000101PDT", "20000101U", "20000101UTCUTC", + ] { + let allowed_signer = format!("mitchell@confurious.io valid-after=\"{}\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", timestamp); + assert!( + matches!( + AllowedSigner::from_string(&allowed_signer), + Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(ref v))) if v == "valid-after", + ), + "{}", timestamp, + ); + } +} + +// Cases from the allowed signers lines in OpenSSH's regress/sshsig.sh +#[test] +fn parse_allowed_signer_lines_from_openssh() { + let allowed_signer = AllowedSigner::from_string( + "user@example.com namespaces=\"test,whatever\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ).unwrap(); + assert_eq!(allowed_signer.namespaces, Some(vec!["test".to_string(), "whatever".to_string()])); + + let allowed_signer = AllowedSigner::from_string( + "user@example.com valid-after=\"19800101\",valid-before=\"19900101\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ).unwrap(); + assert!(allowed_signer.valid_after.is_some()); + assert!(allowed_signer.valid_before.is_some()); + + let allowed_signer = AllowedSigner::from_string( + "user@example.com cert-authority,valid-after=\"19800101\",valid-before=\"19900101\" ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ).unwrap(); + assert!(allowed_signer.cert_authority); + assert!(allowed_signer.valid_after.is_some()); + assert!(allowed_signer.valid_before.is_some()); + + for principals in ["user@example.com", "*@example.com"] { + let allowed_signer = format!("{} cert-authority ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", principals); + let allowed_signer = AllowedSigner::from_string(&allowed_signer).unwrap(); + assert_eq!(allowed_signer.principals, vec![principals.to_string()]); + assert!(allowed_signer.cert_authority); + } + + // Invalid option + let allowed_signer = AllowedSigner::from_string( + "user@example.com octopus ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIDO0VQD9TIdICZLWFWwtf7s8/aENve8twGTEmNV0myh5", + ); + assert!( + matches!( + allowed_signer, + Err(Error::InvalidAllowedSigner(AllowedSignerParsingError::InvalidOption(ref v))) if v == "octopus", + ) + ); +}