simonmcallister0210 / cognito-srp-helper

A helper for SRP authentication in AWS Cognito
Apache License 2.0
12 stars 3 forks source link

Incorrect password hash when using account alias or regular username #26

Closed zolbooo closed 8 months ago

zolbooo commented 8 months ago

Hi there. I would like to thank you for the great job working on this useful library.

I have encountered a problem with using a username during sign in. For example, I call InitiateAuth API with username like user@example.com and Cognito returns a different username like e85de12a-f6b9-4108-a7d6-2f97442ef56e to use in calculations.

Here is the example code:

const formData = new FormData(event.currentTarget);
const username = formData.get("username") as string;
const passwordHash = createPasswordHash(
  username,
  formData.get("password") as string,
  poolId
);
const srpSession = createSrpSession(username, passwordHash, poolId);
const initiateResult = await initiateSRPAuth({
  username,
  srpA: srpSession.largeA,
});
if ("error" in initiateResult) {
  // ...
}

const signedSession = signSrpSession(srpSession, {
  ChallengeParameters: {
    SRP_B: initiateResult.srpB,
    SALT: initiateResult.salt,
    SECRET_BLOCK: initiateResult.secretBlock,
  },
});
const proceedResult = await proceedSRPAuth({
  uid: initiateResult.uid, // This is USER_ID_FOR_SRP
  secret: signedSession.secret,
  timestamp: signedSession.timestamp,
  passwordSignature: signedSession.passwordSignature,
});
if ("error" in proceedResult) {
  // ...
}

Cognito responds with incorrect password error, but I tried using regular password auth and it works. It took a whole day to find out the cause, and I even compared your implementation to Amplify's one.

Turns out, InitiateAuthCommand (initiateSRPAuth in the code above) returns USER_ID_FOR_SRP challenge parameter, which must be used when calculating password hash and signing session. Here is a workaround:

// Create session with empty username and passwordHash,
// because we don't have the username yet, which is required to create the password hash.
const srpSession = createSrpSession("", "", poolId);
const initiateResult = await initiateSRPAuth({
  username,
  srpA: srpSession.largeA,
});
if ("error" in initiateResult) {
  throw Error("TODO: handle error");
}

srpSession.username = initiateResult.uid;
srpSession.passwordHash = createPasswordHash(
  // Important: use username from the Cognito response, not from the form
  initiateResult.uid, // This is USER_ID_FOR_SRP
  formData.get("password") as string,
  poolId
);
const signedSession = signSrpSession(srpSession, {
  ChallengeParameters: {
    SRP_B: initiateResult.srpB,
    SALT: initiateResult.salt,
    SECRET_BLOCK: initiateResult.secretBlock,
  },
});
// ...

This code does not provide initial values for username and passwordHash, and sets them only after the InitiateAuth command response. I think there is a need to create a new API like:

const srpSession = createSrpSession_v2(poolId);
// ...
const passwordHash = createPasswordHash_v2({
  USER_ID_FOR_SRP: initiateAuthResult.ChallengeParameters.USER_ID_FOR_SRP,
  password,
  poolId,
});
const signedSession = signSrpSession_v2(srpSession, passwordHash, initiateAuthResult);
simonmcallister0210 commented 8 months ago

🤔 I'll have a look at this soon. I'll see if I can replicate the issue first

Also, what AWS SDK, Cognito, and cognito-srp-helper versions you are using?

zolbooo commented 8 months ago

I used latest versions of all libraries: @aws-sdk/client-cognito-identity-provider: 3.525.0 cognito-srp-helper: 2.1.0

simonmcallister0210 commented 8 months ago

Thanks 🙂 I can see what you mean, I've managed to replicate issue

So for anyone else who comes across this issue, here's a bit of context. When you set up a Userpool you can configure the attribute used to sign-in. At time of writing you can allow users to sign-in via: Username, Email, Phone Number. When you use an Email or Phone Number for sign-in Cognito will still create a Username for the user, that is a unique ID. According to the docs you can use whatever attribute you want for API calls

The problem we have is the password hash we are generating needs to use the Username not the Email or Phone Number. If you've configured Cognito to use Emails and you're running this library from the front-end odds are you don't have access to the Username, only the Email. So we need to think of a way to somehow get this Username before we authenticate...

@zolbooo Your suggestion of extending the API to provide a function that creates a password hash after the InitiateAuth call sounds like it would work since we have access to InitiateAuthCommandOutput.ChallengeParameters.USER_ID_FOR_SRP at this point. I'd like to think about this a bit more to see if there's a way we can solve this issue without making changes to the API

Maybe we could modify signSrpSession to use USER_ID_FOR_SRP since we have access to USER_ID_FOR_SRP there also? I'll spend some time looking into potential solutions...

simonmcallister0210 commented 8 months ago

Here's the code I used to replicate the issue:

const {
  createSecretHash,
  createPasswordHash,
  createSrpSession,
  signSrpSession,
  wrapAuthChallenge,
  wrapInitiateAuth,
} = require("cognito-srp-helper");
const {
  CognitoIdentityProviderClient,
  InitiateAuthCommand,
  RespondToAuthChallengeCommand,
} = require("@aws-sdk/client-cognito-identity-provider");

