feat: validate a member's timestamps and window before every write
commit
8aad2f0feat: validate a member's timestamps and window before every write
The CLI checked a --valid-after/--valid-before flag was a well-formed OpenSSH timestamp before syncing, but nothing checked the two bounds were not inverted, and the check lived only in the CLI, so any other future caller of members::store (a web admin action, say) would silently accept an inverted window that could never authorize a push. Member::validate now checks both the timestamp shape and the ordering, called from members::store itself so it holds regardless of the caller; the CLI keeps only its fail-fast format check ahead of the network sync, now delegating to the shared members::valid_timestamp instead of duplicating the rule.
feat: add Member::validate (timestamp shape plus valid-after/valid-before ordering) refactor: call Member::validate from members::store refactor: have the CLI’s validate_timestamp reuse members::valid_timestamp Assisted-by: Claude:claude-sonnet-4-6
Reviews
No reviews of this commit yet — record a verdict below.
Start a review
crates/git-ents/src/main.rs
@@ -726,14 +726,12 @@
.map_or(0, |elapsed| elapsed.as_secs())
}
-/// Check that `value` is an OpenSSH `allowed_signers` timestamp: `YYYYMMDD`,
-/// `YYYYMMDDHHMM`, or `YYYYMMDDHHMMSS`, each optionally suffixed `Z` for UTC.
-/// Without `Z` the verifying server reads it in its own local time zone.
+/// Fail-fast check, ahead of any network sync, that `value` is a well-formed
+/// OpenSSH `allowed_signers` timestamp — the same rule [`Member::validate`]
+/// (via [`members::store`]) checks again before the write actually lands, and
+/// which also checks the two bounds are not inverted.
fn validate_timestamp(value: &str) -> Result<(), String> {
- let digits = value.strip_suffix('Z').unwrap_or(value);
- let well_formed =
- matches!(digits.len(), 8 | 12 | 14) && digits.bytes().all(|b| b.is_ascii_digit());
- if well_formed {
+ if members::valid_timestamp(value) {
Ok(())
} else {
Err(format!(
crates/git-ents/src/members.rs
@@ -196,6 +196,56 @@
Trust::Keys(_) | Trust::WebAuthn(_) => None,
}
}
+
+ /// Check invariants the type system does not enforce: a set validity
+ /// bound must be a well-formed OpenSSH timestamp, and when both bounds
+ /// are set, `valid_after` must not be after `valid_before` — an inverted
+ /// window would authorize nothing, silently locking every one of the
+ /// member's keys out rather than the admin's intended restriction.
+ /// [`store`] checks this before every write, so it holds regardless of
+ /// which caller builds the member (the CLI today, an admin web action
+ /// later).
+ pub fn validate(&self) -> Result<(), String> {
+ for bound in [&self.valid_after, &self.valid_before] {
+ if let Some(value) = bound
+ && !valid_timestamp(value)
+ {
+ return Err(format!(
+ "{value:?} is not a valid OpenSSH timestamp \
+ (expected YYYYMMDD[Z] or YYYYMMDDHHMM[SS][Z])"
+ ));
+ }
+ }
+ if let (Some(after), Some(before)) = (&self.valid_after, &self.valid_before)
+ && timestamp_key(after) > timestamp_key(before)
+ {
+ return Err(format!(
+ "valid-after {after:?} is after valid-before {before:?}: \
+ this window would never authorize a push"
+ ));
+ }
+ Ok(())
+ }
+}
+
+/// Whether `value` is a well-formed OpenSSH `allowed_signers` timestamp:
+/// `YYYYMMDD`, `YYYYMMDDHHMM`, or `YYYYMMDDHHMMSS`, each optionally suffixed
+/// `Z` for UTC. Without `Z` the verifying server reads it in its own local
+/// time zone.
+#[must_use]
+pub fn valid_timestamp(value: &str) -> bool {
+ let digits = value.strip_suffix('Z').unwrap_or(value);
+ matches!(digits.len(), 8 | 12 | 14) && digits.bytes().all(|b| b.is_ascii_digit())
+}
+
+/// `value`'s digits, right-padded to 14 (`YYYYMMDDHHMMSS`), so two timestamps
+/// of different precision compare correctly as calendar time. Ignores any `Z`
+/// suffix — comparing a UTC bound against a local one is inherently
+/// approximate; exact time-zone arithmetic is out of scope for this ordering
+/// check.
+fn timestamp_key(value: &str) -> String {
+ let digits = value.strip_suffix('Z').unwrap_or(value);
+ format!("{digits:0<14}")
}
/// Load the member named `username` from an already-open `store`.
@@ -248,8 +298,10 @@
}
/// Write `member` to its `refs/meta/member/<principal>` ref in `repo`,
-/// replacing any prior value, as a new commit.
+/// replacing any prior value, as a new commit. Rejects a member whose
+/// validity window is malformed or inverted — see [`Member::validate`].
pub fn store(repo: &Path, member: &Member) -> Result<(), git_store::Error> {
+ member.validate().map_err(git_store::Error::Invalid)?;
git_store::Store::open(repo)?.store_keyed(MEMBER_NS, member, "Update member")
}
@@ -555,4 +607,39 @@
format!("* cert-authority,valid-before=\"20270101\",namespaces=\"git\" {KEY_A}\n")
);
}
+
+ #[test]
+ fn validate_rejects_a_malformed_timestamp() {
+ let mut member = Member::with_keys("alice".to_owned(), keys(&[("aa:bb", KEY_A)]));
+ member.valid_before = Some("not-a-timestamp".to_owned());
+ assert!(member.validate().is_err());
+ }
+
+ #[test]
+ fn validate_rejects_an_inverted_window() {
+ let mut member = Member::with_keys("alice".to_owned(), keys(&[("aa:bb", KEY_A)]));
+ member.valid_after = Some("20270101".to_owned());
+ member.valid_before = Some("20260101".to_owned());
+ assert!(member.validate().is_err());
+ }
+
+ #[test]
+ fn validate_accepts_an_ordered_window_of_mixed_precision() {
+ let mut member = Member::with_keys("alice".to_owned(), keys(&[("aa:bb", KEY_A)]));
+ member.valid_after = Some("20260101".to_owned());
+ member.valid_before = Some("20270101120000Z".to_owned());
+ member.validate().unwrap();
+ }
+
+ #[test]
+ fn store_rejects_a_member_with_an_inverted_window() {
+ let repo = unique_repo();
+ let mut member = Member::with_keys("alice".to_owned(), keys(&[("aa:bb", KEY_A)]));
+ member.valid_after = Some("20270101".to_owned());
+ member.valid_before = Some("20260101".to_owned());
+ let result = store(&repo, &member);
+ assert!(matches!(result, Err(git_store::Error::Invalid(_))));
+ assert_eq!(load(&repo, "alice").unwrap(), None);
+ let _ = std::fs::remove_dir_all(&repo);
+ }
}