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

Magic Link user not found email manipulation - domain extraction #69

Closed
Simonl9l opened this issue Aug 3, 2023 · 3 comments
Closed

Comments

@Simonl9l
Copy link

Simonl9l commented Aug 3, 2023

Per this snippet:

if (event.request.userNotFound) {
logger.info("User not found");
const chars = [...event.userName].filter((c) => c.match(/[a-z]/i));
const name = chars.join("");
const domain = chars.reverse().slice(1, 6).join("");
email = `${name}@${domain}.***`;
}

Were not sure what testing has been done here but in observation and limited testing it possibly not going work all cases:

The domain extraction if understood correctly given the email reconstruction ${name}@${domain}.***, is attempting to just get the root domain and any subdomains, but not TLD/public suffix parts? the slice(1,6) seem out of place?

Whist it understood that the attempt here is to have a fully self-contained implementation there seems to be maintained TLD/public suffix list like here and more specifically here as used by this url parser.

@ottokruse
Copy link
Contributor

ottokruse commented Aug 4, 2023

That piece of code is related to user enumeration prevention. It basically returns a bogus email address, that will be masked.

The story is that when requesting a magic link we return the masked e-mail address in the public challenge parameters. This is nice so the user knows where the magic link is sent to (in case the user does not sign-in with magic link but e.g. with username). Unfortunately this opens the door for user enumeration. Which is why we mask the e-mail addresses.

The code you pasted comes into play when a non-existing user signs in, and the user pool is configured with preventUserExistenceErrors=true (note, the default is false). To prevent user enumeration Cognito will then appear to proceed with the sign-in (event though it will never work) and call the custom auth lambda with event.request.userNotFound = true (so we know it is a non-existing user). In that case we don't have an e-mail address to mask and return as public challenge parameters, so we use the fake username to create a dummy e-mail address.

Now that I explained this to you I think we should remove sending back the e-mail address if the user pool is configured with preventUserExistenceErrors=true.

@Simonl9l
Copy link
Author

Simonl9l commented Aug 4, 2023

@ottokruse - thanks for the reply - for us the Cognito User Pool is not our System of Record for users (as passwordless makes this possible).

Given CloudFront distribution and latency based routing, our plan is to fire up regional stacks in various regions (as needed based on redundant geographical customer footprint or jurisdictional separations - e.g. due to GDPR) with an immutable architecture, leveraging Dynamo DB Global Tables (with said GDPR separations). Such that we'll create a new User Pool with that stack along with requisite lambdas and regional replicate of Global Table(s) etc.

As such in our Login controller, we can check email as a know user is in the DB and AdminCreateUser and any existing but missing users in the User Pool, before we InitiateAuth since User Migration trigger is incompatible with Custom Auth!

We'd bomb out before the InitiateAuth. Or do you suggest we Initiate the auth anyway to get the effect of the preventUserExistenceErrors=true?

Albeit thinking, we should also be denying magic link auth anyway that have a passkey/fido credential registered per security posture issue #68

well look out for the fix you mention above at the end.

@ottokruse
Copy link
Contributor

We have gotten rid of sending back the email address as public challenge parameter in #134

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

No branches or pull requests

2 participants