Skip to content

Use encrypted GitHub OAuth access tokens #11697

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.

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
wants to merge 3 commits into
base: main
Choose a base branch
from
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
4 changes: 2 additions & 2 deletions crates/crates_io_database/src/models/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub struct User {
#[serde(skip)]
pub gh_access_token: SecretString,
#[serde(skip)]
pub gh_encrypted_token: Option<Vec<u8>>,
pub gh_encrypted_token: Vec<u8>,
pub account_lock_reason: Option<String>,
pub account_lock_until: Option<DateTime<Utc>>,
pub is_admin: bool,
Expand Down Expand Up @@ -96,7 +96,7 @@ pub struct NewUser<'a> {
pub name: Option<&'a str>,
pub gh_avatar: Option<&'a str>,
pub gh_access_token: &'a str,
pub gh_encrypted_token: Option<&'a [u8]>,
pub gh_encrypted_token: &'a [u8],
}

impl NewUser<'_> {
Expand Down
2 changes: 1 addition & 1 deletion crates/crates_io_database/src/schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,7 @@ diesel::table! {
/// Whether or not the user wants to receive notifications when a package they own is published
publish_notifications -> Bool,
/// Encrypted GitHub access token
gh_encrypted_token -> Nullable<Bytea>,
gh_encrypted_token -> Bytea,
}
}

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 @@ -235,6 +235,7 @@ publish_notifications = "private"
gh_encrypted_token = "private"
[users.column_defaults]
gh_access_token = "''"
gh_encrypted_token = "''"

[version_downloads]
dependencies = ["versions"]
Expand Down
2 changes: 2 additions & 0 deletions ...o_database_dump/src/snapshots/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ BEGIN;
-- Set defaults for non-nullable columns not included in the dump.

ALTER TABLE "users" ALTER COLUMN "gh_access_token" SET DEFAULT '';
ALTER TABLE "users" ALTER COLUMN "gh_encrypted_token" SET DEFAULT '';

-- Truncate all tables.

Expand Down Expand Up @@ -67,6 +68,7 @@ BEGIN;
-- Drop the defaults again.

ALTER TABLE "users" ALTER COLUMN "gh_access_token" DROP DEFAULT;
ALTER TABLE "users" ALTER COLUMN "gh_encrypted_token" DROP DEFAULT;

-- Reenable triggers on each table.

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE users ALTER COLUMN gh_encrypted_token DROP NOT NULL;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ALTER TABLE users ALTER COLUMN gh_encrypted_token SET NOT NULL;
108 changes: 0 additions & 108 deletions src/bin/crates-admin/encrypt_github_tokens.rs

This file was deleted.

