Skip to content

Commit eb093ee

Browse files
committed
Auto merge of #3320 - pietroalbini:refactor-token-generation, r=Turbo87
Add SecureToken to enforce good token hygiene This PR adds the `SecureToken` struct, which relies on the type system to enforce good practices for generating and verifying tokens: * `SecureToken` forces adding an unique prefix in front of every kind of generated token. This allows token scanning services to properly identify tokens generated by crates.io, and allows crates.io to detect which kind of token it's about to revoke. * `SecureToken` only stores and exposes the SHA256 of already generated tokens, to prevent accidental plaintext leakages. The plaintext token value is only exposes for newly generated tokens. r? `@Turbo87`
2 parents 5dffae3 + 46840ca commit eb093ee

File tree

3 files changed

+181
-38
lines changed

3 files changed

+181
-38
lines changed

src/models/token.rs

Lines changed: 10 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,11 @@
11
use chrono::NaiveDateTime;
22
use diesel::prelude::*;
3-
use diesel::sql_types::{Bytea, Text};
43

54
use crate::models::User;
65
use crate::schema::api_tokens;
76
use crate::util::errors::{AppResult, InsecurelyGeneratedTokenRevoked};
87
use crate::util::rfc3339;
9-
10-
const TOKEN_LENGTH: usize = 32;
11-
const TOKEN_PREFIX: &str = "cio"; // Crates.IO
8+
use crate::util::token::{SecureToken, SecureTokenKind};
129

1310
/// The model representing a row in the `api_tokens` database table.
1411
#[derive(Clone, Debug, PartialEq, Eq, Identifiable, Queryable, Associations, Serialize)]
@@ -29,41 +26,35 @@ pub struct ApiToken {
2926
pub revoked: bool,
3027
}
3128

