-
Notifications
You must be signed in to change notification settings - Fork 643
Implement PUT /api/v1/trusted_publishing/tokens
API endpoint
#11131
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
base: main
Are you sure you want to change the base?
Changes from all commits
d6fa9d9
6eda81f
512c762
9ed85fa
a0592db
31e6708
6a2caee
b5eb62b
83ce793
222b2b8
8a2abad
35d61c0
8cf78d8
6928349
83be81e
e3a2835
b2ace7b
d203be6
3cd0e7b
a60534f
4006490
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,7 @@ | ||
mod github_config; | ||
mod token; | ||
mod used_jti; | ||
|
||
pub use self::github_config::{GitHubConfig, NewGitHubConfig}; | ||
pub use self::token::NewToken; | ||
pub use self::used_jti::NewUsedJti; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
use crate::schema::trustpub_tokens; | ||
use chrono::{DateTime, Utc}; | ||
use diesel::prelude::*; | ||
use diesel_async::{AsyncPgConnection, RunQueryDsl}; | ||
|
||
#[derive(Debug, Insertable)] | ||
#[diesel(table_name = trustpub_tokens, check_for_backend(diesel::pg::Pg))] | ||
pub struct NewToken<'a> { | ||
pub expires_at: DateTime<Utc>, | ||
pub hashed_token: &'a [u8], | ||
pub crate_ids: &'a [i32], | ||
} | ||
|
||
impl NewToken<'_> { | ||
pub async fn insert(&self, conn: &mut AsyncPgConnection) -> QueryResult<()> { | ||
self.insert_into(trustpub_tokens::table) | ||
.execute(conn) | ||
.await?; | ||
|
||
Ok(()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
use crate::schema::trustpub_used_jtis; | ||
use chrono::{DateTime, Utc}; | ||
use diesel::prelude::*; | ||
use diesel_async::{AsyncPgConnection, RunQueryDsl}; | ||
|
||
#[derive(Debug, Insertable)] | ||
#[diesel(table_name = trustpub_used_jtis, check_for_backend(diesel::pg::Pg))] | ||
pub struct NewUsedJti<'a> { | ||
pub jti: &'a str, | ||
pub expires_at: DateTime<Utc>, | ||
} | ||
|
||
impl<'a> NewUsedJti<'a> { | ||
pub fn new(jti: &'a str, expires_at: DateTime<Utc>) -> Self { | ||
Self { jti, expires_at } | ||
} | ||
|
||
pub async fn insert(&self, conn: &mut AsyncPgConnection) -> QueryResult<usize> { | ||
diesel::insert_into(trustpub_used_jtis::table) | ||
.values(self) | ||
.execute(conn) | ||
.await | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
use secrecy::{ExposeSecret, SecretString}; | ||
use sha2::digest::Output; | ||
use sha2::{Digest, Sha256}; | ||
|
||
/// A temporary access token used to publish crates to crates.io using | ||
/// the "Trusted Publishing" feature. | ||
#[derive(Debug)] | ||
pub struct AccessToken(SecretString); | ||
|
||
impl AccessToken { | ||
const PREFIX: &str = "cio_tp_"; | ||
|
||
/// Generate a new access token. | ||
pub fn generate() -> Self { | ||
Self::from_u64s(rand::random(), rand::random()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Small thing, but two questions:
(The historical reason for not using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
unless I'm misunderstanding our application, crates.io only uses a single process, but then has multiple worker threads inside the process. since we don't have a GIL in Rust, or are generally single-threaded like in JS, there is usually no need for multiple processes.
no real reason actually. it seemed easiest to get to the hex output from there, but I'm totally open to alternatives 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Gotcha, thanks! The concern with
It's an extreme nitpick from me so feel free to ignore! But from a type/domain coherence perspective I'd recommend |
||
} | ||
|
||
/// Create an access token from two u64 values. | ||
/// | ||
/// This is used internally by the `generate()` fn and is extracted | ||
/// to a separate function for testing purposes. | ||
fn from_u64s(r1: u64, r2: u64) -> Self { | ||
let plaintext = format!("{}{r1:016x}{r2:016x}", Self::PREFIX); | ||
Self(SecretString::from(plaintext)) | ||
} | ||
Comment on lines
+13
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yep, 128 bits of entropy is sufficient! You could also do 256 bits to match what PyPI does for its tokens, but that's IMO overkill.
I think it depends partially on expected load/how expensive you expect credential revocation to be -- if there's a mass revocation event where GitHub is sending you tends of thousands of potential credentials, it can be helpful to have a pre-DB filter that weeds out false positives. (This of course doesn't prevent someone from spamming you with fake tokens, since a CRC32 or similar is easy to crunch. But it limits the spam computationally, i.e. requires the spammer to waste time on that and keeps them from having a multiplicative impact on the DB itself.) TL;DR: if the DB roundtrip is or could become expensive for token revocation, then a checksum or check-digit sequence is a good idea. Otherwise, you could probably leave it out of the MVP and version it in later 🙂 (I haven't used it myself, but https://crates.io/crates/crc32fast might be a good candidate?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the past, I've generally regretted not adding in the support for the checksum. 😓 That said, the tokens often looked like other hashes, so there were a lot of false positive submissions. Since these have the unique prefix the number of submissions should end up restricted to things that actually look like tokens.
+1 to leaving this out for the MVP and only implementing if it becomes an issue later on. |
||
|
||
pub fn from_bytes(bytes: &[u8]) -> Option<Self> { | ||
let str = String::from_utf8(bytes.into()).ok()?; | ||
|
||
let suffix = str.strip_prefix(Self::PREFIX)?; | ||
if suffix.len() != 32 { | ||
return None; | ||
} | ||
|
||
let is_hexdigit = |c| matches!(c, 'a'..='f') || c.is_ascii_digit(); | ||
if !suffix.chars().all(is_hexdigit) { | ||
return None; | ||
} | ||
|
||
Some(Self(SecretString::from(str))) | ||
} | ||
|
||
/// Generate a SHA256 hash of the access token. | ||
pub fn sha256(&self) -> Output<Sha256> { | ||
Sha256::digest(self.0.expose_secret()) | ||
} | ||
} | ||
|
||
impl ExposeSecret<str> for AccessToken { | ||
fn expose_secret(&self) -> &str { | ||
self.0.expose_secret() | ||
} | ||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
use claims::{assert_none, assert_some}; | ||
use insta::assert_snapshot; | ||
|
||
#[test] | ||
fn test_generate() { | ||
let token = AccessToken::generate(); | ||
let token_str = token.expose_secret(); | ||
assert!(token_str.starts_with(AccessToken::PREFIX)); | ||
assert_eq!(token_str.len(), AccessToken::PREFIX.len() + 32); | ||
} | ||
|
||
#[test] | ||
fn test_serialization() { | ||
let token = AccessToken::from_u64s(0, 0); | ||
assert_snapshot!(token.expose_secret(), @"cio_tp_00000000000000000000000000000000"); | ||
|
||
let token = AccessToken::from_u64s(u64::MAX, u64::MAX); | ||
assert_snapshot!(token.expose_secret(), @"cio_tp_ffffffffffffffffffffffffffffffff"); | ||
|
||
let token = AccessToken::from_u64s(0xc0ffee, 0xfa8072); | ||
assert_snapshot!(token.expose_secret(), @"cio_tp_0000000000c0ffee0000000000fa8072"); | ||
} | ||
|
||
#[test] | ||
fn test_sha256() { | ||
let token = AccessToken::generate(); | ||
let hash = token.sha256(); | ||
assert_eq!(hash.len(), 32); | ||
} | ||
|
||
#[test] | ||
fn test_from_bytes() { | ||
let token = AccessToken::generate(); | ||
let bytes = token.expose_secret().as_bytes(); | ||
let token2 = assert_some!(AccessToken::from_bytes(bytes)); | ||
assert_eq!(token.expose_secret(), token2.expose_secret()); | ||
|
||
let bytes = b"cio_tp_00000000000000000000000000000000"; | ||
assert_some!(AccessToken::from_bytes(bytes)); | ||
|
||
let invalid_bytes = b"invalid_token"; | ||
assert_none!(AccessToken::from_bytes(invalid_bytes)); | ||
|
||
let invalid_bytes = b"cio_tp_invalid_token"; | ||
assert_none!(AccessToken::from_bytes(invalid_bytes)); | ||
|
||
let invalid_bytes = b"cio_tp_00000000000000000000000000"; | ||
assert_none!(AccessToken::from_bytes(invalid_bytes)); | ||
|
||
let invalid_bytes = b"cio_tp_000000x0000000000000000000000000"; | ||
assert_none!(AccessToken::from_bytes(invalid_bytes)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for regular tokens we use
cio
plus 32 alphanumeric characters. when we implemented GitHub Secret Scanning we were told to use a more unique prefix with a separator. therefore the temporary access tokens are using thecio_to_
prefix to make them easier to recognize with less false positives.