Generate API tokens with a secure RNG, store hashed

migrations/2020-07-13-125308_remove_api_token_default/down.sql

@@ -0,0 +1 @@
+ALTER TABLE api_tokens ALTER COLUMN token SET DEFAULT random_string(32);

migrations/2020-07-13-125308_remove_api_token_default/up.sql

@@ -0,0 +1 @@
+ALTER TABLE api_tokens ALTER COLUMN token DROP DEFAULT;

migrations/2020-07-13-182546_remove_api_tokens_index/down.sql

@@ -0,0 +1,2 @@
+CREATE INDEX api_tokens_token_idx ON api_tokens (token);
+ALTER TABLE api_tokens ADD CONSTRAINT api_tokens_token_key UNIQUE (token);

migrations/2020-07-13-182546_remove_api_tokens_index/up.sql

@@ -0,0 +1,2 @@
+ALTER TABLE api_tokens DROP CONSTRAINT api_tokens_token_key;
+DROP INDEX api_tokens_token_idx;

migrations/2020-07-14-113112_change_api_token_to_bytea/down.sql

@@ -0,0 +1,4 @@
+ALTER TABLE api_tokens
+  ALTER COLUMN token
+  TYPE text USING encode(token, 'escape');
+DROP INDEX api_tokens_token_idx;

migrations/2020-07-14-113112_change_api_token_to_bytea/up.sql

@@ -0,0 +1,5 @@
+CREATE EXTENSION IF NOT EXISTS pgcrypto;
+ALTER TABLE api_tokens
+  ALTER COLUMN token
+  TYPE bytea USING decode(token, 'escape');
+CREATE UNIQUE INDEX ON api_tokens (token);

script/ci/prune-cache.sh

@@ -12,7 +12,7 @@ du -hs target/debug
 
 crate_name="cargo-registry"
 test_name="all"
