brave-intl / bat-go

Pass "go", collect 200 BAT
Mozilla Public License 2.0
41 stars 31 forks source link

Add Idempotency Key Namespace UUID Validation #2380

Open kdenhartog opened 6 months ago

kdenhartog commented 6 months ago

https://github.com/brave-intl/bat-go/pull/1691#discussion_r1494164076

Additionally according to the docs this would panic if this is an invalid UUID. Do we want that to occur? That seems like it could lead to an unintentional 500 error

Sneagan commented 4 months ago

This is intended to be hard coded and to panic if it fails to parse. When a transaction is being inserted into QLDB, the first thing we need to do is determine if it already exists. By using UUID v5 here, we are able to ensure that an idempotency key generated with the same data and same namespace will be the same as any prior run. We use this key to check if a transaction is already present and fail to insert if so. Once the record is created, we immediately switch to referring to the transaction by its QLDB document ID. But the initial check needs to use this idempotency key and changing the namespace value here could break some of our idempotency guarantees.

If anything, we should have this UUID in multiple places including hard coded here and verify that the values match when the application initializes. In fact, I'm going to retarget this issue to represent that change.

Sneagan commented 4 months ago

The UUID v5 namespace used for idempotency key generation should be provided as a value in the encrypted configuration file. It should be extracted and exposed with the other secrets to the application and should be checked that it matches a hard coded value before the application proceeds. This check should happen in secrets.go and then the namespace UUID should be inserted into the context with the rest of the sensitive data. See #2088 for how to insert the value into the context.

Sneagan commented 4 months ago

We also need to consolidate the use of this value into one place and document it properly, as called for in the above comment in #2490