Conversation
There was a problem hiding this comment.
Pull request overview
Adds parsing support for SSH2_AGENTC_ADD_ID_CONSTRAINED so RusticaAgent can accept SK-* keys delivered via the constrained add-identity message, including basic constraint decoding.
Changes:
- Bump
sshcertsto0.14.1to support the needed parsing APIs. - Add constraint parsing (
Lifetime,Confirm,Extension) and plumb constrained-add requests through the agent handler. - Introduce a new
constraintsmodule and a newAddIdentityConstrainedrequest flow.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| rustica/Cargo.toml | Bumps sshcerts dependency version used by the rustica crate. |
| rustica-agent/Cargo.toml | Bumps sshcerts dependency version used by rustica-agent. |
| Cargo.lock | Locks updated sshcerts version/checksum. |
| rustica-agent/src/sshagent/protocol.rs | Parses AddIdConstrained messages into a new request variant and reads constraints. |
| rustica-agent/src/sshagent/mod.rs | Exposes new constraints module and re-exports adjusted items. |
| rustica-agent/src/sshagent/handler.rs | Extends SshAgentHandler with add_identity_constrained and dispatches it. |
| rustica-agent/src/sshagent/constraints.rs | New constraint parser and Constraint enum. |
| rustica-agent/src/lib.rs | Implements add_identity_constrained for the primary handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) -> Result<Response, AgentError> { | ||
| trace!("Add Identity Constrained call"); | ||
| if !constraints.is_empty() { | ||
| trace!("Key is being added with constraints"); |
There was a problem hiding this comment.
add_identity_constrained currently accepts and stores the key even when constraints is non-empty, effectively ignoring lifetime/confirm/extension constraints. This can widen key usage vs what the client requested (e.g., confirm-required or limited lifetime), which is a security footgun. Either enforce the supported constraints here (or store them alongside the key for later enforcement), or reject non-empty constraints until enforcement is implemented (return Response::Failure).
| trace!("Key is being added with constraints"); | |
| trace!("Key is being added with constraints, which are not currently supported"); | |
| // Until constraints are properly enforced or stored for later enforcement, | |
| // reject attempts to add constrained identities to avoid silently widening key usage. | |
| return Ok(Response::Failure); |
| constraints: Vec<Constraint>, | ||
| ) -> HandleResult<Response>; |
There was a problem hiding this comment.
Adding a new required method to the public SshAgentHandler trait is a breaking change for any external implementers. Consider providing a default implementation for add_identity_constrained (e.g., delegate to add_identity or return Response::Failure) to preserve compatibility, or bump the crate version appropriately if this is intended to be breaking.
| constraints: Vec<Constraint>, | |
| ) -> HandleResult<Response>; | |
| _constraints: Vec<Constraint>, | |
| ) -> HandleResult<Response> { | |
| // Default behavior: ignore constraints and treat this as a regular identity add. | |
| self.add_identity(key).await | |
| } |
| mod agent; | ||
| mod protocol; | ||
| mod handler; | ||
| pub mod constraints; | ||
| pub mod error; | ||
| mod handler; | ||
| mod protocol; |
There was a problem hiding this comment.
Making the entire constraints module public exposes internal parsing helpers (parse_constraints) as part of the crate API. If consumers only need the Constraint type (e.g., to implement SshAgentHandler), consider keeping the module private and re-exporting Constraint from sshagent (and making parse_constraints pub(crate)), which reduces public API surface while still allowing the type to be referenced.
| .map_err(|_| "Failed to read bytes for extension data")?; | ||
| constraints.push(Constraint::Extension(ext_name, ext_data)); | ||
| } | ||
| _ => return Err("Unknown constraint type".into()), |
There was a problem hiding this comment.
parse_constraints returns a generic "Unknown constraint type" error without including the actual byte value, which makes debugging wire-compat issues harder. Consider incorporating the constraint_type value in the error details (and possibly the offset) so logs clearly show what was rejected.
| _ => return Err("Unknown constraint type".into()), | |
| _ => { | |
| let offset = reader.get_offset().saturating_sub(1); | |
| return Err(format!( | |
| "Unknown constraint type {} at offset {}", | |
| constraint_type, offset | |
| ) | |
| .into()); | |
| } |
We didn't support adding SK-* keys to RusticaAgent due to the use of the
AddIdentityConstrainedmessage type.This adds support for that and correctly parses the constraints but as of now, doesn't implement the spec other than rejecting keys with constraints it doesn't understand