Conversation
Summary of ChangesHello @nickpismenkov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request extends the attestation reporting capabilities by integrating agent instance attestations into the existing combined attestation report. This allows for a more comprehensive verification of the entire trust chain, including the integrity and authenticity of agent instances running on trusted hardware. The changes involve defining new data structures for agent attestations, updating the API endpoint to accept agent-specific queries, and implementing the necessary backend logic to retrieve and consolidate these attestations. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
PR Review: feat: add agent attestationCritical Issues1. Nonce not forwarded to compose-api (security flaw in attestation)
The Compare to the main attestation flow, where the nonce travels as a query param: let path = format\!("attestation/report?{}", query); // query includes nonceBut in let attestation_path = "attestation/report"; // no nonce\!Fix — pass the nonce as a query parameter: let attestation_path = format\!("attestation/report?nonce={}", request_nonce);2. Path injection via let instance_attestation_path = format\!("instances/{}/attestation", instance_name);
use percent_encoding::{utf8_percent_encode, NON_ALPHANUMERIC};
let encoded = utf8_percent_encode(instance_name, NON_ALPHANUMERIC);
let instance_attestation_path = format\!("instances/{}/attestation", encoded);3. Sequential HTTP calls that can be parallelized The two let (attestation_response, instance_response) = tokio::join\!(
app_state.proxy_service.forward_request(Method::GET, attestation_path, http::HeaderMap::new(), None),
app_state.proxy_service.forward_request(Method::GET, &instance_attestation_path, http::HeaderMap::new(), None),
);
let attestation_response = attestation_response.map_err(|e| { ... })?;
let instance_response = instance_response.map_err(|e| { ... })?;Note: this requires Minor
|
There was a problem hiding this comment.
Code Review
This pull request introduces agent attestation, enhancing trust and security. However, several critical security issues were identified, including a potential panic leading to Denial of Service due to unvalidated nonce length, a missing access control check (IDOR) allowing unauthorized access to agent attestations, and a potential path traversal vulnerability when constructing proxy request paths using user-controlled agent names. Addressing these is crucial for the robustness and security of the attestation service. Additionally, consider refactoring duplicated logic in the new fetch_agent_attestations function to improve maintainability.
There was a problem hiding this comment.
Pull request overview
This PR adds agent attestation functionality to the attestation endpoint, allowing users to retrieve attestation information for their agent instances. The feature integrates agent attestations into the existing combined attestation report that already includes chat-api gateway, cloud-api gateway, and model attestations.
Changes:
- Added new
AgentAttestationmodel with fields for agent instance attestation data (name, image_digest, event_log, intel_quote, TLS certificates, etc.) - Extended
CombinedAttestationReportto include optional agent attestations - Modified attestation endpoint to accept optional
agentquery parameter and fetch agent attestations from compose-api with proper authorization checks
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/api/src/models.rs | Added AgentAttestation struct and updated CombinedAttestationReport to include agent attestations field |
| crates/api/src/routes/attestation.rs | Added agent query parameter, authentication requirement, validation functions (nonce, instance name), and fetch_agent_attestations function with IDOR protection |
Comments suppressed due to low confidence (3)
crates/api/src/routes/attestation.rs:181
- The code clones request_nonce twice (lines 149 and 181) to reuse it later. However, the original request_nonce is only used once at line 130, so you could move that usage to after the clones and avoid one of the clones. Consider whether this micro-optimization is worth the code clarity tradeoff, or if the original single clone approach was clearer.
request_nonce: request_nonce.clone(),
info: None,
vpc: vpc_info,
}
} else {
let client = dstack_sdk::dstack_client::DstackClient::new(None);
let info = client.info().await.map_err(|e| {
tracing::error!(
"Failed to get chat API attestation info, are you running in a CVM?: {:?}",
e
);
ApiError::internal_server_error("Failed to get chat API attestation info")
})?;
let cpu_quote = client.get_quote(report_data).await.map_err(|e| {
tracing::error!(
"Failed to get chat API attestation, are you running in a CVM?: {:?}",
e
);
ApiError::internal_server_error("Failed to get chat API attestation")
})?;
ApiGatewayAttestation {
signing_address: None,
signing_algo: None,
intel_quote: cpu_quote.quote,
event_log: serde_json::from_str(&cpu_quote.event_log)
.map_err(|_| ApiError::internal_server_error("Failed to deserialize event_log"))?,
info: Some(serde_json::to_value(info).map_err(|_| {
ApiError::internal_server_error("Failed to serialize attestation info")
})?),
request_nonce: request_nonce.clone(),
crates/api/src/routes/attestation.rs:257
- The nonce validation logic has an inconsistency: it first checks if the nonce exceeds MAX_NONCE_LEN (256 chars), then checks if it's not exactly EXPECTED_NONCE_LEN (64 chars). This means nonces between 65-256 characters will trigger the "Nonce must be exactly 64 characters" error rather than the "Nonce is too long" error. Consider reordering the checks so the exact length check happens first, and the max length check is only needed as a safety fallback (or remove it entirely if you always require exactly 64 characters).
if nonce.len() > MAX_NONCE_LEN {
tracing::warn!("Nonce exceeds maximum length: {}", nonce.len());
return Err(ApiError::bad_request("Nonce is too long"));
}
if !nonce.chars().all(|c| c.is_ascii_hexdigit()) {
tracing::warn!("Nonce contains non-hex characters");
return Err(ApiError::bad_request("Nonce must be a valid hex string"));
}
if nonce.len() != EXPECTED_NONCE_LEN {
tracing::warn!(
"Nonce has unexpected length: {} (expected {})",
nonce.len(),
EXPECTED_NONCE_LEN
);
return Err(ApiError::bad_request(format!(
"Nonce must be exactly {} characters",
EXPECTED_NONCE_LEN
)));
}
crates/api/src/routes/attestation.rs:270
- The instance name validation rejects backslashes, which might be overly restrictive for Windows-style paths or legitimate instance names. However, given this is likely for Docker container names or similar identifiers that shouldn't contain backslashes anyway, this is probably fine. Consider documenting the allowed character set for instance names in the API documentation or error message to help users understand the constraints.
fn validate_instance_name(name: &str) -> Result<(), ApiError> {
// Reject names containing path traversal sequences
if name.contains("..") || name.contains("/") || name.contains("\\") {
tracing::warn!("Instance name contains invalid characters: {}", name);
return Err(ApiError::bad_request(
"Instance name contains invalid characters",
));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…at-api into feat/agent-attestation
No description provided.