Skip to content
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

refactor(turborepo): Signature Authentication (2nd try) #4980

Merged

Conversation

NicholasLYang
Copy link
Contributor

Description

Refactors the signature authentication to use a language-independent algorithm, as the previous implementation depended on Go's JSON serialization.

NOTE: This is a global hash bump.

Testing Instructions

Testing is using fuzzing across the Go-Rust boundary to check that signatures are correctly generated on both sides.

@NicholasLYang NicholasLYang added the global-hash-bump Requires a bump to the global hash key label May 16, 2023
@NicholasLYang NicholasLYang requested review from a team as code owners May 16, 2023 17:24
@vercel
Copy link

vercel bot commented May 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
examples-tailwind-web 🔄 Building (Inspect) May 16, 2023 5:25pm
8 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) May 16, 2023 5:25pm
examples-cra-web ⬜️ Ignored (Inspect) May 16, 2023 5:25pm
examples-designsystem-docs ⬜️ Ignored (Inspect) May 16, 2023 5:25pm
examples-gatsby-web ⬜️ Ignored (Inspect) May 16, 2023 5:25pm
examples-kitchensink-blog ⬜️ Ignored (Inspect) May 16, 2023 5:25pm
examples-native-web ⬜️ Ignored (Inspect) May 16, 2023 5:25pm
examples-svelte-web ⬜️ Ignored (Inspect) May 16, 2023 5:25pm
examples-vite-web ⬜️ Ignored (Inspect) May 16, 2023 5:25pm

@vercel
Copy link

vercel bot commented May 16, 2023

@NicholasLYang is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@arlyon arlyon left a comment

Choose a reason for hiding this comment

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

Approved with some minor comments. I think we should announce this in the turbo channel / turboverse before merging.

}
}

fn secret_key(&self) -> Result<Vec<u8>, SignatureError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that HMAC_SHA256 doesn't have key length requirements but a comment that verifies that would be handy.

artifact_body: &[u8],
expected_tag: &str,
) -> Result<bool, SignatureError> {
let secret_key = hmac::Key::new(HMAC_SHA256, &self.secret_key()?);
Copy link
Contributor

@arlyon arlyon May 17, 2023

Choose a reason for hiding this comment

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

I also think we could pull HMAC_SHA256 into a constant, since it's used in more than one place. Something like TURBO_HMAC_ALGO

Base64EncodingError(#[from] base64::DecodeError),
}

static TURBO_HMAC_ALGORITHM: Algorithm = HMAC_SHA256;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is static because apparently you can't have a const that refers to another const

@NicholasLYang NicholasLYang merged commit 0ee7128 into vercel:main May 18, 2023
47 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
global-hash-bump Requires a bump to the global hash key
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants