Skip to content

Conversation

@fisx
Copy link
Contributor

@fisx fisx commented Aug 26, 2022

https://wearezeta.atlassian.net/browse/FS-736 (the part where we need to change the way we store nonces)

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • If this PR changes development workflow or dependencies, they have been A) automated and B) documented under docs/developer/. All efforts have been taken to minimize development setup breakage or slowdown for co-workers.
  • If HTTP endpoint paths have been added or renamed, check docs/developer/adding-api-endpoints and follow the steps there.
  • If configuration options have been added or removed, check docs/developer/adding-config-options and follow the steps there.
  • If a cassandra schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
  • I swear that if I have changed internal end-points, I do not implicitly rely on deployment ordering (brig needing to be deployed before galley), i.e. I have not introduced a new internal endpoint in brig and already make use of if in galley, as I'm aware old deployed galleys would throw 500s until all new version have been rolled out and I do not want a few minutes of downtime. Instead, I have thought how to merge brig an galley codebases.
  • I updated changelog.d subsections with one or more entries with the following bits of information (details):
    • If a cassandra schema migration is backwards incompatible (see also these docs), measures to be taken by on-premise instance operators are explained.
    • If a data migration (not schema migration) introduced: measures to be taken by on-premise instance operators.

@fisx fisx temporarily deployed to cachix August 26, 2022 09:04 Inactive
@fisx fisx temporarily deployed to cachix August 26, 2022 09:04 Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 26, 2022
insert :: PrepQuery W (Identity Nonce) ()
insert = fromString $ "INSERT INTO client_nonce (nonce) VALUES (?) USING TTL " <> show ttl
insert :: PrepQuery W (Text, Nonce) ()
insert = fromString $ "INSERT INTO nonce (key, nonce) VALUES (?) USING TTL " <> show ttl
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
insert = fromString $ "INSERT INTO nonce (key, nonce) VALUES (?) USING TTL " <> show ttl
insert = fromString $ "INSERT INTO nonce (key, nonce) VALUES (?, ?) USING TTL " <> show ttl

@fisx fisx temporarily deployed to cachix August 26, 2022 09:55 Inactive
@fisx fisx temporarily deployed to cachix August 26, 2022 09:55 Inactive
@fisx
Copy link
Contributor Author

fisx commented Aug 26, 2022

This PR does not solve the problem that we need to change the API to somehow give us the ClientId (I though it's derivable from the Z-Conn header, but I changed my mind about that).

After TASTY_PATTERN='/nonce/' make ci package=brig has passed:

cqlsh> select * from brig_test.nonce ;

 key | nonce
-----+--------------------------------------
 123 | e002192e-7447-4a8d-9c26-f6edd7563128

(1 rows)
cqlsh> select * from brig_test.nonce ;

 key | nonce
-----+-------

(0 rows)

@fisx
Copy link
Contributor Author

fisx commented Aug 26, 2022

no need to discuss the destructive cassandra update, as (a) the client_nonce has never been released, and (b) it cannot be used for anything useful in production yet.

@fisx fisx marked this pull request as ready for review August 26, 2022 09:59
@fisx
Copy link
Contributor Author

fisx commented Aug 26, 2022

If you want to make sure we don't store negative TTLs:

$ git diff
diff --git a/services/brig/src/Brig/Data/Nonce.hs b/services/brig/src/Brig/Data/Nonce.hs
index ade768e4e..d11fd8372 100644
--- a/services/brig/src/Brig/Data/Nonce.hs
+++ b/services/brig/src/Brig/Data/Nonce.hs
@@ -30,11 +30,11 @@ import Imports
 
 insertNonce ::
   (MonadClient m, MonadReader Brig.App.Env m) =>
-  Int32 ->
+  Word32 ->
   Text ->
   Nonce ->
   m ()
-insertNonce ttl key nonce = retry x5 . write insert $ params LocalQuorum (key, nonce, ttl)
+insertNonce ttl key nonce = retry x5 . write insert $ params LocalQuorum (key, nonce, fromIntegral ttl)
   where
     insert :: PrepQuery W (Text, Nonce, Int32) ()
     insert = "INSERT INTO nonce (key, nonce) VALUES (?, ?) USING TTL ?"

(Word* doesn't have Cql instances, or at least I couldn't find any.)

@fisx fisx changed the title Update DB API Make nonces lookuppable by client ids Aug 26, 2022
@fisx fisx mentioned this pull request Aug 26, 2022
6 tasks
@battermann battermann temporarily deployed to cachix August 29, 2022 08:17 Inactive
@battermann battermann temporarily deployed to cachix August 29, 2022 08:17 Inactive
@battermann battermann temporarily deployed to cachix August 29, 2022 08:28 Inactive
@battermann battermann temporarily deployed to cachix August 29, 2022 08:28 Inactive
@battermann battermann temporarily deployed to cachix August 29, 2022 09:05 Inactive
@battermann battermann temporarily deployed to cachix August 29, 2022 09:05 Inactive
:> "nonce"
:> "clients"
:> CaptureClientId "client"
:> "nonce"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

key text PRIMARY KEY,
nonce uuid
) WITH bloom_filter_fp_chance = 0.01
user uuid,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

I was going to say that we wanted to avoid typing the key, but I can't think of a hypothetical use case where asking for a UserId doesn't make sense. So, good, I think.


-- | Convert 'Word32' to 'Int32', with clipping if it doesn't fit.
word32ToInt32 :: Word32 -> Int32
word32ToInt32 = fromIntegral . min (fromIntegral (maxBound @Int32))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i know it's an impractical corner case in this context, but hum... can we use Range to make sure we don't run into this overflow?

@fisx fisx merged commit cf75adb into develop Aug 29, 2022
@fisx fisx deleted the fix-nonce-table branch August 29, 2022 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants