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

1433: Create user hashing for Koblenz #1499

Merged
merged 12 commits into from
Jul 30, 2024

Conversation

ztefanie
Copy link
Member

Short description

Add argon2 hashing for card info

Proposed changes

  • Added argon2id hashing for user data
  • Added test for argon2id hashing
  • Added Koblenz region and refactored region db setup a little
  • Added tests for koblenz canonical json creation
  • Added docs for Koblenz how to create valid hashes
  • Added pepper to passbold
  • Added pepper to env var in Saltstack pr: https://git.tuerantuer.org/DF/salt/pulls/174

Resolved issues

Fixes: #1433

throw Exception("Invalid Json input for hashing")
}

val pepper = Environment.getVariable(KOBLENZ_PEPPER_SYS_ENV) //TODO handle if Null
Copy link
Member Author

Choose a reason for hiding this comment

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

What error handling do we want, if the env var cannot be accesed / is not set?

Copy link
Contributor

Choose a reason for hiding this comment

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

just added a basic exception. Do we need a separate exception for that?

@ztefanie ztefanie force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch 2 times, most recently from 29ec97a to df38492 Compare June 19, 2024 11:35
@ztefanie ztefanie force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch from df38492 to 38164c7 Compare June 19, 2024 12:03
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

I'm a little bit confused here about the scope of this task and probably there was a misunderstanding. Seems like i didn't explain this clear enough in the issue :/

