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: Clean up Tokenserver code #1087

Merged
merged 5 commits into from
Jun 2, 2021

Conversation

ethowitz
Copy link
Contributor

@ethowitz ethowitz commented May 17, 2021

Description

Adds a tokenserver module, with functionality split into separate files.

Testing

First, you'll need to create a new MySQL database and add the appropriate tables. DDL and SQL to accomplish this can be found in this gist.

I was able to test this by creating a valid JWT (I generated a set of RSA keys to test with) with the jwt Python package and sending off a request like so (I used Postman):

curl --location --request GET 'localhost:8000/1.0/sync/1.5' \
  --header 'Authorization: Bearer <YOUR JWT HERE>'

I tested the following cases:

  • Valid JWT --> 200 status code with expected JSON
  • Valid JWT with FxA UID for nonexistent user --> 404 status code
  • Invalid JWT --> 400 status code

Issue(s)

Closes #1052, #968

@ethowitz ethowitz requested a review from a team May 17, 2021 22:33
static ref SERVER_LIMITS: Arc<ServerLimits> = Arc::new(ServerLimits::default());
}

const RSA_PRIVATE_KEY: &str = "-----BEGIN RSA PRIVATE KEY-----\n\
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 a key I generated specifically for the purposes of testing

Copy link

Choose a reason for hiding this comment

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

In this past, our security teams have advised me not to do even this. They suggested generating the keys at test time / somewhere in the repo, instead of checking them in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, noted - thanks for the tip. I'll go ahead and do that then

Copy link

Choose a reason for hiding this comment

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

Oh, and to expand on my previous message: The reasoning was that if you see an RSA key in a repo, it should set off alarm bells. It's easier to just never set of the alarms, instead of continually noting that it's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. And in this case, it literally set off alarm bells (in the form of a GitGuardian email)


impl TokenserverRequest {
fn get_fxa_uid(jwt: &str, rsa_modulus: String, rsa_exponent: String) -> Result<String, Error> {
decode::<Claims>(
Copy link
Member

Choose a reason for hiding this comment

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

Stupid question, do we know what happens if we send in a JWT with alg="None"? That's a fairly common attack vector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a stupid question! It looks like the crate we use doesn't support the None algorithm, so validation will fail: https://github.com/Keats/jsonwebtoken/blob/2f25cbed0a906e091a278c10eeb6cc1cf30dc24a/src/algorithms.rs#L48-L66

I'll verify this manually to be sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed that "alg": "none" in the JWT headers results in a 400 error

@ethowitz ethowitz merged commit e924769 into master Jun 2, 2021
@ethowitz ethowitz deleted the refactor/1052-tokenserver-cleanup branch June 2, 2021 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tokenserver: Refactor and clean up tokenserver.rs
3 participants