fix(credentials): harden endpoint-bound rotation

Signed-off-by: John Myers <9696606+johntmyers@users.noreply.github.com>

Author: johntmyers

crates/openshell-core/src/provider_credentials.rs

@@ -30,6 +30,13 @@ struct ProviderCredentialStateInner {
     suppressed_keys: HashSet<String>,
     static_credential_bindings: HashMap<String, StaticCredentialBinding>,
     known_static_credential_keys: HashSet<String>,
+    static_credential_identity_epochs: HashMap<String, StaticCredentialIdentityEpoch>,
+}
+
+#[derive(Debug)]
+struct StaticCredentialIdentityEpoch {
+    identity: String,
+    first_revision: u64,
 }
 
 #[derive(Debug, Clone)]
@@ -81,6 +88,7 @@ impl ProviderCredentialState {
                 suppressed_keys: HashSet::new(),
                 static_credential_bindings: HashMap::new(),
                 known_static_credential_keys: HashSet::new(),
+                static_credential_identity_epochs: HashMap::new(),
             })),
         }
     }
@@ -108,6 +116,11 @@ impl ProviderCredentialState {
             inner
                 .known_static_credential_keys
                 .extend(static_credential_bindings.keys().cloned());
+            update_static_credential_identity_epochs(
+                &mut inner.static_credential_identity_epochs,
+                revision,
+                &static_credential_bindings,
+            );
             inner.static_credential_bindings = static_credential_bindings;
         }
         Ok(state)
@@ -137,6 +150,7 @@ impl ProviderCredentialState {
                 suppressed_keys: HashSet::new(),
                 static_credential_bindings: HashMap::new(),
                 known_static_credential_keys: HashSet::new(),
+                static_credential_identity_epochs: HashMap::new(),
             })),
         }
     }
@@ -171,6 +185,7 @@ impl ProviderCredentialState {
         inner.combined_resolver = None;
         inner.static_credential_bindings.clear();
         inner.known_static_credential_keys.clear();
+        inner.static_credential_identity_epochs.clear();
         inner.current.child_env.len()
     }
 