I expected in this task: (#1433)

  • create a UserInfoModel (name, birthday, referenceNr)
  • hash this model with argon2id
  • create tests for the hashing
  • provide sample input and output data to koblenz service provider.

For other tasks like CardCreation we have different tasks, f.e.
#1421

specs/card.proto Outdated Show resolved Hide resolved
@ztefanie ztefanie force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch 2 times, most recently from 2495c6a to ac86f6f Compare July 2, 2024 11:57
@ztefanie ztefanie force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch from ac86f6f to f67524a Compare July 2, 2024 12:05
Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Nice implementation!
Added some minor comments
Unfortunately i can not start the backend receiving this error

Exception in thread "main" java.lang.Error: Required project 'koblenz.pass.app' not found!
	at app.ehrenamtskarte.backend.regions.database.SetupKt.insertOrUpdateRegions$createOrUpdateRegion(Setup.kt:21)
	at app.ehrenamtskarte.backend.regions.database.SetupKt.access$insertOrUpdateRegions$createOrUpdateRegion(Setup.kt:1)
	at app.ehrenamtskarte.backend.regions.database.SetupKt$insertOrUpdateRegions$1.invoke(Setup.kt:61)
	at app.ehrenamtskarte.backend.regions.database.SetupKt$insertOrUpdateRegions$1.invoke(Setup.kt:38)
	```
	

@f1sh1918
Copy link
Contributor

f1sh1918 commented Jul 3, 2024

Nice implementation! Added some minor comments Unfortunately i can not start the backend receiving this error

Exception in thread "main" java.lang.Error: Required project 'koblenz.pass.app' not found!
	at app.ehrenamtskarte.backend.regions.database.SetupKt.insertOrUpdateRegions$createOrUpdateRegion(Setup.kt:21)
	at app.ehrenamtskarte.backend.regions.database.SetupKt.access$insertOrUpdateRegions$createOrUpdateRegion(Setup.kt:1)
	at app.ehrenamtskarte.backend.regions.database.SetupKt$insertOrUpdateRegions$1.invoke(Setup.kt:61)
	at app.ehrenamtskarte.backend.regions.database.SetupKt$insertOrUpdateRegions$1.invoke(Setup.kt:38)

@ztefanie
You probably have to create an entry for koblenz in the backend configuration. (config.yml)
Additionally we need for deployment on staging and production configs in salt or the backend service will not start
@svenseeberg can do that

You probably should talk to daniel and sven that we need some domain for koblenz. Just to prepare that

@ztefanie
Copy link
Member Author

ztefanie commented Jul 9, 2024

Nice implementation! Added some minor comments Unfortunately i can not start the backend receiving this error

Exception in thread "main" java.lang.Error: Required project 'koblenz.pass.app' not found!
	at app.ehrenamtskarte.backend.regions.database.SetupKt.insertOrUpdateRegions$createOrUpdateRegion(Setup.kt:21)
	at app.ehrenamtskarte.backend.regions.database.SetupKt.access$insertOrUpdateRegions$createOrUpdateRegion(Setup.kt:1)
	at app.ehrenamtskarte.backend.regions.database.SetupKt$insertOrUpdateRegions$1.invoke(Setup.kt:61)
	at app.ehrenamtskarte.backend.regions.database.SetupKt$insertOrUpdateRegions$1.invoke(Setup.kt:38)

I can start the backend with no problems, maybe some problem with your local setup. We can talk about this, when your vacation is over.

@ztefanie You probably have to create an entry for koblenz in the backend configuration. (config.yml) Additionally we need for deployment on staging and production configs in salt or the backend service will not start @svenseeberg can do that

Not part of this issue.

You probably should talk to daniel and sven that we need some domain for koblenz. Just to prepare that

Getting the domains for Koblenz is not part of this issue and not even related

docs/CreateKoblenzHash.md Outdated Show resolved Hide resolved
Comment on lines +26 to +27
- The birthday is defined in our protobuf [card.proto](../frontend/card.proto) file: It counts the days since the birthday (calculated from 1970-01-01).
All values of this field are valid, including the 0, which indicates that the birthday is on 1970-01-01. Birthdays before 1970-01-01 have negative values.
Copy link
Member

Choose a reason for hiding this comment

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

Potentially, it would be easier (for Koblenz and for debuggability) to use a ISO 8601 date string instead, but I'm fine with both :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point, would keep it like this, so at least our code is consistent?

Copy link
Member

Choose a reason for hiding this comment

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

ISO 8601 might be problematic because there are maybe multiple encodings of the same date. Not sure though. I'm just thinking about the "+Z" and "+00:00"

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm gonna keep the calculated birthday here if thats okay for everyone

docs/CreateKoblenzHash.md Outdated Show resolved Hide resolved
docs/CreateKoblenzHash.md Outdated Show resolved Hide resolved
docs/CreateKoblenzHash.md Show resolved Hide resolved
Comment on lines +26 to +27
- The birthday is defined in our protobuf [card.proto](../frontend/card.proto) file: It counts the days since the birthday (calculated from 1970-01-01).
All values of this field are valid, including the 0, which indicates that the birthday is on 1970-01-01. Birthdays before 1970-01-01 have negative values.
Copy link
Member

Choose a reason for hiding this comment

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

ISO 8601 might be problematic because there are maybe multiple encodings of the same date. Not sure though. I'm just thinking about the "+Z" and "+00:00"

docs/CreateKoblenzHash.md Outdated Show resolved Hide resolved
@maxammann
Copy link
Member

Another approach would have been to hash on the client and just the data to the server for the lookup, right?

@maxammann
Copy link
Member

I believe this endpoint should have some rate limiting as it might be a target for DoS (argon2id is costly). The alternative approach would be to generate the hash on the client.

@michael-markl
Copy link
Member

Another approach would have been to hash on the client and just the data to the server for the lookup, right?

Not really possible, as we don't want that the client can decide what to write to the card.

@maxammann
Copy link
Member

Another approach would have been to hash on the client and just the data to the server for the lookup, right?

Not really possible, as we don't want that the client can decide what to write to the card.

I believe we could still send the user data to the server and do the additional check only if we find the matching hash. Then the expensive argon2id/pbkdf verification only runs if the hash can be found. The user would still send the name, birthdate etc to the server.

@ztefanie
Copy link
Member Author

I believe this endpoint should have some rate limiting as it might be a target for DoS (argon2id is costly). The alternative approach would be to generate the hash on the client.

Will be done here: #1421
and is already mentioned in the issue, that it needs rate limiting.

# Conflicts:
#	backend/src/main/kotlin/app/ehrenamtskarte/backend/cards/Argon2IdHasher.kt
#	backend/src/main/kotlin/app/ehrenamtskarte/backend/exception/service/NotCorrectProjectExceptions.kt
#	backend/src/test/kotlin/app/ehrenamtskarte/backend/cards/Argon2IdHasherTest.kt
docs/CreateKoblenzHash.md Outdated Show resolved Hide resolved
@f1sh1918 f1sh1918 force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch from eaacbc7 to 396b4c4 Compare July 29, 2024 13:41
@f1sh1918
Copy link
Contributor

f1sh1918 commented Jul 29, 2024

Just updated the pr by adding a project config, adjusting hash and argon config in the docs.
Is there anything else that is missing?

@f1sh1918 f1sh1918 force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch from 396b4c4 to d3879e2 Compare July 29, 2024 14:00
@f1sh1918 f1sh1918 force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch from ba75f0c to c630dec Compare July 29, 2024 15:28
@f1sh1918 f1sh1918 force-pushed the 1433-create-argon2-user-hashing-for-koblenz branch from e8aa059 to 6695e76 Compare July 29, 2024 15:36
@f1sh1918 f1sh1918 requested a review from seluianova July 30, 2024 08:56
@f1sh1918 f1sh1918 merged commit 10a234e into main Jul 30, 2024
1 check passed
@f1sh1918 f1sh1918 deleted the 1433-create-argon2-user-hashing-for-koblenz branch July 30, 2024 09:59
@michael-markl
Copy link
Member

I believe we could still send the user data to the server and do the additional check only if we find the matching hash. Then the expensive argon2id/pbkdf verification only runs if the hash can be found.

I just noticed that this is also not possible, as the user would know the "secret salt/pepper" to generate the hash. So I think we can only do rate limiting.

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.

Create user data hash
5 participants