awslabs / aws-jwt-verify

JS library for verifying JWTs signed by Amazon Cognito, and any OIDC-compatible IDP that signs JWTs with RS256, RS384, and RS512
Apache License 2.0
606 stars 42 forks source link

Expected 2 arguments, but got 1.ts(2554) #36

Closed axelra82 closed 2 years ago

axelra82 commented 2 years ago

Getting Expected 2 arguments, but got 1.ts(2554) on "aws-jwt-verify": "^2.0.0" and "typescript": "^4.5.4" (mentioning props with identical values as CognitoJwtVerifier) with the following:

 const {
    jwtToken: jwt,
    payload: { iss, client_id },
  } = accessToken;
  const userPoolId = iss.replace(/https:\/\/cognito-idp.eu-north-1.amazonaws.com\/(.*?)/, "$1");
  const clientId = client_id;

  // Verifier that expects valid access tokens:
  const verifier = CognitoJwtVerifier.create({
    userPoolId: userPoolId,
    clientId: clientId,
    tokenUse: "access",
  });
  await verifier.verify(jwt);

Works as expected in version 1.0.3.

ottokruse commented 2 years ago

Hi @axelra82 . V2.0.0 has slightly stricter types, and one of the things it does using type checks, is ensure you've provided values for all fields that should have a value. I suspect this check triggers in your case now, and the reason is probably that your clientId is of type string | undefined, can you check that?

The way this works (supposed to) is as follows:

Alternatively, add an exclamation mark so that Typescript can understand the value is in fact there:

const {
    jwtToken: jwt,
    payload: { iss, client_id },
  } = accessToken;
  const userPoolId = iss.replace(/https:\/\/cognito-idp.eu-north-1.amazonaws.com\/(.*?)/, "$1");
  const clientId = client_id!; // exclamation mark added

  // Verifier that expects valid access tokens:
  const verifier = CognitoJwtVerifier.create({
    userPoolId: userPoolId,
    clientId: clientId,
    tokenUse: "access",
  });
  await verifier.verify(jwt);

If you hover over the verifier variable, intellisense shows the type, I suspect in your case it looks like this (note the undefined for clientId): image

Side note: the example you provided here seems to read the userPoolId from the decoded JWT itself? (You seem to have a step where you decode the JWT first, and create the accessToken variable). Is that just an example to help us reproduce? If not, please ensure that the userPoolId you create the verifier for is one you actually trust, and not a potential attacker controlled userPoolId. Forgive me if I'm telling you things you already know!

axelra82 commented 2 years ago

@ottokruse Thank you for the speedy response... and clarification! <-- 😉 Didn't think to check the intellisense on the verifier.

Haven't had time to test it out in sandbox yet, but at least now it doesn't give any errors (so it looks good anyway) 👍

// deconstruct data
  const {
    idToken: {
      jwtToken: jwt,
      payload: { iss, client_id, email, ["cognito:groups"]: role, token_use },
    },
  } = data; // <-- data is coming in from the API post body

  // get clean user pool id
  const userPoolId: string = iss!.replace(
    /https:\/\/cognito-idp\..*?\.amazonaws\.com\/(.*?)/,
    "$1"
  );
  const clientId: string = client_id;
  const tokenUse: "id" | "access" = token_use;

  // verifier expects valid access tokens
  const verifier = CognitoJwtVerifier.create({
    clientId,
    userPoolId,
    tokenUse,
  });

  try {
    await verifier.verify(jwt);
    [...]

Regarding the side note... first, wether I knew it or not, thank you for pointing it out! 😄 For me (in case) and for anyone else looking at this in the future 💯 Now, I'm not sure what you mean by "...read the userPoolId from the decoded JWT itself?". I'm passing in an idToken in all API calls. That token contains the JWT and the payload as two separate objects. I'm extracting the userPoolId from the iss using a regex.

Finally using a try catch for the verifier, and if that passes the rest of the lambda can continue.

PS. Switched to using idToken as it contains some keys that accessToken doesn't (and we need one of them).

ottokruse commented 2 years ago

Using ID token is fine, but do treat the token as untrusted until you've verified it, and verification must not use reference values (e.g. userPoolId, clientId) you derive from the token header or payload itself.

Because an attacker might send in a JWT that the attacker created with his/her own user pool (their iss). Even though the signature of those JWTs will be valid (for their pool), you don't wanna trust those JWTs :)

You mention data is coming in from the API post body, and the JWT is part of that data, so the question is, do you trust that data (I presume not yet, which is why you're verifying the JWT).

Normally, you'd provide static values into userPoolId and clientId, often sourced from environment variables (e.g. process.env.USER_POOL_ID), that you make available to the Lambda function in your CloudFormation (/CDK /Terraform) template, and that has the value of the UserPoolId from the UserPool in that same CloudFormation template (e.g. !Ref UserPool). An example within this repo is here where the env vars are populated from these lines of CDK code.

Hope that clarifies what I meant.

axelra82 commented 2 years ago

We are currently setting up a multi-tenant platform, and using APIs with proxy to route to different lambdas with path based actions. So environment variables in the lambda are not an option for us.

The API post requests are coming from a closed system (i.e. the user has to login). The end goal of using the aws-jwt-verify is to make sure that the request coming in is from a valid user in the user pool of that tenant with their specific identity pool.

One step in that process is using this pack to verify the token itself. So if the JWT verification fails, the call falls into the initial catch. Are you saying that someone could just fake an idToken with valid data (which would allow access)? 🤔

I'm not really following the logic in that case. What is the point of a JWT verifier if it doesn't make sure that the JWT contains Cognito provided authentication that can be verified?

I would've expected some values in the JWT payload, like event_id and origin_jti (just to clarify, I'm not sure what these values represent and where they come from 😅) to be used to actually verify that the JWT is real and confirms the requests validity.

Again, not 100% sure I'm ready everything correctly, but from what I understand... what you're saying is that we need to pass in the user pool id and client id from some other "behind the scenes" (secure) location, rather than sending them in the post body? 🤔 The reason I'm asking is simply because I thought that the JWT verifier made sure that the content of the JWT payload was "correct" 😄

PS. Thanks for the feedback! 💯

ottokruse commented 2 years ago

PS. Thanks for the feedback! 💯

No problem!

Long story short, you must make sure that the userPoolId and clientId that you provide in the CognitoJwtVerifier.create call, are a userPoolId and clientId that you trust, i.e. one of your tenants. How do you ensure that?

axelra82 commented 2 years ago

OK, great. It took a minute and we had internal discussions regarding this, but I think it's clear now why doing it this way would be insecure.

I guess one simple way to bypass this would be to: Create an AWS account, create a user pool, web client and user in that account and then use the idToken from that. Then in the current implementation, I could call our API using the idToken created in an external account (which has a JWT that matches the payload from that token).

Just writing it out to better understand the issue (and as always, for any other lost souls stumbling upon this issue in the future).

Luckily we have internal processes for dealing with this so we can secure the user pool and client id from another source that can't be tampered with externally 👍 So we'll use that and then all should be good. From my understanding, the scenario above would fail, since even if the JWT is correct (from my external account), it wouldn't match our tenants user pool and client id records. So that should be secure 🤓

Thanks again for the discussion around this 🙌

ottokruse commented 2 years ago

That's it mate 👍 And note btw that you can use multiple User Pool IDs with this lib too. Have fun building.