32-
diesel::sql_function! {
33-
fn digest(input: Text, method: Text) -> Bytea;
34-
}
35-
3629
impl ApiToken {
3730
/// Generates a new named API token for a user
3831
pub fn insert(conn: &PgConnection, user_id: i32, name: &str) -> AppResult<CreatedApiToken> {
39-
let plaintext = format!(
40-
"{}{}",
41-
TOKEN_PREFIX,
42-
crate::util::generate_secure_alphanumeric_string(TOKEN_LENGTH)
43-
);
32+
let token = SecureToken::generate(SecureTokenKind::API);
4433

4534
let model: ApiToken = diesel::insert_into(api_tokens::table)
4635
.values((
4736
api_tokens::user_id.eq(user_id),
4837
api_tokens::name.eq(name),
49-
api_tokens::token.eq(digest(&plaintext, "sha256")),
38+
api_tokens::token.eq(token.sha256()),
5039
))
5140
.get_result(conn)?;
5241

53-
Ok(CreatedApiToken { plaintext, model })
42+
Ok(CreatedApiToken {
43+
plaintext: token.plaintext().into(),
44+
model,
45+
})
5446
}
5547

5648
pub fn find_by_api_token(conn: &PgConnection, token_: &str) -> AppResult<ApiToken> {
5749
use crate::schema::api_tokens::dsl::*;
5850
use diesel::{dsl::now, update};
5951

60-
if !token_.starts_with(TOKEN_PREFIX) {
61-
return Err(InsecurelyGeneratedTokenRevoked::boxed());
62-
}
52+
let token_ = SecureToken::parse(SecureTokenKind::API, token_)
53+
.ok_or_else(InsecurelyGeneratedTokenRevoked::boxed)?;
6354

6455
let tokens = api_tokens
6556
.filter(revoked.eq(false))
66-
.filter(token.eq(digest(token_, "sha256")));
57+
.filter(token.eq(token_.sha256()));
6758

6859
// If the database is in read only mode, we can't update last_used_at.
6960
// Try updating in a new transaction, if that fails, fall back to reading
@@ -98,14 +89,6 @@ mod tests {
9889
use crate::views::EncodableApiTokenWithToken;
9990
use chrono::NaiveDate;
10091

101-
#[test]
102-
fn test_token_constants() {
103-
// Changing this by itself will implicitly revoke all existing tokens.
104-
// If this test needs to be change, make sure you're handling tokens
105-
// with the old prefix or that you wanted to revoke them.
106-
assert_eq!("cio", TOKEN_PREFIX);
107-
}
108-
10992
#[test]
11093
fn api_token_serializes_to_rfc3339() {
11194
let tok = ApiToken {

src/util.rs

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
use std::cmp;
22

33
use conduit::{header, Body, Response};
4-
use rand::{distributions::Uniform, rngs::OsRng, Rng};
54
use serde::Serialize;
65

76
pub use self::io_util::{read_fill, read_le_u32, LimitErrorReader};
@@ -13,6 +12,7 @@ mod io_util;
1312
mod request_helpers;
1413
mod request_proxy;
1514
pub mod rfc3339;
15+
pub(crate) mod token;
1616

1717
pub type AppResponse = Response<conduit::Body>;
1818
pub type EndpointResult = Result<AppResponse, Box<dyn errors::AppError>>;
@@ -33,16 +33,6 @@ pub fn json_response<T: Serialize>(t: &T) -> AppResponse {
3333
.unwrap() // Header values are well formed, so should not panic
3434
}
3535

36-
pub fn generate_secure_alphanumeric_string(len: usize) -> String {
37-
const CHARS: &[u8] = b"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
38-
39-
OsRng
40-
.sample_iter(Uniform::from(0..CHARS.len()))
41-
.map(|idx| CHARS[idx] as char)
42-
.take(len)
43-
.collect()
44-
}
45-
4636
#[derive(Debug, Copy, Clone)]
4737
pub struct Maximums {
4838
pub max_upload_size: u64,

src/util/token.rs

Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
use rand::{distributions::Uniform, rngs::OsRng, Rng};
2+
use sha2::{Digest, Sha256};
3+
4+
const TOKEN_LENGTH: usize = 32;
5+
6+
pub(crate) struct SecureToken {
7+
sha256: Vec<u8>,
8+
}
9+
10+
impl SecureToken {
11+
pub(crate) fn generate(kind: SecureTokenKind) -> NewSecureToken {
12+
let plaintext = format!(
13+
"{}{}",
14+
kind.prefix(),
15+
generate_secure_alphanumeric_string(TOKEN_LENGTH)
16+
);
17+
let sha256 = Sha256::digest(plaintext.as_bytes()).as_slice().to_vec();
18+
19+
NewSecureToken {
20+
plaintext,
21+
inner: Self { sha256 },
22+
}
23+
}
24+
25+
pub(crate) fn parse(kind: SecureTokenKind, plaintext: &str) -> Option<Self> {
26+
// This will both reject tokens without a prefix and tokens of the wrong kind.
27+
if SecureTokenKind::from_token(plaintext) != Some(kind) {
28+
return None;
29+
}
30+
31+
let sha256 = Sha256::digest(plaintext.as_bytes()).as_slice().to_vec();
32+
Some(Self { sha256 })
33+
}
34+
35+
pub(crate) fn sha256(&self) -> &[u8] {
36+
&self.sha256
37+
}
38+
}
39+
40+
pub(crate) struct NewSecureToken {
41+
plaintext: String,
42+
inner: SecureToken,
43+
}
44+
45+
impl NewSecureToken {
46+
pub(crate) fn plaintext(&self) -> &str {
47+
&self.plaintext
48+
}
49+
}
50+
51+
impl std::ops::Deref for NewSecureToken {
52+
type Target = SecureToken;
53+
54+
fn deref(&self) -> &Self::Target {
55+
&self.inner
56+
}
57+
}
58+
59+
fn generate_secure_alphanumeric_string(len: usize) -> String {
60+
const CHARS: &[u8] = b"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
61+
62+
OsRng
63+
.sample_iter(Uniform::from(0..CHARS.len()))
64+
.map(|idx| CHARS[idx] as char)
65+
.take(len)
66+
.collect()
67+
}
68+
69+
macro_rules! secure_token_kind {
70+
($(#[$attr:meta])* $vis:vis enum $name:ident { $($key:ident => $repr:expr,)* }) => {
71+
$(#[$attr])*
72+
$vis enum $name {
73+
$($key,)*
74+
}
75+
76+
impl $name {
77+
const VARIANTS: &'static [Self] = &[$(Self::$key,)*];
78+
79+
fn prefix(&self) -> &'static str {
80+
match self {
81+
$(Self::$key => $repr,)*
82+
}
83+
}
84+
}
85+
}
86+
}
87+
88+
secure_token_kind! {
89+
/// Represents every kind of secure token generated by crates.io. When you need to generate a
90+
/// new kind of token you should also add its own kind with its own unique prefix.
91+
///
92+
/// NEVER CHANGE THE PREFIX OF EXISTING TOKEN TYPES!!! Doing so will implicitly revoke all the
93+
/// tokens of that kind, distrupting production users.
94+
#[derive(Debug, Copy, Clone, Eq, PartialEq, Hash)]
95+
pub(crate) enum SecureTokenKind {
96+
API => "cio", // Crates.IO
97+
}
98+
}
99+
100+
impl SecureTokenKind {
101+
fn from_token(token: &str) -> Option<Self> {
102+
Self::VARIANTS
103+
.iter()
104+
.find(|v| token.starts_with(v.prefix()))
105+
.copied()
106+
}
107+
}
108+
109+
#[cfg(test)]
110+
mod tests {
111+
use super::*;
112+
use std::collections::HashSet;
113+
114+
#[test]
115+
fn test_generated_and_parse() {
116+
const KIND: SecureTokenKind = SecureTokenKind::API;
117+
118+
let token = SecureToken::generate(KIND);
119+
assert!(token.plaintext().starts_with(KIND.prefix()));
120+
assert_eq!(
121+
token.sha256(),
122+
Sha256::digest(token.plaintext().as_bytes()).as_slice()
123+
);
124+
125+
let parsed =
126+
SecureToken::parse(KIND, &token.plaintext()).expect("failed to parse back the token");
127+
assert_eq!(parsed.sha256(), token.sha256());
128+
}
129+
130+
#[test]
131+
fn test_parse_no_kind() {
132+
assert!(SecureToken::parse(SecureTokenKind::API, "nokind").is_none());
133+
}
134+
135+
#[test]
136+
fn test_persistent_prefixes() {
137+
// Changing prefixes will implicitly revoke all the tokens of that kind, disrupting users.
138+
// This test serves as a reminder for future maintainers not to change the prefixes, and
139+
// to ensure all the variants are tested by this test.
140+
let mut remaining: HashSet<_> = SecureTokenKind::VARIANTS.iter().copied().collect();
141+
let mut ensure = |kind: SecureTokenKind, prefix| {
142+
assert_eq!(kind.prefix(), prefix);
143+
remaining.remove(&kind);
144+
};
145+
146+
ensure(SecureTokenKind::API, "cio");
147+
148+
assert!(
149+
remaining.is_empty(),
150+
"not all variants have a test to check the prefix"
151+
);
152+
}
153+
154+
#[test]
155+
fn test_conflicting_prefixes() {
156+
// This sanity check prevents multiple tokens from starting with the same prefix, which
157+
// would mess up the token kind detection. If this test fails after adding another variant
158+
// do not change the test, choose another prefix instead.
159+
for variant in SecureTokenKind::VARIANTS {
160+
for other in SecureTokenKind::VARIANTS {
161+
if variant == other {
162+
continue;
163+
}
164+
if variant.prefix().starts_with(other.prefix()) {
165+
panic!("variants {:?} and {:?} share a prefix", variant, other);
166+
}
167+
}
168+
}
169+
}
170+
}

0 commit comments

Comments
 (0)