From 0f02c8d9298234151aa78845670c1d1d116f7049 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 13 Dec 2022 12:15:43 +0100 Subject: [PATCH 1/7] github/secret_scanning: Improve `is_cache_valid()` readability The comparison is essentially the same, but slightly more straight forward to understand. --- src/controllers/github/secret_scanning.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/controllers/github/secret_scanning.rs b/src/controllers/github/secret_scanning.rs index 1423be511cd..8308ff203a7 100644 --- a/src/controllers/github/secret_scanning.rs +++ b/src/controllers/github/secret_scanning.rs @@ -66,8 +66,8 @@ fn key_from_spki(key: &GitHubPublicKey) -> Result, std::io::Error> { /// Check if cache of public keys is populated and not expired fn is_cache_valid(timestamp: Option>) -> bool { timestamp.is_some() - && chrono::Utc::now() - timestamp.unwrap() - < chrono::Duration::seconds(PUBLIC_KEY_CACHE_LIFETIME_SECONDS) + && chrono::Utc::now() + < timestamp.unwrap() + chrono::Duration::seconds(PUBLIC_KEY_CACHE_LIFETIME_SECONDS) } // Fetches list of public keys from GitHub API From 23595d3159b94fa739bb74201c188586757a6ea7 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 13 Dec 2022 12:16:50 +0100 Subject: [PATCH 2/7] github/secret_scanning: Prevent potential panic in `get_public_keys()` function We shouldn't panic if the network call fails, instead we can forward the error and return a 500 response. --- src/controllers/github/secret_scanning.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/controllers/github/secret_scanning.rs b/src/controllers/github/secret_scanning.rs index 8308ff203a7..a869a40368b 100644 --- a/src/controllers/github/secret_scanning.rs +++ b/src/controllers/github/secret_scanning.rs @@ -82,8 +82,7 @@ fn get_public_keys(req: &dyn RequestExt) -> Result, Box Date: Tue, 13 Dec 2022 12:19:24 +0100 Subject: [PATCH 3/7] github/secret_scanning: Reduce nesting in `verify_github_signature()` function The logic is essentially the same, but using `find()` and `let-else` to simplify the code. --- src/controllers/github/secret_scanning.rs | 47 +++++++++++------------ 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/src/controllers/github/secret_scanning.rs b/src/controllers/github/secret_scanning.rs index a869a40368b..e130fdd17db 100644 --- a/src/controllers/github/secret_scanning.rs +++ b/src/controllers/github/secret_scanning.rs @@ -109,32 +109,31 @@ fn verify_github_signature(req: &dyn RequestExt, json: &[u8]) -> Result<(), Box< let public_keys = get_public_keys(req) .map_err(|e| bad_request(&format!("failed to fetch GitHub public keys: {e:?}")))?; - for key in public_keys { - if key.key_identifier == req_key_id { - if !key.is_current { - return Err(bad_request(&format!( - "key id {req_key_id} is not a current key" - ))); - } - let key_bytes = - key_from_spki(&key).map_err(|_| bad_request("cannot parse public key"))?; - let gh_key = - signature::UnparsedPublicKey::new(&signature::ECDSA_P256_SHA256_ASN1, &key_bytes); - - return match gh_key.verify(json, &sig) { - Ok(v) => { - info!( - "GitHub secret alert request validated with key id {}", - key.key_identifier - ); - Ok(v) - } - Err(e) => Err(bad_request(&format!("invalid signature: {e:?}"))), - }; - } + let key = public_keys + .iter() + .find(|key| key.key_identifier == req_key_id); + + let Some(key) = key else { + return Err(bad_request(&format!("unknown key id {req_key_id}"))); + }; + + if !key.is_current { + let error = bad_request(&format!("key id {req_key_id} is not a current key")); + return Err(error); } - return Err(bad_request(&format!("unknown key id {req_key_id}"))); + let key_bytes = key_from_spki(key).map_err(|_| bad_request("cannot parse public key"))?; + let gh_key = signature::UnparsedPublicKey::new(&signature::ECDSA_P256_SHA256_ASN1, &key_bytes); + + gh_key + .verify(json, &sig) + .map_err(|e| bad_request(&format!("invalid signature: {e:?}")))?; + + debug!( + key_id = %key.key_identifier, + "GitHub secret alert request validated", + ); + Ok(()) } #[derive(Deserialize, Serialize)] From 81536a1d98d37b3859a14c051e7541fed1b0181c Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 13 Dec 2022 12:20:51 +0100 Subject: [PATCH 4/7] github/secret_scanning: Remove redundant string conversion before JSON parsing We can use `from_slice()` instead to simplify the code --- src/controllers/github/secret_scanning.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/controllers/github/secret_scanning.rs b/src/controllers/github/secret_scanning.rs index e130fdd17db..8b501b797ba 100644 --- a/src/controllers/github/secret_scanning.rs +++ b/src/controllers/github/secret_scanning.rs @@ -205,9 +205,7 @@ pub fn verify(req: &mut dyn RequestExt) -> EndpointResult { verify_github_signature(req, &json) .map_err(|e| bad_request(&format!("failed to verify request signature: {e:?}")))?; - let json = String::from_utf8(json) - .map_err(|e| bad_request(&format!("failed to decode request body: {e:?}")))?; - let alerts: Vec = json::from_str(&json) + let alerts: Vec = json::from_slice(&json) .map_err(|e| bad_request(&format!("invalid secret alert request: {e:?}")))?; let feedback: Vec = alerts From cfc3f1126d6c67ca1601f6db9e9e07ca32323cd9 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 13 Dec 2022 12:21:17 +0100 Subject: [PATCH 5/7] tests/github: Simplify `public_keys()` implementation --- src/tests/util/github.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/src/tests/util/github.rs b/src/tests/util/github.rs index 4dd351a675e..a3d7f3b7364 100644 --- a/src/tests/util/github.rs +++ b/src/tests/util/github.rs @@ -170,7 +170,7 @@ impl GitHubClient for MockGitHubClient { } fn public_keys(&self, _username: &str, _password: &str) -> AppResult> { - Ok(self.data.public_keys.iter().collect()) + Ok(self.data.public_keys.iter().map(Into::into).collect()) } } @@ -215,13 +215,3 @@ impl From<&'static MockPublicKey> for GitHubPublicKey { } } } - -impl FromIterator<&'static MockPublicKey> for Vec { - fn from_iter>(iter: I) -> Self { - let mut c = Vec::new(); - for k in iter { - c.push(k.into()); - } - c - } -} From 26cbca3976fbacfcaabd9460fa916c1be3903f7f Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 13 Dec 2022 12:22:02 +0100 Subject: [PATCH 6/7] github/secret_scanning: Extract `GitHubSecretAlertFeedbackLabel` enum --- src/controllers/github/secret_scanning.rs | 15 ++++++++++++--- src/tests/github_secret_scanning.rs | 14 +++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/controllers/github/secret_scanning.rs b/src/controllers/github/secret_scanning.rs index 8b501b797ba..b166c8178a7 100644 --- a/src/controllers/github/secret_scanning.rs +++ b/src/controllers/github/secret_scanning.rs @@ -186,11 +186,20 @@ fn alert_revoke_token( pub struct GitHubSecretAlertFeedback { pub token_raw: String, pub token_type: String, - pub label: String, + pub label: GitHubSecretAlertFeedbackLabel, +} + +#[derive(Copy, Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] +#[serde(rename_all = "snake_case")] +pub enum GitHubSecretAlertFeedbackLabel { + TruePositive, + FalsePositive, } /// Handles the `POST /api/github/secret-scanning/verify` route. pub fn verify(req: &mut dyn RequestExt) -> EndpointResult { + use GitHubSecretAlertFeedbackLabel::*; + let max_size = 8192; let length = req .content_length() @@ -214,13 +223,13 @@ pub fn verify(req: &mut dyn RequestExt) -> EndpointResult { token_raw: alert.token.clone(), token_type: alert.r#type.clone(), label: match alert_revoke_token(req, &alert) { - Ok(()) => "true_positive".to_string(), + Ok(()) => TruePositive, Err(e) => { warn!( "Error revoking API token in GitHub secret alert: {} ({e:?})", alert.token ); - "false_positive".to_string() + FalsePositive } }, }) diff --git a/src/tests/github_secret_scanning.rs b/src/tests/github_secret_scanning.rs index e9cf7f6c582..9624f00b805 100644 --- a/src/tests/github_secret_scanning.rs +++ b/src/tests/github_secret_scanning.rs @@ -1,5 +1,7 @@ use crate::{RequestHelper, TestApp}; -use cargo_registry::controllers::github::secret_scanning::GitHubSecretAlertFeedback; +use cargo_registry::controllers::github::secret_scanning::{ + GitHubSecretAlertFeedback, GitHubSecretAlertFeedbackLabel, +}; use cargo_registry::{models::ApiToken, schema::api_tokens}; use conduit::StatusCode; use diesel::prelude::*; @@ -49,7 +51,10 @@ fn github_secret_alert_revokes_token() { assert_eq!(feedback.len(), 1); assert_eq!(feedback[0].token_raw, "some_token"); assert_eq!(feedback[0].token_type, "some_type"); - assert_eq!(feedback[0].label, "true_positive"); + assert_eq!( + feedback[0].label, + GitHubSecretAlertFeedbackLabel::TruePositive + ); // Ensure that the token was revoked app.db(|conn| { @@ -95,7 +100,10 @@ fn github_secret_alert_for_unknown_token() { assert_eq!(feedback.len(), 1); assert_eq!(feedback[0].token_raw, "some_token"); assert_eq!(feedback[0].token_type, "some_type"); - assert_eq!(feedback[0].label, "false_positive"); + assert_eq!( + feedback[0].label, + GitHubSecretAlertFeedbackLabel::FalsePositive + ); // Ensure that the token was not revoked app.db(|conn| { From 27c3e798118cdc254436854a30029788c10edbf2 Mon Sep 17 00:00:00 2001 From: Tobias Bieniek Date: Tue, 13 Dec 2022 12:23:18 +0100 Subject: [PATCH 7/7] github/secret_scanning: Treat missing token as `Ok` If GitHub reports a potential token we should treat false positives as legitimate cases instead of errors. Only real errors like failing to revoke a token should be considered errors. --- src/controllers/github/secret_scanning.rs | 68 ++++++++++++----------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/src/controllers/github/secret_scanning.rs b/src/controllers/github/secret_scanning.rs index b166c8178a7..b16c0872341 100644 --- a/src/controllers/github/secret_scanning.rs +++ b/src/controllers/github/secret_scanning.rs @@ -148,38 +148,50 @@ struct GitHubSecretAlert { fn alert_revoke_token( req: &dyn RequestExt, alert: &GitHubSecretAlert, -) -> Result<(), Box> { +) -> Result> { let conn = req.db_write()?; // not using ApiToken::find_by_api_token in order to preserve last_used_at // the token field has a uniqueness constraint so get_result() should be safe to use - let token: ApiToken = diesel::update(api_tokens::table) + let token = diesel::update(api_tokens::table) .filter(api_tokens::token.eq(alert.token.as_bytes())) .set(api_tokens::revoked.eq(true)) - .get_result::(&*conn)?; + .get_result::(&*conn) + .optional()?; + + let Some(token) = token else { + return Ok(GitHubSecretAlertFeedbackLabel::FalsePositive); + }; // send email notification to the token owner let user = User::find(&conn, token.user_id)?; - info!( + warn!( "Revoked API token '{}' for user {} ({})", alert.token, user.gh_login, user.id ); - match user.email(&conn)? { - None => { - info!( - "No email address for user {} ({}), cannot send email notification", - user.gh_login, user.id - ); - Ok(()) - } - Some(email) => req.app().emails.send_token_exposed_notification( + + if let Some(email) = user.email(&conn)? { + let result = req.app().emails.send_token_exposed_notification( &email, &alert.url, "GitHub", &alert.source, &token.name, - ), - } + ); + if let Err(error) = result { + warn!( + gh_login = %user.gh_login, user_id = %user.id, ?error, + "Failed to send email notification", + ); + } + } else { + warn!( + gh_login = %user.gh_login, user_id = %user.id, + "Failed to send email notification: No address found", + ); + }; + + Ok(GitHubSecretAlertFeedbackLabel::TruePositive) } #[derive(Deserialize, Serialize)] @@ -198,8 +210,6 @@ pub enum GitHubSecretAlertFeedbackLabel { /// Handles the `POST /api/github/secret-scanning/verify` route. pub fn verify(req: &mut dyn RequestExt) -> EndpointResult { - use GitHubSecretAlertFeedbackLabel::*; - let max_size = 8192; let length = req .content_length() @@ -217,23 +227,17 @@ pub fn verify(req: &mut dyn RequestExt) -> EndpointResult { let alerts: Vec = json::from_slice(&json) .map_err(|e| bad_request(&format!("invalid secret alert request: {e:?}")))?; - let feedback: Vec = alerts + let feedback = alerts .into_iter() - .map(|alert| GitHubSecretAlertFeedback { - token_raw: alert.token.clone(), - token_type: alert.r#type.clone(), - label: match alert_revoke_token(req, &alert) { - Ok(()) => TruePositive, - Err(e) => { - warn!( - "Error revoking API token in GitHub secret alert: {} ({e:?})", - alert.token - ); - FalsePositive - } - }, + .map(|alert| { + let label = alert_revoke_token(req, &alert)?; + Ok(GitHubSecretAlertFeedback { + token_raw: alert.token, + token_type: alert.r#type, + label, + }) }) - .collect(); + .collect::, Box>>()?; Ok(req.json(&feedback)) }