(async () => {
  // const usernameUsername = "a2c2e290-6d0f-4a08-a5ca-f0162935f3a6"; // this works
  // const usernameEmail = "yorej57765@comsb.com"; // this doesn't
  const username = "a2c2e290-6d0f-4a08-a5ca-f0162935f3a6";
  const password = "Qwerty1!";
  const poolId = "eu-west-2_eYpv1mFHB";
  const clientId = "18u8119jgbpr464n28s1itk2mq";
  //   const secretId = "" // no secret
  const cognitoIdentityProviderClient = new CognitoIdentityProviderClient({
    region: "eu-west-2",
  });

  //   const secretHash = createSecretHash(username, clientId, secretId);
  const passwordHash = createPasswordHash(username, password, poolId);
  const srpSession = createSrpSession(username, passwordHash, poolId);

  const initiateAuthRes = await cognitoIdentityProviderClient
    .send(
      new InitiateAuthCommand(
        wrapInitiateAuth(srpSession, {
          ClientId: clientId,
          AuthFlow: "USER_SRP_AUTH",
          AuthParameters: {
            CHALLENGE_NAME: "SRP_A",
            // SECRET_HASH: secretHash, // no secret
            USERNAME: username,
          },
        }),
      ),
    )
    .catch((err) => {
      console.error(err);
      throw err;
    });

  console.log("initiateAuthRes:");
  console.log(initiateAuthRes);

  const signedSrpSession = signSrpSession(srpSession, initiateAuthRes);

  console.log("signedSrpSession:");
  console.log(signedSrpSession);

  const respondToAuthChallengeRes = await cognitoIdentityProviderClient
    .send(
      new RespondToAuthChallengeCommand(
        wrapAuthChallenge(signedSrpSession, {
          ClientId: clientId,
          ChallengeName: "PASSWORD_VERIFIER",
          ChallengeResponses: {
            // SECRET_HASH: secretHash, // not configured for this userpool
            USERNAME: username,
          },
        }),
      ),
    )
    .catch((err) => {
      console.error(err);
      throw err;
    });

  console.log("respondToAuthChallengeRes:");
  console.log(respondToAuthChallengeRes);
})();
zolbooo commented 8 months ago

Thanks 🙂 I can see what you mean, I've managed to replicate issue

So for anyone else who comes across this issue, here's a bit of context. When you set up a Userpool you can configure the attribute used to sign-in. At time of writing you can allow users to sign-in via: Username, Email, Phone Number. When you use an Email or Phone Number for sign-in Cognito will still create a Username for the user, that is a unique ID. According to the docs you can use whatever attribute you want for API calls

The problem we have is the password hash we are generating needs to use the Username not the Email or Phone Number. If you've configured Cognito to use Emails and you're running this library from the front-end odds are you don't have access to the Username, only the Email. So we need to think of a way to somehow get this Username before we authenticate...

@zolbooo Your suggestion of extending the API to provide a function that creates a password hash after the InitiateAuth call sounds like it would work since we have access to InitiateAuthCommandOutput.ChallengeParameters.USER_ID_FOR_SRP at this point. I'd like to think about this a bit more to see if there's a way we can solve this issue without making changes to the API

Maybe we could modify signSrpSession to use USER_ID_FOR_SRP since we have access to USER_ID_FOR_SRP there also? I'll spend some time looking into potential solutions...

We can overload function signatures as following:

function createSrpSession(username: string, passwordHash: string, poolId: string); // Old signature
function createSrpSession(poolId: string); // New one, we can ignore next 2 arguments

function signSrpSession(session: SrpSession, response: InitiateAuthResponse); // Old signature
function signSrpSession(session: SrpSession, response: InitiateAuthResponse, password: string); // New signature, we can check if third argument is provided

This way we can keep old behavior when functions are called with old arguments and implement new behavior when different arguments are provided.

simonmcallister0210 commented 8 months ago

Hey @zolbooo . How about we allow the user to pass in unhashed passwords during session creation, then hash them at the point of signing? Would allow for email and phone-number login, and would only require 1 additional optional parameter in createSrpSession so would be backwards compatible with <= 2.1.0 ?

zolbooo commented 8 months ago

Hey @zolbooo . How about we allow the user to pass in unhashed passwords during session creation, then hash them at the point of signing? Would allow for email and phone-number login, and would only require 1 additional optional parameter in createSrpSession so would be backwards compatible with <= 2.1.0 ?

Sounds good to me 😉

renatoargh commented 8 months ago

Hey guys, I have the same problem. Do you have any plans on merging the discussed improvements? Thanks a lot for the great work in this lib :)

simonmcallister0210 commented 8 months ago

Hey @renatoargh . I do, just finished updating the integration tests recently, only have to update the docs now. Then I'll get it merged

Along the way I noticed the tests ran very slow on Node v21, it takes ~20 seconds to sign the session 😬 . That's something I can address later though

renatoargh commented 8 months ago

Alright! Thanks for the heads up! I will keep using the workaround in the meantime! And again, thanks a lot!

simonmcallister0210 commented 8 months ago

Just merged and published the changes with v2.2.0 , if there's any issues let me know