3 changes: 0 additions & 3 deletions src/bin/crates-admin/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ mod default_versions;
mod delete_crate;
mod delete_version;
mod dialoguer;
mod encrypt_github_tokens;
mod enqueue_job;
mod migrate;
mod populate;
Expand All @@ -22,7 +21,6 @@ enum Command {
BackfillOgImages(backfill_og_images::Opts),
DeleteCrate(delete_crate::Opts),
DeleteVersion(delete_version::Opts),
EncryptGithubTokens(encrypt_github_tokens::Opts),
Populate(populate::Opts),
RenderReadmes(render_readmes::Opts),
TransferCrates(transfer_crates::Opts),
Expand Down Expand Up @@ -53,7 +51,6 @@ async fn main() -> anyhow::Result<()> {
Command::BackfillOgImages(opts) => backfill_og_images::run(opts).await,
Command::DeleteCrate(opts) => delete_crate::run(opts).await,
Command::DeleteVersion(opts) => delete_version::run(opts).await,
Command::EncryptGithubTokens(opts) => encrypt_github_tokens::run(opts).await,
Command::Populate(opts) => populate::run(opts).await,
Command::RenderReadmes(opts) => render_readmes::run(opts).await,
Command::TransferCrates(opts) => transfer_crates::run(opts).await,
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/crate_owner_invitation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ async fn prepare_list(
// Only allow crate owners to query pending invitations for their crate.
let krate: Crate = Crate::by_name(&crate_name).first(conn).await?;
let owners = krate.owners(conn).await?;
if Rights::get(user, &*state.github, &owners).await? != Rights::Full {
let encryption = &state.config.gh_token_encryption;
if Rights::get(user, &*state.github, &owners, encryption).await? != Rights::Full {
let detail = "only crate owners can query pending invitations for their crate";
return Err(forbidden(detail));
}
Expand Down
8 changes: 5 additions & 3 deletions src/controllers/helpers/authorization.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use crate::models::{Owner, User};
use crate::util::gh_token_encryption::GitHubTokenEncryption;
use crates_io_github::{GitHubClient, GitHubError};
use oauth2::AccessToken;
use secrecy::ExposeSecret;

/// Access rights to the crate (publishing and ownership management)
/// NOTE: The order of these variants matters!
Expand All @@ -25,8 +24,11 @@ impl Rights {
user: &User,
gh_client: &dyn GitHubClient,
owners: &[Owner],
encryption: &GitHubTokenEncryption,
) -> Result<Self, GitHubError> {
let token = AccessToken::new(user.gh_access_token.expose_secret().to_string());
let token = encryption
.decrypt(&user.gh_encrypted_token)
.map_err(GitHubError::Other)?;

let mut best = Self::None;
for owner in owners {
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/krate/delete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ pub async fn delete_crate(
// Check that the user is an owner of the crate (team owners are not allowed to delete crates)
let user = auth.user();
let owners = krate.owners(&mut conn).await?;
match Rights::get(user, &*app.github, &owners).await? {
match Rights::get(user, &*app.github, &owners, &app.config.gh_token_encryption).await? {
Rights::Full => {}
Rights::Publish => {
let msg = "team members don't have permission to delete crates";
Expand Down
30 changes: 24 additions & 6 deletions src/controllers/krate/owners.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use crate::models::{
krate::NewOwnerInvite, token::EndpointScope,
};
use crate::util::errors::{AppResult, BoxedAppError, bad_request, crate_not_found, custom};
use crate::util::gh_token_encryption::GitHubTokenEncryption;
use crate::views::EncodableOwner;
use crate::{App, app::AppState};
use crate::{auth::AuthCheck, email::EmailMessage};
Expand Down Expand Up @@ -199,7 +200,7 @@ async fn modify_owners(

let owners = krate.owners(conn).await?;

match Rights::get(user, &*app.github, &owners).await? {
match Rights::get(user, &*app.github, &owners, &app.config.gh_token_encryption).await? {
Rights::Full => {}
// Yes!
Rights::Publish => {
Expand Down Expand Up @@ -320,7 +321,8 @@ async fn add_owner(
login: &str,
) -> Result<NewOwnerInvite, OwnerAddError> {
if login.contains(':') {
add_team_owner(&*app.github, conn, req_user, krate, login).await
let encryption = &app.config.gh_token_encryption;
add_team_owner(&*app.github, conn, req_user, krate, login, encryption).await
} else {
invite_user_owner(app, conn, req_user, krate, login).await
}
Expand Down Expand Up @@ -363,6 +365,7 @@ async fn add_team_owner(
req_user: &User,
krate: &Crate,
login: &str,
encryption: &GitHubTokenEncryption,
) -> Result<NewOwnerInvite, OwnerAddError> {
// github:rust-lang:owners
let mut chunks = login.split(':');
Expand All @@ -381,9 +384,16 @@ async fn add_team_owner(
})?;

// Always recreate teams to get the most up-to-date GitHub ID
let team =
create_or_update_github_team(gh_client, conn, &login.to_lowercase(), org, team, req_user)
.await?;
let team = create_or_update_github_team(
gh_client,
conn,
&login.to_lowercase(),
org,
team,
req_user,
encryption,
)
.await?;

// Teams are added as owners immediately, since the above call ensures
// the user is a team member.
Expand All @@ -408,6 +418,7 @@ pub async fn create_or_update_github_team(
org_name: &str,
team_name: &str,
req_user: &User,
encryption: &GitHubTokenEncryption,
) -> AppResult<Team> {
// GET orgs/:org/teams
// check that `team` is the `slug` in results, and grab its data
Expand All @@ -424,7 +435,14 @@ pub async fn create_or_update_github_team(
)));
}

let token = AccessToken::new(req_user.gh_access_token.expose_secret().to_string());
let token = encryption
.decrypt(&req_user.gh_encrypted_token)
.map_err(|err| {
custom(
StatusCode::INTERNAL_SERVER_ERROR,
format!("Failed to decrypt GitHub token: {err}"),
)
})?;
let team = gh_client.team_by_name(org_name, team_name, &token).await
.map_err(|_| {
bad_request(format_args!(
Expand Down
2 changes: 1 addition & 1 deletion src/controllers/krate/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,7 @@ pub async fn publish(app: AppState, req: Parts, body: Body) -> AppResult<Json<Go
};

let owners = krate.owners(conn).await?;
if Rights::get(user, &*app.github, &owners).await? < Rights::Publish {
if Rights::get(user, &*app.github, &owners, &app.config.gh_token_encryption).await? < Rights::Publish {
return Err(custom(StatusCode::FORBIDDEN, MISSING_RIGHTS_ERROR_MESSAGE));
}

Expand Down
15 changes: 10 additions & 5 deletions src/controllers/trustpub/github_configs/create/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::auth::AuthCheck;
use crate::controllers::krate::load_crate;
use crate::controllers::trustpub::github_configs::json;
use crate::email::EmailMessage;
use crate::util::errors::{AppResult, bad_request, forbidden};
use crate::util::errors::{AppResult, bad_request, forbidden, server_error};
use anyhow::Context;
use axum::Json;
use crates_io_database::models::OwnerKind;
Expand All @@ -17,8 +17,6 @@ use diesel::prelude::*;
use diesel_async::RunQueryDsl;
use http::request::Parts;
use minijinja::context;
use oauth2::AccessToken;
use secrecy::ExposeSecret;
use tracing::warn;

#[cfg(test)]
Expand Down Expand Up @@ -77,8 +75,15 @@ pub async fn create_trustpub_github_config(
// Lookup `repository_owner_id` via GitHub API

let owner = &json_config.repository_owner;
let gh_auth = &auth_user.gh_access_token;
let gh_auth = AccessToken::new(gh_auth.expose_secret().to_string());

let encryption = &state.config.gh_token_encryption;
let gh_auth = &auth_user.gh_encrypted_token;
let gh_auth = encryption.decrypt(gh_auth).map_err(|err| {
let login = &auth_user.gh_login;
warn!("Failed to decrypt GitHub token for user {login}: {err}");
server_error("Internal server error")
})?;

let github_user = match state.github.get_user(owner, &gh_auth).await {
Ok(user) => user,
Err(GitHubError::NotFound(_)) => Err(bad_request("Unknown GitHub user or organization"))?,
Expand Down
4 changes: 3 additions & 1 deletion src/controllers/version/docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@ pub async fn rebuild_version_docs(
// Check that the user is an owner of the crate, or a team member (= publish rights)
let user = auth.user();
let owners = krate.owners(&mut conn).await?;
if Rights::get(user, &*app.github, &owners).await? < Rights::Publish {
let encryption = &app.config.gh_token_encryption;
if Rights::get(user, &*app.github, &owners, encryption).await? < Rights::Publish {
return Err(custom(
StatusCode::FORBIDDEN,
"user doesn't have permission to trigger a docs rebuild",
Expand Down Expand Up @@ -107,6 +108,7 @@ mod tests {
.gh_id(111)
.gh_login("other_user")
.gh_access_token("token")
.gh_encrypted_token(&[])
.build()
.insert(&mut conn)
.await?;
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/version/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ pub async fn perform_version_yank_update(

let yanked = yanked.unwrap_or(version.yanked);

if Rights::get(user, &*state.github, &owners).await? < Rights::Publish {
let encryption = &state.config.gh_token_encryption;
if Rights::get(user, &*state.github, &owners, encryption).await? < Rights::Publish {
if user.is_admin {
let action = if yanked { "yanking" } else { "unyanking" };
warn!(
Expand Down
1 change: 1 addition & 0 deletions src/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ mod tests {
users::gh_login.eq("user1"),
users::gh_id.eq(42),
users::gh_access_token.eq("some random token"),
users::gh_encrypted_token.eq(&[]),
))
.returning(users::id)
.get_result::<i32>(&mut conn)
Expand Down
1 change: 1 addition & 0 deletions src/rate_limiter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,7 @@ mod tests {
.gh_id(0)
.gh_login(gh_login)
.gh_access_token("some random token")
.gh_encrypted_token(&[])
.build()
.insert(conn)
.await
Expand Down
Loading