@@ -205,7 +220,7 @@ impl ProviderCredentialState {
             .read()
             .expect("provider credential state poisoned");
         let request_path = path.split_once('?').map_or(path, |(path, _)| path);
-        let allowed = inner
+        let allowed: HashSet<String> = inner
             .static_credential_bindings
             .iter()
             .filter(|(_, binding)| {
@@ -216,7 +231,23 @@ impl ProviderCredentialState {
             .map(|(key, _)| key.clone())
             .collect();
         inner.combined_resolver.as_ref().map(|resolver| {
-            Arc::new(resolver.scoped_to_env_keys(&inner.known_static_credential_keys, &allowed))
+            let revision_fallback_min_revisions = inner
+                .static_credential_identity_epochs
+                .iter()
+                .filter(|(key, epoch)| {
+                    allowed.contains(*key)
+                        && inner
+                            .static_credential_bindings
+                            .get(*key)
+                            .is_some_and(|binding| binding.credential_identity == epoch.identity)
+                })
+                .map(|(key, epoch)| (key.clone(), epoch.first_revision))
+                .collect();
+            Arc::new(resolver.scoped_to_env_keys(
+                &inner.known_static_credential_keys,
+                &allowed,
+                revision_fallback_min_revisions,
+            ))
         })
     }
 
@@ -442,6 +473,11 @@ impl ProviderCredentialState {
         inner
             .known_static_credential_keys
             .extend(static_credential_bindings.keys().cloned());
+        update_static_credential_identity_epochs(
+            &mut inner.static_credential_identity_epochs,
+            revision,
+            &static_credential_bindings,
+        );
         inner.static_credential_bindings = static_credential_bindings;
         Ok(inner.current.child_env.len())
     }
@@ -474,6 +510,7 @@ impl ProviderCredentialState {
         inner.current_resolver = None;
         inner.combined_resolver = None;
         inner.static_credential_bindings.clear();
+        inner.static_credential_identity_epochs.clear();
     }
 }
 
@@ -545,6 +582,32 @@ fn static_credential_identities(
         .collect()
 }
 
+fn update_static_credential_identity_epochs(
+    epochs: &mut HashMap<String, StaticCredentialIdentityEpoch>,
+    revision: u64,
+    bindings: &HashMap<String, StaticCredentialBinding>,
+) {
+    epochs.retain(|key, _| bindings.contains_key(key));
+    for (key, binding) in bindings {
+        match epochs.get_mut(key) {
+            Some(epoch) if epoch.identity == binding.credential_identity => {}
+            Some(epoch) => {
+                epoch.identity.clone_from(&binding.credential_identity);
+                epoch.first_revision = revision;
+            }
+            None => {
+                epochs.insert(
+                    key.clone(),
+                    StaticCredentialIdentityEpoch {
+                        identity: binding.credential_identity.clone(),
+                        first_revision: revision,
+                    },
+                );
+            }
+        }
+    }
+}
+
 fn binding_error(message: &str) -> StaticCredentialBindingError {
     StaticCredentialBindingError {
         message: message.to_string(),
@@ -722,6 +785,47 @@ mod tests {
         );
     }
 
+    #[test]
+    fn aged_generation_falls_back_after_many_rotations_of_same_provider_credential() {
+        let state = ProviderCredentialState::from_bound_environment(
+            1,
+            HashMap::from([("API_KEY".to_string(), "secret-1".to_string())]),
+            HashMap::new(),
+            HashMap::new(),
+            HashMap::from([(
+                "API_KEY".to_string(),
+                binding("api.example.com", 443, "/**"),
+            )]),
+            Vec::new(),
+        )
+        .expect("initial bindings");
+
+        for revision in 2..=10 {
+            state
+                .install_bound_environment(
+                    revision,
+                    HashMap::from([("API_KEY".to_string(), format!("secret-{revision}"))]),
+                    HashMap::new(),
+                    HashMap::new(),
+                    HashMap::from([(
+                        "API_KEY".to_string(),
+                        binding("api.example.com", 443, "/**"),
+                    )]),
+                    Vec::new(),
+                )
+                .expect("rotated bindings");
+        }
+
+        let resolver = state
+            .resolver_for_endpoint("api.example.com", 443, "/v1")
+            .expect("resolver");
+        assert_eq!(
+            resolver.resolve_placeholder("openshell:resolve:env:v1_API_KEY"),
+            Some("secret-10"),
+            "an aged-out placeholder may use the current secret while its provider identity is unchanged"
+        );
+    }
+
     #[test]
     fn replacing_provider_with_reused_key_purges_retained_generation() {
         let state = ProviderCredentialState::from_bound_environment(

crates/openshell-core/src/secrets.rs

@@ -123,6 +123,7 @@ pub struct SecretResolver {
     by_placeholder: HashMap<String, SecretValue>,
     denied_env_keys: std::collections::HashSet<String>,
     no_revision_fallback_env_keys: std::collections::HashSet<String>,
+    revision_fallback_min_revisions: HashMap<String, u64>,
 }
 
 #[derive(Clone)]
@@ -245,6 +246,7 @@ impl SecretResolver {
                     by_placeholder,
                     denied_env_keys: std::collections::HashSet::new(),
                     no_revision_fallback_env_keys: std::collections::HashSet::new(),
+                    revision_fallback_min_revisions: HashMap::new(),
                 }),
             )
         }
@@ -254,11 +256,14 @@ impl SecretResolver {
         let mut by_placeholder = HashMap::new();
         let mut denied_env_keys = std::collections::HashSet::new();
         let mut no_revision_fallback_env_keys = std::collections::HashSet::new();
+        let mut revision_fallback_min_revisions = HashMap::new();
         for resolver in resolvers {
             by_placeholder.extend(resolver.by_placeholder.clone());
             denied_env_keys.extend(resolver.denied_env_keys.iter().cloned());
             no_revision_fallback_env_keys
                 .extend(resolver.no_revision_fallback_env_keys.iter().cloned());
+            revision_fallback_min_revisions
+                .extend(resolver.revision_fallback_min_revisions.clone());
         }
         if by_placeholder.is_empty() {
             None
@@ -267,6 +272,7 @@ impl SecretResolver {
                 by_placeholder,
                 denied_env_keys,
                 no_revision_fallback_env_keys,
+                revision_fallback_min_revisions,
             })
         }
     }
@@ -281,6 +287,7 @@ impl SecretResolver {
         &self,
         bound_keys: &std::collections::HashSet<String>,
         allowed_bound_keys: &std::collections::HashSet<String>,
+        revision_fallback_min_revisions: HashMap<String, u64>,
     ) -> Self {
         let denied_env_keys = bound_keys
             .difference(allowed_bound_keys)
@@ -298,6 +305,7 @@ impl SecretResolver {
             by_placeholder,
             denied_env_keys,
             no_revision_fallback_env_keys: bound_keys.clone(),
+            revision_fallback_min_revisions,
         }
     }
 
@@ -331,8 +339,12 @@ impl SecretResolver {
             // to one provider identity, so falling back by key after replacement
             // would let the old process obtain the replacement provider's secret.
             let key = revisioned_placeholder_env_key(value).or_else(|| alias_env_key(value))?;
-            if revisioned_placeholder_env_key(value).is_some()
+            if let Some((revision, key)) = revisioned_placeholder_parts(value)
                 && self.no_revision_fallback_env_keys.contains(key)
+                && self
+                    .revision_fallback_min_revisions
+                    .get(key)
+                    .is_none_or(|minimum| revision < *minimum)
             {
                 return None;
             }
@@ -615,9 +627,13 @@ fn alias_env_key(token: &str) -> Option<&str> {
 }
 
 fn revisioned_placeholder_env_key(token: &str) -> Option<&str> {
+    revisioned_placeholder_parts(token).map(|(_, key)| key)
+}
+
+fn revisioned_placeholder_parts(token: &str) -> Option<(u64, &str)> {
     let suffix = token.strip_prefix(PLACEHOLDER_PREFIX)?;
-    let (_, key) = split_revisioned_env_key(suffix)?;
-    Some(key)
+    let (revision, key) = split_revisioned_env_key(suffix)?;
+    Some((revision.parse().ok()?, key))
 }
 
 fn placeholder_env_key(token: &str) -> Option<&str> {