Skip to content

ct_monitor: Use HTTP endpoint instead of pRPC for acme-info #421

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.

Sign up for GitHub

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

Open
h4x3rotab wants to merge 1 commit into
base: master
Choose a base branch
from
+41 −13
Open

ct_monitor: Use HTTP endpoint instead of pRPC for acme-info #421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 1 addition & 3 deletions ct_monitor/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ license.workspace = true
[dependencies]
anyhow.workspace = true
clap = { workspace = true, features = ["derive"] }
hex = { workspace = true, features = ["alloc", "std"] }
hex_fmt.workspace = true
regex.workspace = true
reqwest = { workspace = true, default-features = false, features = ["json", "rustls-tls", "charset", "hickory-dns"] }
Expand All @@ -21,6 +22,3 @@ tokio = { workspace = true, features = ["full"] }
tracing.workspace = true
tracing-subscriber.workspace = true
x509-parser.workspace = true

dstack-gateway-rpc.workspace = true
ra-rpc = { workspace = true, default-features = false, features = ["client"] }
47 changes: 39 additions & 8 deletions ct_monitor/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

use anyhow::{bail, Context, Result};
use clap::Parser;
use dstack_gateway_rpc::gateway_client::GatewayClient;
use ra_rpc::client::RaClient;
use regex::Regex;
use serde::{Deserialize, Serialize};
use std::collections::BTreeSet;
Expand All @@ -15,6 +13,13 @@

const BASE_URL: &str = "https://crt.sh";

#[derive(Debug, Deserialize)]
struct AcmeInfoResponse {
#[allow(dead_code)]
account_uri: String,
hist_keys: Vec<String>,
}

struct Monitor {
gateway_uri: String,
domain: String,
Expand Down Expand Up @@ -48,12 +53,38 @@
}

async fn refresh_known_keys(&mut self) -> Result<()> {
info!("fetching known public keys from {}", self.gateway_uri);
// TODO: Use RA-TLS
let tls_no_check = true;
let rpc = GatewayClient::new(RaClient::new(self.gateway_uri.clone(), tls_no_check)?);
let info = rpc.acme_info().await?;
self.known_keys = info.hist_keys.into_iter().collect();
let acme_info_url = format!("{}/acme-info", self.gateway_uri.trim_end_matches('/'));
info!("fetching known public keys from {}", acme_info_url);

let client = reqwest::Client::builder()
.danger_accept_invalid_certs(true) // TODO: Use RA-TLS verification

Check failure

Code scanning / CodeQL

Disabled TLS certificate check High

Disabling TLS certificate validation can expose the application to man-in-the-middle attacks.

Copilot Autofix

AI about 11 hours ago

In general, the fix is to ensure TLS certificate validation is enabled for the reqwest client used in refresh_known_keys. This is done by not calling danger_accept_invalid_certs(true) (relying on the secure default) or explicitly setting it to false. This preserves existing functionality (an HTTPS GET to /acme-info and JSON parsing) while restoring protection against MITM attacks.

The best minimal change in ct_monitor/src/main.rs is to configure the client as:

let client = reqwest::Client::builder()
    .danger_accept_invalid_certs(false)
    .build()
    .context("failed to build http client")?;

This keeps the structure and error handling intact and simply removes the insecure behavior. We do not add any new imports or change other logic; we only adjust the TLS configuration at line 60. If RA-TLS or more advanced verification is to be added later, it can be implemented on top of this secure baseline, but for now we just ensure standard certificate verification is not disabled.

Concretely:

  • Edit ct_monitor/src/main.rs, within impl Monitor { async fn refresh_known_keys(&mut self) -> Result<()> { ... } }.
  • Replace the .danger_accept_invalid_certs(true) call with .danger_accept_invalid_certs(false) (or remove the call entirely; here we choose false to be explicit, matching the background’s “GOOD” example).
  • Keep the existing comment or adjust it to reflect that the temporary insecure workaround is gone; however, to minimize non-functional change, we can leave the comment in place if desired.

No additional methods, imports, or definitions are required for this fix.

Suggested changeset 1
ct_monitor/src/main.rs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/ct_monitor/src/main.rs b/ct_monitor/src/main.rs
--- a/ct_monitor/src/main.rs
+++ b/ct_monitor/src/main.rs
@@ -57,7 +57,7 @@
         info!("fetching known public keys from {}", acme_info_url);
 
         let client = reqwest::Client::builder()
-            .danger_accept_invalid_certs(true) // TODO: Use RA-TLS verification
+            .danger_accept_invalid_certs(false) // TODO: Use RA-TLS verification
             .build()
             .context("failed to build http client")?;
 
EOF
@@ -57,7 +57,7 @@
info!("fetching known public keys from {}", acme_info_url);

let client = reqwest::Client::builder()
.danger_accept_invalid_certs(true) // TODO: Use RA-TLS verification
.danger_accept_invalid_certs(false) // TODO: Use RA-TLS verification
.build()
.context("failed to build http client")?;

Copilot is powered by AI and may make mistakes. Always verify output.
.build()
.context("failed to build http client")?;

let response = client
.get(&acme_info_url)
.send()
.await
.context("failed to fetch acme-info")?;

if !response.status().is_success() {
bail!(
"failed to fetch acme-info: HTTP {}",
response.status().as_u16()
);
}

let info: AcmeInfoResponse = response
.json()
.await
.context("failed to parse acme-info response")?;

self.known_keys = info
.hist_keys
.into_iter()
.map(|hex_key| hex::decode(&hex_key).context("invalid hex in hist_keys"))
.collect::<Result<BTreeSet<_>>>()?;

info!("got {} known public keys", self.known_keys.len());
for key in self.known_keys.iter() {
debug!(" {}", hex_fmt::HexFmt(key));
Expand Down