Skip to content

Commit

Permalink
Merge pull request #870 from portier/feat/uncounted
Browse files Browse the repository at this point in the history
Ignore specific emails in metrics
  • Loading branch information
stephank authored Apr 18, 2024
2 parents 527d589 + 3996fdf commit a9ec6b8
Show file tree
Hide file tree
Showing 13 changed files with 72 additions and 20 deletions.
11 changes: 11 additions & 0 deletions config.toml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,17 @@ limits = [
"ip:email:origin:decr_complete:2/15m",
]

################################################################
# Metrics

# The broker exposes a Prometheus-compatible `GET /metrics` endpoint:
# https://prometheus.io/docs/instrumenting/exposition_formats/#text-based-format

# Email addresses that are not counted towards metrics.
# This is useful for ignoring automated tests from monitoring systems.
# Note that only success metrics are skipped. Errors are always counted.
#uncounted_emails = ["[email protected]"]

################################################################
# WebFinger overrides

Expand Down
10 changes: 6 additions & 4 deletions src/agents/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ pub struct FetchUrl {
/// The request to make.
pub request: Request,
/// Latency metric to use.
pub metric: &'static Histogram,
pub metric: Option<&'static Histogram>,
}
impl Message for FetchUrl {
type Reply = Result<FetchUrlResult, FetchError>;
}
impl FetchUrl {
/// Create a simple GET request message.
pub fn get(url: Url, metric: &'static Histogram) -> Self {
pub fn get(url: Url, metric: Option<&'static Histogram>) -> Self {
let request = Request::new(Method::GET, url);
FetchUrl { request, metric }
}
Expand All @@ -63,7 +63,7 @@ impl Agent for FetchAgent {}

impl Handler<FetchUrl> for FetchAgent {
fn handle(&mut self, message: FetchUrl, cx: Context<Self, FetchUrl>) {
let timer = message.metric.start_timer();
let timer = message.metric.map(Histogram::start_timer);
let future = self.client.execute(message.request);
cx.reply_later(async {
let res = future.await?;
Expand All @@ -82,7 +82,9 @@ impl Handler<FetchUrl> for FetchAgent {
return Err(FetchError::BadStatus { status, data });
}

timer.observe_duration();
if let Some(timer) = timer {
timer.observe_duration();
}

Ok(FetchUrlResult { data, max_age })
});
Expand Down
2 changes: 1 addition & 1 deletion src/agents/mailer/mailgun.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl Handler<SendMail> for MailgunMailer {

let future = self.fetcher.send(FetchUrl {
request,
metric: &metrics::AUTH_EMAIL_SEND_DURATION,
metric: Some(&metrics::AUTH_EMAIL_SEND_DURATION),
});
cx.reply_later(async move {
match future.await {
Expand Down
2 changes: 1 addition & 1 deletion src/agents/mailer/postmark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ impl Handler<SendMail> for PostmarkMailer {

let future = self.fetcher.send(FetchUrl {
request,
metric: &metrics::AUTH_EMAIL_SEND_DURATION,
metric: Some(&metrics::AUTH_EMAIL_SEND_DURATION),
});
cx.reply_later(async move {
let data = match future.await {
Expand Down
2 changes: 1 addition & 1 deletion src/agents/mailer/sendgrid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ impl Handler<SendMail> for SendgridMailer {

let future = self.fetcher.send(FetchUrl {
request,
metric: &metrics::AUTH_EMAIL_SEND_DURATION,
metric: Some(&metrics::AUTH_EMAIL_SEND_DURATION),
});
cx.reply_later(async move {
match future.await {
Expand Down
2 changes: 1 addition & 1 deletion src/agents/store/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ pub struct FetchUrlCached {
/// The URL to fetch.
pub url: Url,
/// Latency metric to use on cache miss.
pub metric: &'static Histogram,
pub metric: Option<&'static Histogram>,
}
impl Message for FetchUrlCached {
type Reply = Result<String, BoxError>;
Expand Down
10 changes: 8 additions & 2 deletions src/bridges/email.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ pub struct EmailBridgeData {
/// A form is rendered as an alternative way to confirm, without following the link. Submitting the
/// form results in the same callback as the email link.
pub async fn auth(ctx: &mut Context, email_addr: EmailAddress) -> HandlerResult {
metrics::AUTH_EMAIL_REQUESTS.inc();
if !ctx.app.uncounted_emails.contains(&email_addr) {
metrics::AUTH_EMAIL_REQUESTS.inc();
}

// Generate a 12-character one-time pad.
let code = random_zbase32(12, &ctx.app.rng).await;
Expand Down Expand Up @@ -144,6 +146,10 @@ pub async fn confirmation(ctx: &mut Context) -> HandlerResult {
return Err(BrokerError::ProviderInput("incorrect code".to_owned()));
}

metrics::AUTH_EMAIL_COMPLETED.inc();
let data = &ctx.session_data.as_ref().expect("session vanished");
if !ctx.app.uncounted_emails.contains(&data.email_addr) {
metrics::AUTH_EMAIL_COMPLETED.inc();
}

complete_auth(ctx).await
}
28 changes: 21 additions & 7 deletions src/bridges/oidc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,8 @@ pub async fn auth(
link: &Link,
prompt: &str,
) -> HandlerResult {
let is_counted = !ctx.app.uncounted_emails.contains(email_addr);

// Generate a nonce for the provider.
let provider_nonce = crypto::nonce(&ctx.app.rng).await;

Expand All @@ -98,7 +100,9 @@ pub async fn auth(
})?;
let mut bridge_data = match link.rel {
Relation::Portier => {
metrics::AUTH_OIDC_REQUESTS_PORTIER.inc();
if is_counted {
metrics::AUTH_OIDC_REQUESTS_PORTIER.inc();
}
#[cfg(not(feature = "insecure"))]
{
if link.href.scheme() != "https" {
Expand All @@ -118,7 +122,9 @@ pub async fn auth(
}
// Delegate to the OpenID Connect bridge for Google, if configured.
Relation::Google => {
metrics::AUTH_OIDC_REQUESTS_GOOGLE.inc();
if is_counted {
metrics::AUTH_OIDC_REQUESTS_GOOGLE.inc();
}
let client_id = ctx
.app
.google_client_id
Expand Down Expand Up @@ -149,7 +155,7 @@ pub async fn auth(
..
},
key_set,
) = fetch_config(ctx, &bridge_data).await?;
) = fetch_config(ctx, &bridge_data, is_counted).await?;

{
// Create the URL to redirect to.
Expand Down Expand Up @@ -234,6 +240,11 @@ pub async fn callback(ctx: &mut Context) -> HandlerResult {
return Err(BrokerError::ProviderInput("invalid session".to_owned()));
};

let is_counted = {
let data = ctx.session_data.as_ref().expect("session vanished");
!ctx.app.uncounted_emails.contains(&data.email_addr)
};

// Handle errors.
match params.get("error").map(String::as_str).unwrap_or_default() {
"" => {}
Expand All @@ -254,7 +265,7 @@ pub async fn callback(ctx: &mut Context) -> HandlerResult {
let id_token = try_get_provider_param!(params, "id_token");

// Retrieve the provider's configuration.
let (_, key_set) = fetch_config(ctx, &bridge_data).await?;
let (_, key_set) = fetch_config(ctx, &bridge_data, is_counted).await?;

// Verify the signature.
let jwt_payload = crypto::verify_jws(&id_token, &key_set.keys, bridge_data.signing_alg)
Expand Down Expand Up @@ -313,14 +324,17 @@ pub async fn callback(ctx: &mut Context) -> HandlerResult {
}

// Everything is okay. Build a new identity token and send it to the relying party.
metrics::AUTH_OIDC_COMPLETED.inc();
if is_counted {
metrics::AUTH_OIDC_COMPLETED.inc();
}
complete_auth(ctx).await
}

// Retrieve and verify the provider's configuration.
async fn fetch_config(
ctx: &mut Context,
bridge_data: &OidcBridgeData,
is_counted: bool,
) -> Result<(ProviderConfig, ProviderKeys), BrokerError> {
let config_url = format!("{}/.well-known/openid-configuration", bridge_data.origin)
.parse()
Expand All @@ -331,7 +345,7 @@ async fn fetch_config(
.store
.send(FetchUrlCached {
url: config_url,
metric: &metrics::AUTH_OIDC_FETCH_CONFIG_DURATION,
metric: is_counted.then_some(&metrics::AUTH_OIDC_FETCH_CONFIG_DURATION),
})
.await
.map_err(|e| {
Expand Down Expand Up @@ -369,7 +383,7 @@ async fn fetch_config(
.store
.send(FetchUrlCached {
url: provider_config.jwks_uri.clone(),
metric: &metrics::AUTH_OIDC_FETCH_JWKS_DURATION,
metric: is_counted.then_some(&metrics::AUTH_OIDC_FETCH_JWKS_DURATION),
})
.await
.map_err(|e| {
Expand Down
6 changes: 6 additions & 0 deletions src/config/env.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::{ConfigBuilder, LegacyLimitPerEmail, LimitConfig};
use crate::config::StringList;
use crate::crypto::SigningAlgorithm;
use crate::email_address::EmailAddress;
use ipnetwork::IpNetwork;
use serde::Deserialize;
use std::borrow::ToOwned;
Expand Down Expand Up @@ -70,6 +71,8 @@ pub struct EnvConfig {
limit_per_email: Option<LegacyLimitPerEmail>,

google_client_id: Option<String>,
#[serde(default)]
uncounted_emails: Vec<EmailAddress>,

// Deprecated
ip: Option<String>,
Expand Down Expand Up @@ -266,5 +269,8 @@ impl EnvConfig {
if let Some(val) = parsed.google_client_id {
builder.google_client_id = Some(val);
}
for email in parsed.uncounted_emails {
builder.uncounted_emails.insert(email);
}
}
}
6 changes: 5 additions & 1 deletion src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use url::Url;

use std::{
borrow::ToOwned,
collections::HashMap,
collections::{HashMap, HashSet},
env::var as env_var,
io::Error as IoError,
path::{Path, PathBuf},
Expand Down Expand Up @@ -85,6 +85,7 @@ pub struct Config {

pub google_client_id: Option<String>,
pub domain_overrides: HashMap<String, Vec<Link>>,
pub uncounted_emails: HashSet<EmailAddress>,

pub static_resolver: Resolver,
pub templates: Templates,
Expand Down Expand Up @@ -421,6 +422,7 @@ pub struct ConfigBuilder {

pub google_client_id: Option<String>,
pub domain_overrides: HashMap<String, Vec<Link>>,
pub uncounted_emails: HashSet<EmailAddress>,
}

impl ConfigBuilder {
Expand Down Expand Up @@ -494,6 +496,7 @@ impl ConfigBuilder {

google_client_id: None,
domain_overrides: HashMap::new(),
uncounted_emails: HashSet::new(),
}
}

Expand Down Expand Up @@ -661,6 +664,7 @@ impl ConfigBuilder {

google_client_id: self.google_client_id,
domain_overrides,
uncounted_emails: self.uncounted_emails,

static_resolver: Resolver::new(res_dir),
templates,
Expand Down
6 changes: 6 additions & 0 deletions src/config/toml.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::{ConfigBuilder, LegacyLimitPerEmail, LimitConfig};
use crate::config::StringList;
use crate::crypto::SigningAlgorithm;
use crate::email_address::EmailAddress;
use crate::webfinger::Link;
use ipnetwork::IpNetwork;
use serde::Deserialize;
Expand Down Expand Up @@ -68,6 +69,8 @@ pub struct TomlConfig {

google_client_id: Option<String>,
domain_overrides: Option<HashMap<String, Vec<Link>>>,
#[serde(default)]
uncounted_emails: Vec<EmailAddress>,

// Deprecated.
server: Option<TomlServerTable>,
Expand Down Expand Up @@ -401,5 +404,8 @@ impl TomlConfig {
builder.domain_overrides.insert(domain, links);
}
}
for email in parsed.uncounted_emails {
builder.uncounted_emails.insert(email);
}
}
}
4 changes: 3 additions & 1 deletion src/handlers/auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,9 @@ pub async fn auth(ctx: &mut Context) -> HandlerResult {
}

// At this point, we've done all the local input verification.
metrics::AUTH_REQUESTS.inc();
if !ctx.app.uncounted_emails.contains(&email_addr) {
metrics::AUTH_REQUESTS.inc();
}

// Verify the email domain.
if let Err(err) = ctx.app.domain_validator.validate(email_addr.domain()).await {
Expand Down
3 changes: 2 additions & 1 deletion src/webfinger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -119,11 +119,12 @@ pub async fn query(app: &ConfigRc, email_addr: &EmailAddress) -> Result<Vec<Link
.map_err(|e| BrokerError::Internal(format!("could not build webfinger query url: {e}")))?;

// Make the request.
let is_counted = !app.uncounted_emails.contains(email_addr);
let descriptor = app
.store
.send(FetchUrlCached {
url,
metric: &metrics::AUTH_WEBFINGER_DURATION,
metric: is_counted.then_some(&metrics::AUTH_WEBFINGER_DURATION),
})
.await
.map_err(|e| BrokerError::Provider(format!("webfinger request failed: {e}")))?;
Expand Down

0 comments on commit a9ec6b8

Please sign in to comment.