-bin_names="background-worker delete-crate delete-version enqueue-job monitor populate render-readmes server test-pagerduty transfer-crates"
+bin_names="background-worker delete-crate delete-version enqueue-job monitor populate render-readmes server test-pagerduty transfer-crates verify-token"
 
 normalized_crate_name=${crate_name//-/_}
 rm -vf target/debug/$normalized_crate_name-*

src/bin/verify-token.rs

@@ -0,0 +1,14 @@
+// Look up a username by API token. Used by staff to verify someone's identity
+// by having an API token given. If an error occurs, including being unable to
+// find a user with that API token, the error will be displayed.
+
+use cargo_registry::{db, models::User, util::errors::AppResult};
+use std::env;
+
+fn main() -> AppResult<()> {
+    let conn = db::connect_now()?;
+    let token = env::args().nth(1).expect("API token argument required");
+    let user = User::find_by_api_token(&conn, &token)?;
+    println!("The token belongs to user {}", user.gh_login);
+    Ok(())
+}

src/controllers/util.rs

@@ -2,7 +2,9 @@ use super::prelude::*;
 
 use crate::middleware::current_user::TrustedUserId;
 use crate::models::{ApiToken, User};
-use crate::util::errors::{forbidden, internal, AppResult, ChainError};
+use crate::util::errors::{
+    forbidden, internal, AppError, AppResult, ChainError, InsecurelyGeneratedTokenRevoked,
+};
 
 #[derive(Debug)]
 pub struct AuthenticatedUser {
@@ -42,8 +44,13 @@ impl<'a> UserAuthenticationExt for dyn RequestExt + 'a {
                         user_id: token.user_id,
                         token_id: Some(token.id),
                     })
-                    .chain_error(|| internal("invalid token"))
-                    .chain_error(forbidden)
+                    .map_err(|e| {
+                        if e.is::<InsecurelyGeneratedTokenRevoked>() {
+                            e
+                        } else {
+                            e.chain(internal("invalid token")).chain(forbidden())
+                        }
+                    })
             } else {
                 // Unable to authenticate the user
                 Err(internal("no cookie session or auth header found")).chain_error(forbidden)

src/models.rs

@@ -11,7 +11,7 @@ pub use self::krate::{Crate, CrateVersions, NewCrate, RecentCrateDownloads};
 pub use self::owner::{CrateOwner, Owner, OwnerKind};
 pub use self::rights::Rights;
 pub use self::team::{NewTeam, Team};
-pub use self::token::ApiToken;
+pub use self::token::{ApiToken, CreatedApiToken};
 pub use self::user::{NewUser, User};
 pub use self::version::{NewVersion, Version};
 

src/models/token.rs

@@ -1,20 +1,26 @@
 use chrono::NaiveDateTime;
 use diesel::prelude::*;
+use diesel::sql_types::{Bytea, Text};
 
 use crate::models::User;
 use crate::schema::api_tokens;
+use crate::util::errors::{AppResult, InsecurelyGeneratedTokenRevoked};
 use crate::util::rfc3339;
 use crate::views::EncodableApiTokenWithToken;
 
+const TOKEN_LENGTH: usize = 32;
+const TOKEN_PREFIX: &str = "cio"; // Crates.IO
+
 /// The model representing a row in the `api_tokens` database table.
 #[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable, Associations, Serialize)]
 #[belongs_to(User)]
 pub struct ApiToken {
     pub id: i32,
     #[serde(skip)]
     pub user_id: i32,
+    // Nothing should ever access the plaintext token.
     #[serde(skip)]
-    pub token: String,
+    token: Vec<u8>,
     pub name: String,
     #[serde(with = "rfc3339")]
     pub created_at: NaiveDateTime,
@@ -24,36 +30,41 @@ pub struct ApiToken {
     pub revoked: bool,
 }
 
+diesel::sql_function! {
+    fn digest(input: Text, method: Text) -> Bytea;
+}
+
 impl ApiToken {
     /// Generates a new named API token for a user
-    pub fn insert(conn: &PgConnection, user_id: i32, name: &str) -> QueryResult<ApiToken> {
-        diesel::insert_into(api_tokens::table)
-            .values((api_tokens::user_id.eq(user_id), api_tokens::name.eq(name)))
-            .get_result::<ApiToken>(conn)
-    }
+    pub fn insert(conn: &PgConnection, user_id: i32, name: &str) -> AppResult<CreatedApiToken> {
+        let plaintext = format!(
+            "{}{}",
+            TOKEN_PREFIX,
+            crate::util::generate_secure_alphanumeric_string(TOKEN_LENGTH)
+        );
 
-    /// Converts this `ApiToken` model into an `EncodableApiToken` including
-    /// the actual token value for JSON serialization.  This should only be
-    /// used when initially creating a new token to minimize the chance of
-    /// token leaks.
-    pub fn encodable_with_token(self) -> EncodableApiTokenWithToken {
-        EncodableApiTokenWithToken {
-            id: self.id,
-            name: self.name,
-            token: self.token,
-            revoked: self.revoked,
-            created_at: self.created_at,
-            last_used_at: self.last_used_at,
-        }
+        let model = diesel::insert_into(api_tokens::table)
+            .values((
+                api_tokens::user_id.eq(user_id),
+                api_tokens::name.eq(name),
+                api_tokens::token.eq(digest(&plaintext, "sha256")),
+            ))
+            .get_result::<ApiToken>(conn)?;
+
+        Ok(CreatedApiToken { plaintext, model })
     }
 
-    pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> QueryResult<ApiToken> {
-        use crate::schema::api_tokens::dsl::{api_tokens, last_used_at, revoked, token};
+    pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> AppResult<ApiToken> {
+        use crate::schema::api_tokens::dsl::*;
         use diesel::{dsl::now, update};
 
+        if !token_.starts_with(TOKEN_PREFIX) {
+            return Err(InsecurelyGeneratedTokenRevoked::boxed());
+        }
+
         let tokens = api_tokens
-            .filter(token.eq(token_))
-            .filter(revoked.eq(false));
+            .filter(revoked.eq(false))
+            .filter(token.eq(digest(token_, "sha256")));
 
         // If the database is in read only mode, we can't update last_used_at.
         // Try updating in a new transaction, if that fails, fall back to reading
@@ -63,6 +74,37 @@ impl ApiToken {
                 .get_result::<ApiToken>(conn)
         })
         .or_else(|_| tokens.first(conn))
+        .map_err(Into::into)
+    }
+}
+
+pub struct CreatedApiToken {
+    pub model: ApiToken,
+    pub plaintext: String,
+}
+
+impl CreatedApiToken {
+    /// Converts this `CreatedApiToken` into an `EncodableApiToken` including
+    /// the actual token value for JSON serialization.
+    pub fn encodable_with_token(self) -> EncodableApiTokenWithToken {
+        EncodableApiTokenWithToken {
+            id: self.model.id,
+            name: self.model.name,
+            token: self.plaintext,
+            revoked: self.model.revoked,
+            created_at: self.model.created_at,
+            last_used_at: self.model.last_used_at,
+        }
+    }
+}
+
+// Use a custom implementation of Debug to hide the plaintext token.
+impl std::fmt::Debug for CreatedApiToken {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("CreatedApiToken")
+            .field("model", &self.model)
+            .field("plaintext", &"(sensitive)")
+            .finish()
     }
 }
 
@@ -71,12 +113,20 @@ mod tests {
     use super::*;
     use chrono::NaiveDate;
 
+    #[test]
+    fn test_token_constants() {
+        // Changing this by itself will implicitly revoke all existing tokens.
+        // If this test needs to be change, make sure you're handling tokens
+        // with the old prefix or that you wanted to revoke them.
+        assert_eq!("cio", TOKEN_PREFIX);
+    }
+
     #[test]
     fn api_token_serializes_to_rfc3339() {
         let tok = ApiToken {
             id: 12345,
             user_id: 23456,
-            token: "".to_string(),
+            token: Vec::new(),
             revoked: false,
             name: "".to_string(),
             created_at: NaiveDate::from_ymd(2017, 1, 6).and_hms(14, 23, 11),

src/models/user.rs

@@ -110,10 +110,10 @@ impl User {
     }
 
     /// Queries the database for a user with a certain `api_token` value.
-    pub fn find_by_api_token(conn: &PgConnection, token: &str) -> QueryResult<User> {
+    pub fn find_by_api_token(conn: &PgConnection, token: &str) -> AppResult<User> {
         let api_token = ApiToken::find_by_api_token(conn, token)?;
 
-        Self::find(conn, api_token.user_id)
+        Ok(Self::find(conn, api_token.user_id)?)
     }
 
     pub fn owning(krate: &Crate, conn: &PgConnection) -> QueryResult<Vec<Owner>> {

src/schema.rs

@@ -22,10 +22,10 @@ table! {
         user_id -> Int4,
         /// The `token` column of the `api_tokens` table.
         ///
-        /// Its SQL type is `Varchar`.
+        /// Its SQL type is `Bytea`.
         ///
         /// (Automatically generated by Diesel.)
-        token -> Varchar,
+        token -> Bytea,
         /// The `name` column of the `api_tokens` table.
         ///
         /// Its SQL type is `Varchar`.

src/tests/authentication.rs

@@ -36,7 +36,7 @@ fn anonymous_user_unauthorized() {
 fn token_auth_cannot_find_token() {
     let (app, anon) = TestApp::init().empty();
     let mut request = anon.request_builder(Method::GET, URL);
-    request.header(header::AUTHORIZATION, "fake-token");
+    request.header(header::AUTHORIZATION, "cio1tkfake-token");
 
     let (status, body) = into_parts(call(&app, request));
     assert_eq!(status, StatusCode::FORBIDDEN);

src/tests/krate.rs

@@ -725,7 +725,7 @@ fn new_wrong_token() {
     // Try to publish with the wrong token (by changing the token in the database)
     app.db(|conn| {
         diesel::update(api_tokens::table)
-            .set(api_tokens::token.eq("bad"))
+            .set(api_tokens::token.eq(b"bad" as &[u8]))
             .execute(conn)
             .unwrap();
     });

src/tests/token.rs

@@ -1,4 +1,8 @@
-use crate::{user::UserShowPrivateResponse, util::StatusCode, RequestHelper, TestApp};
+use crate::{
+    user::UserShowPrivateResponse,
+    util::{header, StatusCode},
+    RequestHelper, TestApp,
+};
 use cargo_registry::{
     models::ApiToken,
     schema::api_tokens,
@@ -67,7 +71,10 @@ fn list_tokens() {
             .into_iter()
             .map(|t| t.name)
             .collect::<HashSet<_>>(),
-        tokens.into_iter().map(|t| t.name).collect::<HashSet<_>>()
+        tokens
+            .into_iter()
+            .map(|t| t.model.name)
+            .collect::<HashSet<_>>()
     );
 }
 
@@ -88,7 +95,7 @@ fn list_tokens_exclude_revoked() {
 
     // Revoke the first token.
     let _json: RevokedResponse = user
-        .delete(&format!("/api/v1/me/tokens/{}", tokens[0].id))
+        .delete(&format!("/api/v1/me/tokens/{}", tokens[0].model.id))
         .good();
 
     // Check that we now have one less token being listed.
@@ -97,7 +104,7 @@ fn list_tokens_exclude_revoked() {
     assert!(json
         .api_tokens
         .iter()
-        .find(|token| token.name == tokens[0].name)
+        .find(|token| token.name == tokens[0].model.name)
         .is_none());
 }
 
@@ -167,7 +174,6 @@ fn create_token_success() {
     let tokens = app.db(|conn| t!(ApiToken::belonging_to(user.as_model()).load::<ApiToken>(conn)));
     assert_eq!(tokens.len(), 1);
     assert_eq!(tokens[0].name, "bar");
-    assert_eq!(tokens[0].token, json.api_token.token);
     assert_eq!(tokens[0].revoked, false);
     assert_eq!(tokens[0].last_used_at, None);
 }
@@ -297,3 +303,17 @@ fn using_token_updates_last_used_at() {
     // based on the start of the database transaction so it doesn't work in
     // this test framework.
 }
+
+#[test]
+fn old_tokens_give_specific_error_message() {
+    let url = "/api/v1/me";
+    let (_, anon) = TestApp::init().empty();
+
+    let mut request = anon.get_request(url);
+    request.header(header::AUTHORIZATION, "oldtoken");
+    let json = anon
+        .run::<()>(request)
+        .bad_with_status(StatusCode::UNAUTHORIZED);
+
+    assert_contains!(json.errors[0].detail, "revoked");
+}

src/tests/user.rs

@@ -355,7 +355,7 @@ fn user_total_downloads_no_crates() {
 fn updating_existing_user_doesnt_change_api_token() {
     let (app, _, user, token) = TestApp::init().with_token();
     let gh_id = user.as_model().gh_id;
-    let token = &token.as_model().token;
+    let token = token.plaintext();
 
     let user = app.db(|conn| {
         // Reuse gh_id but use new gh_login and gh_access_token