Support Direct Import of Certificates via URL Feature req #3350#3351
Support Direct Import of Certificates via URL Feature req #3350#3351BViganotti wants to merge 3 commits intogolemfactory:masterfrom
Conversation
…y#3350 Adding the import-cert-url command to import a cert with a url
|
|
There was a problem hiding this comment.
Pull request overview
Adds a new import-cert-url CLI flow to let the provider import certificates directly from an http(s) URL, implemented via a small download helper and new rule manager support.
Changes:
- Adds
cert_utilswithreqwest-based blocking download into a temporary location. - Extends
RulesManagerwith a temp download directory andimport_certs_from_url. - Adds new
ImportCertUrlvariants to the rule CLI for both audited-payload (X509) and partner (Golem) rules.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
agent/provider/src/rules.rs |
Adds temp download directory and import_certs_from_url helper on RulesManager. |
agent/provider/src/lib.rs |
Registers the new internal cert_utils module. |
agent/provider/src/cli/rule.rs |
Adds import-cert-url subcommands and wiring into rule setting flows. |
agent/provider/src/cert_utils.rs |
Implements URL detection and blocking HTTP download to a temp path. |
agent/provider/Cargo.toml |
Adds reqwest (blocking) + tempfile, bumps url crate. |
Cargo.lock |
Updates lockfile for new dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let response = reqwest::blocking::get(url)?; | ||
| if !response.status().is_success() { | ||
| return Err(anyhow!( | ||
| "Failed to download certificate. Status: {}", | ||
| response.status() | ||
| )); | ||
| } | ||
|
|
||
| // Save the certificate to a temporary file | ||
| let content = response.bytes()?; | ||
| fs::write(&temp_path, content)?; |
There was a problem hiding this comment.
reqwest::blocking::get(url)? uses default timeouts, which can hang indefinitely on slow/unresponsive endpoints and block the CLI. Consider using a reqwest::blocking::Client with explicit connect/request timeouts (and optionally a max download size) to avoid stuck commands and excessive memory use from response.bytes().
| /// Import and set rule for X509 certificate from URL. | ||
| ImportCertUrl { | ||
| /// URL to X509 certificate or X509 certificates chain. | ||
| url: String, | ||
| #[structopt(short, long, possible_values = Mode::VARIANTS)] |
There was a problem hiding this comment.
There are integration tests for the existing import-cert CLI paths (see agent/provider/tests/rule_cli.rs), but none covering the new import-cert-url subcommands. Adding a test that serves a cert over a local HTTP endpoint (or uses an HTTP mocking crate) would help prevent regressions and confirm URL import works end-to-end.
| } = rules.keystore.add_x509_cert(&AddParams { | ||
| certs: vec![PathBuf::from(url)], | ||
| })?; |
There was a problem hiding this comment.
ImportCertUrl for audited-payload passes PathBuf::from(url) directly into add_x509_cert, which expects a local filesystem path. For http(s)://... inputs this will fail to import (it never downloads). Consider reusing the same URL-download path as RulesManager::import_certs_from_url (or a dedicated download_cert_if_url helper here) and pass the downloaded temp file path into add_x509_cert.
| } = rules.keystore.add_x509_cert(&AddParams { | |
| certs: vec![PathBuf::from(url)], | |
| })?; | |
| } = rules.import_certs_from_url(&url)?; |
| let temp_dir = tempfile::Builder::new() | ||
| .prefix("ya-provider-certs-") | ||
| .tempdir()? | ||
| .into_path(); | ||
|
|
There was a problem hiding this comment.
tempfile::Builder::tempdir()?.into_path() prevents the temporary directory from being deleted on drop, so each run will leave a ya-provider-certs-* directory behind. If you want automatic cleanup, keep a TempDir (e.g., Arc<TempDir>) in RulesManager or create a per-download NamedTempFile instead of persisting the directory path.
| path::{Path, PathBuf}, | ||
| }; | ||
| use strum_macros::Display; | ||
| use tempfile; |
There was a problem hiding this comment.
use tempfile; is unused (the code references tempfile::Builder via the crate path), which will trigger an unused_imports warning. Remove this import or import the specific items you need.
| use tempfile; |
| // Generate a unique filename based on the URL | ||
| let file_name = url | ||
| .split('/') | ||
| .last() | ||
| .ok_or_else(|| anyhow!("Invalid URL format"))?; | ||
| let temp_path = temp_dir.join(file_name); |
There was a problem hiding this comment.
Deriving file_name by splitting the full URL string can produce invalid/unsafe filenames (e.g., cert.pem?download=1 includes ?, which is invalid on Windows) and .../ yields an empty filename (leading to attempts to write to the directory path). Prefer using Url parsing (path_segments) and generating a sanitized, unique name (e.g., tempfile::Builder::new().suffix(".pem")...) rather than trusting the URL tail.
Adding the import-cert-url command to import a cert with a url.
Chose to add this as a separate command to keep existing working functionality