Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions crates/crates_io_api_types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@ pub struct EncodableCrate {
/// Whether the crate name was an exact match.
#[schema(deprecated)]
pub exact_match: bool,

/// Whether this crate can only be published via Trusted Publishing.
pub trustpub_only: bool,
}

impl EncodableCrate {
Expand All @@ -386,6 +389,7 @@ impl EncodableCrate {
homepage,
documentation,
repository,
trustpub_only,
..
} = krate;
let versions_link = match versions {
Expand Down Expand Up @@ -451,6 +455,7 @@ impl EncodableCrate {
exact_match,
description,
repository,
trustpub_only,
links: EncodableCrateLinks {
version_downloads: format!("/api/v1/crates/{name}/downloads"),
versions: versions_link,
Expand Down Expand Up @@ -1201,6 +1206,7 @@ mod tests {
reverse_dependencies: "".to_string(),
},
exact_match: false,
trustpub_only: false,
};
let json = serde_json::to_string(&crt).unwrap();
assert_some!(json.as_str().find(r#""updated_at":"2017-01-06T14:23:11Z""#));
Expand Down
3 changes: 3 additions & 0 deletions crates/crates_io_database/src/models/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ pub struct Crate {
pub repository: Option<String>,
pub max_upload_size: Option<i32>,
pub max_features: Option<i16>,
pub trustpub_only: bool,
}

/// We literally never want to select `textsearchable_index_col`
Expand All @@ -52,6 +53,7 @@ type AllColumns = (
crates::repository,
crates::max_upload_size,
crates::max_features,
crates::trustpub_only,
);

pub const ALL_COLUMNS: AllColumns = (
Expand All @@ -65,6 +67,7 @@ pub const ALL_COLUMNS: AllColumns = (
crates::repository,
crates::max_upload_size,
crates::max_features,
crates::trustpub_only,
);

pub const MAX_NAME_LENGTH: usize = 64;
Expand Down
2 changes: 2 additions & 0 deletions crates/crates_io_database/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,8 @@ diesel::table! {
///
/// (Automatically generated by Diesel.)
textsearchable_index_col -> Tsvector,
/// When true, this crate can only be published via Trusted Publishing, not with API tokens
trustpub_only -> Bool,
/// The `updated_at` column of the `crates` table.
///
/// Its SQL type is `Timestamptz`.
Expand Down
1 change: 1 addition & 0 deletions crates/crates_io_database_dump/src/dump-db.toml
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ textsearchable_index_col = "private" # This Postgres specific and can be derived
repository = "public"
max_upload_size = "public"
max_features = "public"
trustpub_only = "public"

[crates_categories]
dependencies = ["categories", "crates"]
Expand Down
2 changes: 1 addition & 1 deletion ...o_database_dump/src/snapshots/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ BEGIN ISOLATION LEVEL REPEATABLE READ, READ ONLY;

\copy "categories" ("category", "crates_cnt", "created_at", "description", "id", "path", "slug") TO 'data/categories.csv' WITH CSV HEADER
\copy "crate_downloads" ("crate_id", "downloads") TO 'data/crate_downloads.csv' WITH CSV HEADER
\copy "crates" ("created_at", "description", "documentation", "homepage", "id", "max_features", "max_upload_size", "name", "readme", "repository", "updated_at") TO 'data/crates.csv' WITH CSV HEADER
\copy "crates" ("created_at", "description", "documentation", "homepage", "id", "max_features", "max_upload_size", "name", "readme", "repository", "trustpub_only", "updated_at") TO 'data/crates.csv' WITH CSV HEADER
\copy "keywords" ("crates_cnt", "created_at", "id", "keyword") TO 'data/keywords.csv' WITH CSV HEADER
\copy "metadata" ("total_downloads") TO 'data/metadata.csv' WITH CSV HEADER
\copy "reserved_crate_names" ("name") TO 'data/reserved_crate_names.csv' WITH CSV HEADER
Expand Down
2 changes: 1 addition & 1 deletion ...o_database_dump/src/snapshots/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ BEGIN;

\copy "categories" ("category", "crates_cnt", "created_at", "description", "id", "path", "slug") FROM 'data/categories.csv' WITH CSV HEADER
\copy "crate_downloads" ("crate_id", "downloads") FROM 'data/crate_downloads.csv' WITH CSV HEADER
\copy "crates" ("created_at", "description", "documentation", "homepage", "id", "max_features", "max_upload_size", "name", "readme", "repository", "updated_at") FROM 'data/crates.csv' WITH CSV HEADER
\copy "crates" ("created_at", "description", "documentation", "homepage", "id", "max_features", "max_upload_size", "name", "readme", "repository", "trustpub_only", "updated_at") FROM 'data/crates.csv' WITH CSV HEADER
\copy "keywords" ("crates_cnt", "created_at", "id", "keyword") FROM 'data/keywords.csv' WITH CSV HEADER
\copy "metadata" ("total_downloads") FROM 'data/metadata.csv' WITH CSV HEADER
\copy "reserved_crate_names" ("name") FROM 'data/reserved_crate_names.csv' WITH CSV HEADER
Expand Down
16 changes: 16 additions & 0 deletions crates/crates_io_test_utils/src/builders/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub struct CrateBuilder<'a> {
krate: NewCrate<'a>,
owner_id: i32,
recent_downloads: Option<i32>,
trustpub_only: bool,
updated_at: Option<DateTime<Utc>>,
versions: Vec<VersionBuilder>,
}
Expand All @@ -34,6 +35,7 @@ impl<'a> CrateBuilder<'a> {
},
owner_id,
recent_downloads: None,
trustpub_only: false,
updated_at: None,
versions: Vec::new(),
}
Expand Down Expand Up @@ -113,6 +115,12 @@ impl<'a> CrateBuilder<'a> {
self
}

/// Sets the crate's `trustpub_only` flag.
pub fn trustpub_only(mut self, trustpub_only: bool) -> Self {
self.trustpub_only = trustpub_only;
self
}

pub async fn build(mut self, connection: &mut AsyncPgConnection) -> anyhow::Result<Crate> {
use diesel::{insert_into, select, update};

Expand Down Expand Up @@ -171,6 +179,14 @@ impl<'a> CrateBuilder<'a> {
.await?;
}

if self.trustpub_only {
krate = update(&krate)
.set(crates::trustpub_only.eq(true))
.returning(Crate::as_returning())
.get_result(connection)
.await?;
}

update_default_version(krate.id, connection).await?;

Ok(krate)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE crates DROP COLUMN trustpub_only;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ALTER TABLE crates ADD COLUMN trustpub_only BOOLEAN NOT NULL DEFAULT FALSE;
COMMENT ON COLUMN crates.trustpub_only IS 'When true, this crate can only be published via Trusted Publishing, not with API tokens';
1 change: 1 addition & 0 deletions src/controllers/krate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub mod owners;
pub mod publish;
pub mod rev_deps;
pub mod search;
pub mod update;
pub mod versions;

#[derive(Deserialize, FromRequestParts, IntoParams)]
Expand Down
10 changes: 10 additions & 0 deletions src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,16 @@ pub async fn publish(app: AppState, req: Parts, body: Body) -> AppResult<Json<Go
AuthType::Regular(Box::new(auth))
};

// Check if crate requires trusted publishing
if let Some(existing_crate) = &existing_crate
&& existing_crate.trustpub_only
&& matches!(auth, AuthType::Regular(_))
{
return Err(forbidden(
"You tried to publish with an API token but this crate requires trusted publishing.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think people may be confused by "API token" as a term of art, so how about:

Suggested change
"You tried to publish with an API token but this crate requires trusted publishing.",
"New versions of this crate can only be published using Trusted Publishing.",

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm... I specifically wanted to include the "API token" term to make it more obvious to the user what the problem is. I'm not sure what exactly you mean by "term of art". Do you think that crate owners that try to publish something are not aware of what the term "API token" means? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think that crate owners that try to publish something are not aware of what the term "API token" means?

Honestly, yes, especially in a scenario where this might be an error message they get some time after they configured their crate to require TP. I could see a crate owner — particularly one that's stressed because they're trying to publish a fix for a vulnerability or critical bug — throwing their hands up and wondering how a TP token differs from an API token.

I don't mind including it somewhere in there in a second sentence, but I think we need to frontload the actionable part, which is that Trusted Publishing is required.

));
}

let verified_email_address = if let Some(user) = auth.user() {
let verified_email_address = user.verified_email(&mut conn).await?;
Some(verified_email_address.ok_or_else(|| verified_email_error(&app.config.domain_name))?)
Expand Down
213 changes: 213 additions & 0 deletions src/controllers/krate/update.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
use crate::app::AppState;
use crate::auth::AuthCheck;
use crate::controllers::krate::CratePath;
use crate::email::EmailMessage;
use crate::middleware::real_ip::RealIp;
use crate::models::token::EndpointScope;
use crate::models::{Crate, User};
use crate::schema::*;
use crate::util::errors::{AppResult, crate_not_found, custom, forbidden};
use crate::views::EncodableCrate;
use anyhow::Context;
use axum::{Extension, Json};
use diesel::prelude::*;
use diesel_async::scoped_futures::ScopedFutureExt;
use diesel_async::{AsyncConnection, RunQueryDsl};
use http::{StatusCode, request::Parts};
use serde::{Deserialize, Serialize};
use tracing::{info, warn};

#[derive(Debug, Deserialize, utoipa::ToSchema)]
pub struct PatchRequest {
/// Whether this crate can only be published via Trusted Publishing.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub trustpub_only: Option<bool>,
}

#[derive(Debug, Serialize, utoipa::ToSchema)]
pub struct PatchResponse {
/// The updated crate metadata.
#[serde(rename = "crate")]
krate: EncodableCrate,
}

/// Update crate settings.
#[utoipa::path(
patch,
path = "/api/v1/crates/{name}",
params(CratePath),
request_body = PatchRequest,
security(
("api_token" = []),
("cookie" = []),
),
tag = "crates",
responses((status = 200, description = "Successful Response", body = inline(PatchResponse))),
)]
pub async fn update_crate(
app: AppState,
path: CratePath,
req: Parts,
Extension(real_ip): Extension<RealIp>,
Json(body): Json<PatchRequest>,
) -> AppResult<Json<PatchResponse>> {
let mut conn = app.db_write().await?;

// Check that the crate exists
let krate = path.load_crate(&mut conn).await?;

// Check that the user is authenticated with appropriate permissions
let auth = AuthCheck::default()
.with_endpoint_scope(EndpointScope::TrustedPublishing)
.for_crate(&krate.name)
.check(&req, &mut conn)
.await?;

if auth
.api_token()
.is_some_and(|token| token.endpoint_scopes.is_none())
{
return Err(forbidden(
"This endpoint cannot be used with legacy API tokens. Use a scoped API token instead.",
));
}

// Update crate settings in a transaction
conn.transaction(|conn| {
update_inner(conn, &app, &krate, auth.user(), &real_ip, body).scope_boxed()
})
.await
}

async fn update_inner(
conn: &mut diesel_async::AsyncPgConnection,
app: &AppState,
krate: &Crate,
user: &User,
real_ip: &RealIp,
body: PatchRequest,
) -> AppResult<Json<PatchResponse>> {
// Query user owners to check permissions and send emails
let user_owners = crate_owners::table
.inner_join(users::table)
.inner_join(emails::table.on(users::id.eq(emails::user_id)))
.filter(crate_owners::crate_id.eq(krate.id))
.filter(crate_owners::deleted.eq(false))
.filter(crate_owners::owner_kind.eq(crate::models::OwnerKind::User))
.select((users::id, users::gh_login, emails::email, emails::verified))
.load::<(i32, String, String, bool)>(conn)
.await?;

// Check that the authenticated user is an owner
if !user_owners.iter().any(|(id, _, _, _)| *id == user.id) {
let msg = "only owners have permission to modify crate settings";
return Err(custom(StatusCode::FORBIDDEN, msg));
}

// Update trustpub_only if provided
if let Some(trustpub_only) = body.trustpub_only {
diesel::update(crates::table)
.filter(crates::id.eq(krate.id))
.set(crates::trustpub_only.eq(trustpub_only))
.execute(conn)
.await?;

// Audit log the setting change
info!(
target: "audit",
action = "trustpub_only_change",
krate.name = %krate.name,
network.client.ip = %**real_ip,
usr.id = user.id,
usr.name = %user.gh_login,
"User {} set trustpub_only={trustpub_only} for crate {}",
user.gh_login,
krate.name
);

// Send email notifications to all crate owners
for (_, gh_login, email_address, email_verified) in &user_owners {
if *email_verified {
let email = TrustpubOnlyChangedEmail {
recipient: gh_login,
auth_user: user,
krate,
trustpub_only,
};

if let Err(err) = email.send(app, email_address).await {
warn!("Failed to send trustpub_only notification to {email_address}: {err}");
}
}
}
}
Comment on lines +107 to +143
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker, but I'm wondering if we should check whether the given value is different from the existing value since we are sending a notification?


Nevertheless, I especially love the introduced notification mechanism here. Without it, a malicious user with a leaked API token could change the setting and publish a malicious version, which the crate owner might not notice. But with this mechanism, the crate owner has a chance to rotate the token and make amends!


// Reload the crate to get updated data
let (krate, downloads, recent_downloads, default_version, yanked, num_versions): (
Crate,
i64,
Option<i64>,
Option<String>,
Option<bool>,
Option<i32>,
) = Crate::by_name(&krate.name)
.inner_join(crate_downloads::table)
.left_join(recent_crate_downloads::table)
.left_join(default_versions::table)
.left_join(versions::table.on(default_versions::version_id.eq(versions::id)))
.select((
Crate::as_select(),
crate_downloads::downloads,
recent_crate_downloads::downloads.nullable(),
versions::num.nullable(),
versions::yanked.nullable(),
default_versions::num_versions.nullable(),
))
.first(conn)
.await
.optional()?
.ok_or_else(|| crate_not_found(&krate.name))?;

let encodable_crate = EncodableCrate::from(
krate,
default_version.as_deref(),
num_versions.unwrap_or_default(),
yanked,
None,
None,
None,
None,
false,
downloads,
recent_downloads,
);

Ok(Json(PatchResponse {
krate: encodable_crate,
}))
}

#[derive(Serialize)]
struct TrustpubOnlyChangedEmail<'a> {
/// The GitHub login of the email recipient.
recipient: &'a str,
/// The user who changed the setting.
auth_user: &'a User,
/// The crate for which the setting was changed.
krate: &'a Crate,
/// The new value of the trustpub_only flag.
trustpub_only: bool,
}

impl TrustpubOnlyChangedEmail<'_> {
async fn send(&self, state: &AppState, email_address: &str) -> anyhow::Result<()> {
let email = EmailMessage::from_template("trustpub_only_changed", self);
let email = email.context("Failed to render email template")?;

state
.emails
.send(email_address, email)
.await
.context("Failed to send email")
}
}
1 change: 1 addition & 0 deletions src/controllers/trustpub/emails.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ mod tests {
repository: None,
max_upload_size: None,
max_features: None,
trustpub_only: false,
}
}

Expand Down
Loading