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

Hash SCIM tokens #1240

Merged
merged 11 commits into from
Nov 12, 2020
Merged

Hash SCIM tokens #1240

merged 11 commits into from
Nov 12, 2020

Conversation

smatting
Copy link
Contributor

@smatting smatting commented Oct 28, 2020

Currently SCIM tokens are persisted in plain text. Fixes https://github.com/zinfra/backend-issues/issues/1652

With this PR new SCIM tokens will only have their SHA512 hashes persisted.
Existing tokens (still persisted in plaintext) will remain valid, but will be re-persisted with their hash on every validation.

The columns team_provisioning_by_token.token_, team_provisioning_by_team.token_ which contained the plaintext token will be repurposed and also contain the hash value.
To distinguish plaintext from hash values the hash values are prefixed with sha512:. In Haskell these two types are represented as the sum type

data ScimTokenLookupKey
  = ScimTokenLookupKeyHashed ScimTokenHash
  | ScimTokenLookupKeyPlaintext ScimToken
  deriving (Eq, Show)

This way all plaintext tokens which are regularly used should be converted to their hash. The remaining tokens should be converted via batch process or invalidated.

  • create follow-up issue for that

@smatting smatting marked this pull request as ready for review October 29, 2020 10:17
@smatting smatting force-pushed the smatting/scim-tokens branch from 75ebe88 to f374253 Compare October 29, 2020 12:26
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

👍 (some comments)

parser = string "sha512:" *> (ScimTokenHash <$> parser)

instance ToByteString ScimTokenHash where
builder (ScimTokenHash t) = BB.fromByteString "sha512:" <> builder t
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a roundtrip test, like these?

(I admit that there are more gaps in the roundtrip test coverage, but we might as well start now to patch them...)

Copy link
Contributor

Choose a reason for hiding this comment

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

(we can also do that in a separate PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added roundtrip test

services/spar/src/Spar/Data.hs Outdated Show resolved Hide resolved
where
sel :: PrepQuery R (Identity ScimToken) ScimTokenRow
let tokenHash = hashScimToken token
mbRow <- retry x1 . query1 sel $ params Quorum (tokenHash, token)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can return both hashed and unhashed in any order, if both are still in the DB (race condition).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Found two things (both harmless). Integration tests pass locally. :shipit:

services/spar/src/Spar/Data.hs Outdated Show resolved Hide resolved
services/spar/src/Spar/Data.hs Outdated Show resolved Hide resolved
@fisx
Copy link
Contributor

fisx commented Nov 5, 2020

Found two things (both harmless). Integration tests pass locally. :shipit:

That was wrong: the stray delete query risks a race condition where a token that should be available is already gone in unhashed from and not there yet in hashed form. Not very likely to materialize, but please fix it anyway.

smatting and others added 4 commits November 6, 2020 10:32
Co-authored-by: fisx <mf@zerobuzz.net>
Co-authored-by: fisx <mf@zerobuzz.net>
@smatting
Copy link
Contributor Author

1 out of 247 tests failed (356.63s)
The failing test is unrelated to this PR: https://github.com/zinfra/backend-issues/issues/1875

@smatting smatting merged commit 48607a7 into develop Nov 12, 2020
@smatting smatting deleted the smatting/scim-tokens branch November 12, 2020 10:24
@jschaul jschaul mentioned this pull request Nov 24, 2020
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.

2 participants