Deuxfleurs/tricot
16
3
Fork 4
Code Issues 5 Pull requests 2 Projects Releases Wiki Activity

Add support for domain checks. #11

Merged
quentin merged 3 commits from domain-check into main 2023-11-30 17:07:58 +00:00
Conversation 1 Commits 3 Files changed 2 +52 -50
1 changed files with 52 additions and 50 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit b9b035034f - Show all commits

74
src/cert_store.rs
View file

@ -22,12 +22,19 @@ pub struct CertStore {
consul: Consul, consul: Consul,
node_name: String, node_name: String,
letsencrypt_email: String, letsencrypt_email: String,
certs: RwLock<HashMap<String, Arc<Cert>>>, certs: RwLock<HashMap<String, Arc<Cert>>>,
self_signed_certs: RwLock<HashMap<String, Arc<Cert>>>, self_signed_certs: RwLock<HashMap<String, Arc<Cert>>>,
rx_proxy_config: watch::Receiver<Arc<ProxyConfig>>, rx_proxy_config: watch::Receiver<Arc<ProxyConfig>>,
tx_need_cert: mpsc::UnboundedSender<String>, tx_need_cert: mpsc::UnboundedSender<String>,
} }
struct ProcessedDomains {
static_domains: HashSet<String>,
on_demand_domains: Vec<(glob::Pattern, Option<String>)>,
}
impl CertStore { impl CertStore {
pub fn new( pub fn new(
consul: Consul, consul: Consul,
@ -41,10 +48,10 @@ impl CertStore {
let cert_store = Arc::new(Self { let cert_store = Arc::new(Self {
consul, consul,
node_name, node_name,
letsencrypt_email,
certs: RwLock::new(HashMap::new()), certs: RwLock::new(HashMap::new()),
self_signed_certs: RwLock::new(HashMap::new()), self_signed_certs: RwLock::new(HashMap::new()),
rx_proxy_config, rx_proxy_config,
letsencrypt_email,
tx_need_cert: tx, tx_need_cert: tx,
}); });
@ -66,23 +73,21 @@ impl CertStore {
let mut rx_proxy_config = self.rx_proxy_config.clone(); let mut rx_proxy_config = self.rx_proxy_config.clone();
let mut t_last_check: HashMap<String, Instant> = HashMap::new(); let mut t_last_check: HashMap<String, Instant> = HashMap::new();
let mut proc_domains: Option<ProcessedDomains> = None;
// Collect data from proxy config
let mut static_domains: HashSet<String> = HashSet::new();
let mut on_demand_checks: Vec<(glob::Pattern, Option<String>)> = vec![];
loop { loop {
// Collect domains that need a TLS certificate
// either from the proxy configuration (eagerly)
// or on reaction to a user request (lazily)
let domains = select! { let domains = select! {
// Refresh some internal states, schedule static_domains for renew
res = rx_proxy_config.changed() => { res = rx_proxy_config.changed() => {
if res.is_err() { if res.is_err() {
bail!("rx_proxy_config closed"); bail!("rx_proxy_config closed");
} }
on_demand_checks.clear(); let mut static_domains: HashSet<String> = HashSet::new();
let mut on_demand_domains: Vec<(glob::Pattern, Option<String>)> = vec![];
let proxy_config: Arc<ProxyConfig> = rx_proxy_config.borrow().clone(); let proxy_config: Arc<ProxyConfig> = rx_proxy_config.borrow().clone();
for ent in proxy_config.entries.iter() { for ent in proxy_config.entries.iter() {
// Eagerly generate certificates for domains that // Eagerly generate certificates for domains that
// are not patterns // are not patterns
@ -90,21 +95,21 @@ impl CertStore {
HostDescription::Hostname(domain) => { HostDescription::Hostname(domain) => {
if let Some((host, _port)) = domain.split_once(':') { if let Some((host, _port)) = domain.split_once(':') {
static_domains.insert(host.to_string()); static_domains.insert(host.to_string());
//domains.insert(host.to_string());
} else { } else {
static_domains.insert(domain.clone()); static_domains.insert(domain.clone());
//domains.insert(domain.clone());
} }
}, },
HostDescription::Pattern(pattern) => { HostDescription::Pattern(pattern) => {
on_demand_checks.push((pattern.clone(), ent.on_demand_tls_ask.clone())); on_demand_domains.push((pattern.clone(), ent.on_demand_tls_ask.clone()));
} },
} }
} }
// only static_domains are refreshed // only static_domains are refreshed
static_domains.clone() proc_domains = Some(ProcessedDomains { static_domains: static_domains.clone(), on_demand_domains });
self.domain_validation(static_domains, proc_domains.as_ref()).await
} }
// renew static and on-demand domains
need_cert = rx_need_cert.recv() => { need_cert = rx_need_cert.recv() => {
match need_cert { match need_cert {
Some(dom) => { Some(dom) => {
@ -116,7 +121,7 @@ impl CertStore {
candidates.insert(dom2); candidates.insert(dom2);
} }
self.domain_validation(candidates, &static_domains, on_demand_checks.as_slice()).await self.domain_validation(candidates, proc_domains.as_ref()).await
} }
None => bail!("rx_need_cert closed"), None => bail!("rx_need_cert closed"),
} }
@ -145,28 +150,36 @@ impl CertStore {
async fn domain_validation( async fn domain_validation(
&self, &self,
candidates: HashSet<String>, candidates: HashSet<String>,
static_domains: &HashSet<String>, maybe_proc_domains: Option<&ProcessedDomains>,
checks: &[(glob::Pattern, Option<String>)],
) -> HashSet<String> { ) -> HashSet<String> {
let mut domains: HashSet<String> = HashSet::new(); let mut domains: HashSet<String> = HashSet::new();
// Handle initialization
let proc_domains = match maybe_proc_domains {
None => {
warn!("Proxy config is not yet loaded, refusing all certificate generation");
return domains;
lx commented 2023-11-30 17:16:13 +00:00
Review
Copy link

Du coup ici ça devrait plutôt être return HashSet::new() non?

Du coup ici ça devrait plutôt être `return HashSet::new()` non?
quentin commented 2023-11-30 18:16:23 +00:00
Review
Copy link

Il me semble que c'est équivalent non ? domains est intialisé avec HashSet::new() juste au dessus et on a pas eu l'occasion de le modifier.

Il me semble que c'est équivalent non ? domains est intialisé avec HashSet::new() juste au dessus et on a pas eu l'occasion de le modifier.
}
Some(proc) => proc,
};
// Filter certificates... // Filter certificates...
for candidate in candidates.into_iter() { 'outer: for candidate in candidates.into_iter() {
// Disallow obvious wrong domains... // Disallow obvious wrong domains...
if !candidate.contains('.') || candidate.ends_with(".local") { if !candidate.contains('.') || candidate.ends_with(".local") {
warn!("Probably not a publicly accessible domain, skipping (a self-signed certificate will be used)"); warn!("{} is probably not a publicly accessible domain, skipping (a self-signed certificate will be used)", candidate);
continue; continue;
} }
// Try to register domain as a static domain // Try to register domain as a static domain
if static_domains.contains(&candidate) { if proc_domains.static_domains.contains(&candidate) {
trace!("domain {} validated as static domain", candidate); trace!("domain {} validated as static domain", candidate);
domains.insert(candidate); domains.insert(candidate);
continue; continue;
} }
// It's not a static domain, maybe an on-demand domain? // It's not a static domain, maybe an on-demand domain?
for (pattern, maybe_check_url) in checks.iter() { for (pattern, maybe_check_url) in proc_domains.on_demand_domains.iter() {
lx commented 2023-11-30 17:20:58 +00:00
Review
Copy link

Pour éviter la double boucle et de devoir faire des break 'outer, je propose de réécrire plutôt:

If let Some((patern, maybe_check_url)) = proc_domains.on_demand_domains.iter().find(|(pattern, _)| pattern.matches(&candidate)) {

Pour éviter la double boucle et de devoir faire des `break 'outer`, je propose de réécrire plutôt: `If let Some((patern, maybe_check_url)) = proc_domains.on_demand_domains.iter().find(|(pattern, _)| pattern.matches(&candidate)) {`
quentin commented 2023-11-30 18:17:30 +00:00
Review
Copy link

Ah oui ça me semble bien !

Ah oui ça me semble bien !
// check glob pattern // check glob pattern
if pattern.matches(&candidate) { if pattern.matches(&candidate) {
// if no check url is set, accept domain as long as it matches the pattern // if no check url is set, accept domain as long as it matches the pattern
@ -178,12 +191,14 @@ impl CertStore {
pattern pattern
); );
domains.insert(candidate); domains.insert(candidate);
break; continue 'outer;
} }
Some(url) => url, Some(url) => url,
}; };
// if a check url is set, call it // if a check url is set, call it
// -- avoid DDoSing a backend
tokio::time::sleep(Duration::from_secs(2)).await;
lx commented 2023-11-30 17:18:34 +00:00
Review
Copy link

Je pense que le délai peut être abaissé à 100ms, voir complètement supprimé, en tout cas dans un cas où c'est Garage le backend

Je pense que le délai peut être abaissé à 100ms, voir complètement supprimé, en tout cas dans un cas où c'est Garage le backend
quentin commented 2023-11-30 18:19:02 +00:00
Review
Copy link

J'aime bien quand même l'idée de garder un délai, ça nous évitera de DoS un truc si un bot venait à spam d'une manière imprévue tricot. Je préfère que la partie control plane de tricot soit ralentie que on propage la charge en arrière. Meme si je reconnais que c'est quand meme bien primitif comme méthode de controle. Hmm, du coup 100ms ça me parait être un bon compromis.

J'aime bien quand même l'idée de garder un délai, ça nous évitera de DoS un truc si un bot venait à spam d'une manière imprévue tricot. Je préfère que la partie control plane de tricot soit ralentie que on propage la charge en arrière. Meme si je reconnais que c'est quand meme bien primitif comme méthode de controle. Hmm, du coup 100ms ça me parait être un bon compromis.
lx commented 2023-11-30 18:54:34 +00:00
Review
Copy link

Mon hypothèse c'était que même si on se prend un DoS, vu que les requêtes vers le back-end garage sont pas faites en parallèle mais l'une après l'autre, même sans délai ça fera jamais tomber garage en fait, c'est rien du tout par rapport à ce qu'il peut gérer. Du coup la proposition d'enlever le délai ça permettait juste de pas avoir un délai dans le happy path qui serait pas particulièrement utile ^^ Après 100ms de délai c'est vraiment pas grand chose donc ça me va aussi de le laisser

Mon hypothèse c'était que même si on se prend un DoS, vu que les requêtes vers le back-end garage sont pas faites en parallèle mais l'une après l'autre, même sans délai ça fera jamais tomber garage en fait, c'est rien du tout par rapport à ce qu'il peut gérer. Du coup la proposition d'enlever le délai ça permettait juste de pas avoir un délai dans le happy path qui serait pas particulièrement utile ^^ Après 100ms de délai c'est vraiment pas grand chose donc ça me va aussi de le laisser
match self.on_demand_tls_ask(check_url, &candidate).await { match self.on_demand_tls_ask(check_url, &candidate).await {
Ok(()) => { Ok(()) => {
trace!( trace!(
@ -193,7 +208,7 @@ impl CertStore {
check_url check_url
); );
domains.insert(candidate); domains.insert(candidate);
break; continue 'outer;
} }
Err(e) => { Err(e) => {
warn!("domain {} validation refused on glob pattern {} and on check url {} with error: {}", candidate, pattern, check_url, e); warn!("domain {} validation refused on glob pattern {} and on check url {} with error: {}", candidate, pattern, check_url, e);
@ -201,8 +216,6 @@ impl CertStore {
} }
} }
} }
// Avoid DDoSing a backend
tokio::time::sleep(Duration::from_secs(2)).await;
} }
return domains; return domains;
@ -210,17 +223,6 @@ impl CertStore {
/// This function is also in charge of the refresh of the domain names /// This function is also in charge of the refresh of the domain names
fn get_cert_for_https(self: &Arc<Self>, domain: &str) -> Result<Arc<Cert>> { fn get_cert_for_https(self: &Arc<Self>, domain: &str) -> Result<Arc<Cert>> {
// Check if domain is authorized
if !self
.rx_proxy_config
.borrow()
.entries
.iter()
.any(|ent| ent.url_prefix.host.matches(domain))
{
bail!("Domain {} should not have a TLS certificate.", domain);
}
// Check in local memory if it exists // Check in local memory if it exists
if let Some(cert) = self.certs.read().unwrap().get(domain) { if let Some(cert) = self.certs.read().unwrap().get(domain) {
if cert.is_old() { if cert.is_old() {