-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Ignore SSL_CERT_FILE with empty value
#16772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Ignore SSL_CERT_FILE with empty value
#16772
Conversation
SSL_CERT_FILE with empty value
| // ** Set SSL_CERT_FILE to an empty string | ||
| // ** Then verify it is treated as unset (falls back to default behavior) | ||
|
|
||
| unsafe { | ||
| std::env::set_var(EnvVars::SSL_CERT_FILE, ""); | ||
| } | ||
| let (server_task, addr) = start_https_user_agent_server(&standalone_server_cert).await?; | ||
| let url = DisplaySafeUrl::from_str(&format!("https://{addr}"))?; | ||
| let cache = Cache::temp()?.init()?; | ||
| let client = RegistryClientBuilder::new(BaseClientBuilder::default(), cache).build(); | ||
| let res = client | ||
| .cached_client() | ||
| .uncached() | ||
| .for_host(&url) | ||
| .get(Url::from(url)) | ||
| .send() | ||
| .await; | ||
| unsafe { | ||
| std::env::remove_var(EnvVars::SSL_CERT_FILE); | ||
| } | ||
|
|
||
| // Empty SSL_CERT_FILE should be ignored, falling back to default certificate handling. | ||
| // The connection will fail because we're using a self-signed cert without providing the cert, | ||
| // but the important thing is it doesn't fail due to "empty path does not exist". | ||
| let Some(reqwest_middleware::Error::Middleware(middleware_error)) = res.err() else { | ||
| panic!("expected middleware error"); | ||
| }; | ||
| let reqwest_error = middleware_error | ||
| .chain() | ||
| .find_map(|err| { | ||
| err.downcast_ref::<reqwest_middleware::Error>().map(|err| { | ||
| if let reqwest_middleware::Error::Reqwest(inner) = err { | ||
| inner | ||
| } else { | ||
| panic!("expected reqwest error") | ||
| } | ||
| }) | ||
| }) | ||
| .expect("expected reqwest error"); | ||
| assert!(reqwest_error.is_connect()); | ||
|
|
||
| // Validate the server error - should be UnknownCA (not a path-related error) | ||
| let server_res = server_task.await?; | ||
| let expected_err = if let Err(anyhow_err) = server_res | ||
| && let Some(io_err) = anyhow_err.downcast_ref::<std::io::Error>() | ||
| && let Some(wrapped_err) = io_err.get_ref() | ||
| && let Some(tls_err) = wrapped_err.downcast_ref::<rustls::Error>() | ||
| && matches!( | ||
| tls_err, | ||
| rustls::Error::AlertReceived(AlertDescription::UnknownCA) | ||
| ) { | ||
| true | ||
| } else { | ||
| false | ||
| }; | ||
| assert!(expected_err); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding a test!
In this case the test with SSL_CERT_FILE="" would behave the same way before and after the code changes given this is a server using a self-signed certificate, hence I don't think its actually verifying the changes.
Having said that, I'm not quite sure if we should have a test for this change. If we'd like to have a test I believe we can add one to the bottom of this file that specifically tests the warning scenarios for these env vars, e.g. something like
// SAFETY: This test is meant to run in isolation
#[tokio::test]
#[allow(unsafe_code)]
async fn ssl_env_vars_warnings() -> Result<()> {
// Enable user-facing warnings
uv_warnings::enable();
unsafe {
std::env::set_var(EnvVars::SSL_CERT_FILE, "");
}
let (server_task, addr) = start_http_user_agent_server().await?;
let url = DisplaySafeUrl::from_str(&format!("http://{addr}"))?;
let cache = Cache::temp()?.init()?;
let client = RegistryClientBuilder::new(BaseClientBuilder::default(), cache).build();
let _ = client
.cached_client()
.uncached()
.for_host(&url)
.get(Url::from(url))
.send()
.await;
let _ = server_task.await?;
unsafe {
std::env::remove_var(EnvVars::SSL_CERT_FILE);
}
// Check no warning was emitted
let warning = uv_warnings::WARNINGS
.lock()
.expect("Failed to retrieve warnings")
.iter()
.any(|msg| msg.starts_with("Ignoring invalid `SSL_CERT_FILE`."));
assert!(!warning, "Unexpected warning emitted");
unsafe {
std::env::set_var(EnvVars::SSL_CERT_FILE, "foo");
}
let (server_task, addr) = start_http_user_agent_server().await?;
let url = DisplaySafeUrl::from_str(&format!("http://{addr}"))?;
let cache = Cache::temp()?.init()?;
let client = RegistryClientBuilder::new(BaseClientBuilder::default(), cache).build();
let _ = client
.cached_client()
.uncached()
.for_host(&url)
.get(Url::from(url))
.send()
.await;
let _ = server_task.await?;
unsafe {
std::env::remove_var(EnvVars::SSL_CERT_FILE);
}
// Check warning was emitted
let warning = uv_warnings::WARNINGS
.lock()
.expect("Failed to retrieve warnings")
.iter()
.any(|msg| msg.starts_with("Ignoring invalid `SSL_CERT_FILE`."));
assert!(warning, "Expected warning to be emitted");
// Disable user-facing warnings
uv_warnings::disable();
Ok(())
}Note, this would require you to use nextest to run these tests as you'll need process isolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the feedback! I see what you're saying, good catch.
It seems like maybe a test isn't necessary for this, but I'd be happy to add the test you suggested if you'd like.
for now I'll remove the test case I added.
samypr100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Summary
Fixes #16712.
This PR adds empty value filtering to
SSL_CERT_FILEenvironment variable handling, matching the existing behavior ofSSL_CERT_DIR.Previously, setting
SSL_CERT_FILE=""would cause uv to attempt to use an empty string as a file path, resulting in a "file does not exist" warning. With this change, an emptySSL_CERT_FILEis treated as unset, falling back to default certificate handling.The fix adds
.filter(|v| !v.is_empty())to theSSL_CERT_FILEprocessing incrates/uv-client/src/base_client.rs, consistent with the pattern used forSSL_CERT_DIRand other environment variables likeNO_COLORandFORCE_COLOR.Test Plan
Added a test case to
crates/uv-client/tests/it/ssl_certs.rsthat:SSL_CERT_FILEto an empty stringUnknownCA(indicating default certificate handling was used, not a path-related error)Run the test with:
cargo test --package uv-client --test it -- ssl_env_vars --exact