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

WIP: Enforce MIN_SECONDS_BETWEEN for successful authentications #176

Merged
merged 9 commits into from
Jun 18, 2024

Conversation

tinti
Copy link
Contributor

@tinti tinti commented Jun 10, 2024

Issue #175

Description of changes:

V1

Enforces MIN_SECONDS_BETWEEN in both cases by creating an used flag for each magic link.

Reorder magic link validation. Checks expiration before anything.

Validate all fields stored in DynamoDB.

V2

Enforces MIN_SECONDS_BETWEEN in both cases by creating an uat epoch attribute for each magic link.

Reorder magic link validation. Checks expiration before anything.

Validate all fields stored in DynamoDB. Note that uat is optional.

Fix: change DeleteCommand to UpdateCommand. Make sure that UpdateCommand do not perform an insert.

tinti added 5 commits June 9, 2024 21:05
Currently the signature evaluation is done before the expiration.
By doing the expiration first it is possible to save a KMS call if
the link is expired.
Validate all fields returned by DynamoDB. Throw an error if any is
missing.
The MIN_SECONDS_BETWEEN variable is used to protect against multiple
authentications in a short period of time.

However it is only able to protect against multiple authentication
failures. Successful authentications are not limited. A malicious actor
may perform a DoS or drive costs if she has access to a valid user
credential.

This fix enforces MIN_SECONDS_BETWEEN in both cases by creating an
`used` flag for each magic link. When created `used` is set to false.
To be used a magic link must have `used` as false. On use the `used`
is set to true. This will allow the MIN_SECONDS_BETWEEN to deny too
short creation even on successful uses.
Copy link
Contributor

@ottokruse ottokruse left a comment

Choose a reason for hiding this comment

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

Hey mate, i had a look and we're nearly there.

  • Let's use uat (used at) instead of used (because we also do iat for issued at)
  • Let's not make it a boolean but make it a number, in which we store the unix timestamp of when it was used (like iat and exp)
  • In the condition expression we can just say that this attribute MUST NOT yet exist. It's presence signifies that the magic link was already used (and at which time exactly, which may be helpful to know for audit purposes)

The full condition expression then becomes:

`attribute_exists(#signatureHash) AND #signatureHash = :signatureHash AND attribute_exists(#userNameHash) AND #userNameHash = :userNameHash AND attribute_not_exists(#uat)`

How does that sound?

Cheers

@tinti
Copy link
Contributor Author

tinti commented Jun 13, 2024

Hey mate, i had a look and we're nearly there.

  • Let's use uat (used at) instead of used (because we also do iat for issued at)
  • Let's not make it a boolean but make it a number, in which we store the unix timestamp of when it was used (like iat and exp)
  • In the condition expression we can just say that this attribute MUST NOT yet exist. It's presence signifies that the magic link was already used (and at which time exactly, which may be helpful to know for audit purposes)

The full condition expression then becomes:

`attribute_exists(#signatureHash) AND #signatureHash = :signatureHash AND attribute_exists(#userNameHash) AND #userNameHash = :userNameHash AND attribute_not_exists(#uat)`

How does that sound?

Sounds great. Working on it.

Cheers

Use `uat` instead of `used`. `uat` stands for "used at" and stores the
epoch time of the last magic link usage.

Note that `uat` is an optional field.
@tinti
Copy link
Contributor Author

tinti commented Jun 14, 2024

@ottokruse Done. Please take a look.

Fix: ensure UpdateCommand do not perform an insert
Feat: change used to uat
Feat: make uat an optional field when validating

Cheers.

@ottokruse
Copy link
Contributor

Had a look and a play around, pushed a tweak. Think this looks pretty good now! Will merge and publish to NPM in a bit.

(Part of the tweak is a little pedantry, you don't need to check the value of userNameHash in the condition expression as it is the table primary key and so DynamoDB will make sure it is the same)

@tinti
Copy link
Contributor Author

tinti commented Jun 17, 2024

Had a look and a play around, pushed a tweak. Think this looks pretty good now! Will merge and publish to NPM in a bit.

(Part of the tweak is a little pedantry, you don't need to check the value of userNameHash in the condition expression as it is the table primary key and so DynamoDB will make sure it is the same)

Don't we need it to ensure that DynamoDB will perform an update and not an insert? I mean without the condition the an insert could happen.

@ottokruse
Copy link
Contributor

Because it is the primary key, just specifying attribute_exists(userNameHash) is enough. Because if it would need to create that item (if the key is not present in the table yet) then surely attribute userNameHash does not yet exist for that item, because it's a new item.

Try it :) The following only works if the item with userNameHash test exists in the table and raises a ConditionalCheckFailedException otherwise:

aws dynamodb update-item \
    --table-name passwordless-end-to-end-example-SecretsTablePasswordlessXYZ \
    --key '{"userNameHash": {"B": "test"}}' \
    --update-expression "SET #uat = :uat" \
    --expression-attribute-names '{"#uat": "uat"}' \
    --expression-attribute-values '{":uat": {"N": "12345" }}' \
    --condition-expression "attribute_exists(userNameHash)"

@ottokruse ottokruse merged commit c9ee1d2 into aws-samples:main Jun 18, 2024
1 check passed
@tinti
Copy link
Contributor Author

tinti commented Jun 19, 2024

aws dynamodb update-item
--table-name passwordless-end-to-end-example-SecretsTablePasswordlessXYZ
--key '{"userNameHash": {"B": "test"}}'
--update-expression "SET #uat = :uat"
--expression-attribute-names '{"#uat": "uat"}'
--expression-attribute-values '{":uat": {"N": "12345" }}'
--condition-expression "attribute_exists(userNameHash)"

A little bit late. But I tested and agree. Thanks for accepting the patch.

@ottokruse
Copy link
Contributor

Published to npm in v0.14.0

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.

None yet

2 participants