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

WPB-10207 Match cipher suite tag in query parameters against key packages on replacing key packages #4158

Merged
merged 3 commits into from
Jul 17, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/3-bug-fixes/WPB-10207
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Match cipher suite tag in query parameters against key packages on replacing key packages
7 changes: 5 additions & 2 deletions integration/test/API/Brig.hs
Original file line number Diff line number Diff line change
Expand Up @@ -350,13 +350,16 @@ deleteKeyPackages cid kps = do
submit "DELETE" $ req & addJSONObject ["key_packages" .= kps]

replaceKeyPackages :: ClientIdentity -> [Ciphersuite] -> [ByteString] -> App Response
replaceKeyPackages cid suites kps = do
replaceKeyPackages cid suites kps = replaceKeyPackages' cid (Just suites) kps

replaceKeyPackages' :: ClientIdentity -> Maybe [Ciphersuite] -> [ByteString] -> App Response
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention we've been sticking to in cases like this one is to create a data type say ReplaceKeyPackages with a Default instance and redefine replaceKeyPackages as ClientIdentity -> ReplaceKeyPackages -> App Response, so we can have something that resembles named arguments. Having primed method names is confusing and doesn't really scale.

replaceKeyPackages' cid mSuites kps = do
req <-
baseRequest cid Brig Versioned $
"/mls/key-packages/self/" <> cid.client
submit "PUT" $
req
& addQueryParams [("ciphersuites", intercalate "," (map (.code) suites))]
& maybe id (\suites -> addQueryParams [("ciphersuites", intercalate "," (map (.code) suites))]) mSuites
& addJSONObject ["key_packages" .= map (T.decodeUtf8 . Base64.encode) kps]

-- | https://staging-nginz-https.zinfra.io/v6/api/swagger-ui/#/default/get_self
Expand Down
43 changes: 43 additions & 0 deletions integration/test/Test/MLS/KeyPackage.hs
Original file line number Diff line number Diff line change
Expand Up @@ -278,3 +278,46 @@ testReplaceKeyPackages = do

checkCount def 2
checkCount suite 2

do
setMLSCiphersuite def
defKeyPackages <- replicateM 3 (fmap fst (generateKeyPackage alice1))
setMLSCiphersuite suite
suiteKeyPackages <- replicateM 3 (fmap fst (generateKeyPackage alice1))

void
$ replaceKeyPackages' alice1 (Just []) []
`bindResponse` \resp -> do
resp.status `shouldMatchInt` 201

void
$ replaceKeyPackages' alice1 Nothing defKeyPackages
`bindResponse` \resp -> do
resp.status `shouldMatchInt` 201

checkCount def 3
checkCount suite 2

let testErrorCases :: (HasCallStack) => Maybe [Ciphersuite] -> [ByteString] -> App ()
testErrorCases ciphersuites keyPackages = do
void
$ replaceKeyPackages' alice1 ciphersuites keyPackages
`bindResponse` \resp -> do
resp.status `shouldMatchInt` 400
resp.json %. "label" `shouldMatch` "mls-protocol-error"
checkCount def 3
checkCount suite 2

testErrorCases (Just []) defKeyPackages
testErrorCases (Just []) suiteKeyPackages
testErrorCases Nothing []
testErrorCases Nothing suiteKeyPackages
testErrorCases Nothing (suiteKeyPackages <> defKeyPackages)

testErrorCases (Just [suite]) defKeyPackages
testErrorCases (Just [suite]) (suiteKeyPackages <> defKeyPackages)
testErrorCases (Just [suite]) []

testErrorCases (Just [def]) suiteKeyPackages
testErrorCases (Just [def]) (suiteKeyPackages <> defKeyPackages)
testErrorCases (Just [def]) []
19 changes: 16 additions & 3 deletions services/brig/src/Brig/API/MLS/CipherSuite.hs
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@
-- You should have received a copy of the GNU Affero General Public License along
-- with this program. If not, see <https://www.gnu.org/licenses/>.

module Brig.API.MLS.CipherSuite (getCipherSuite, getCipherSuites) where
module Brig.API.MLS.CipherSuite (getCipherSuite, validateCipherSuites) where

import Brig.API.Handler
import Brig.API.MLS.KeyPackages.Validation
import Data.Set qualified as Set
import Imports
import Wire.API.MLS.CipherSuite
import Wire.API.MLS.KeyPackage
import Wire.API.MLS.Serialisation

getOneCipherSuite :: CipherSuite -> Handler r CipherSuiteTag
getOneCipherSuite s =
Expand All @@ -32,5 +35,15 @@ getOneCipherSuite s =
getCipherSuite :: Maybe CipherSuite -> Handler r CipherSuiteTag
getCipherSuite = maybe (pure defCipherSuite) getOneCipherSuite

getCipherSuites :: Maybe [CipherSuite] -> Handler r [CipherSuiteTag]
getCipherSuites = maybe (pure [defCipherSuite]) (traverse getOneCipherSuite)
validateCipherSuites ::
Maybe [CipherSuite] ->
KeyPackageUpload ->
Handler r (Set CipherSuiteTag)
validateCipherSuites suites upload = do
suitesQuery <- Set.fromList <$> maybe (pure [defCipherSuite]) (traverse getOneCipherSuite) suites
when (any isNothing suitesKPM) . void $ mlsProtocolError "uploaded key packages contains unsupported cipher suite"
unless (suitesQuery == suitesKP) . void $ mlsProtocolError "uploaded key packages for unannounced cipher suites"
pure suitesQuery
where
suitesKPM = map (cipherSuiteTag . (.cipherSuite) . value) upload.keyPackages
suitesKP = Set.fromList $ catMaybes suitesKPM
2 changes: 1 addition & 1 deletion services/brig/src/Brig/API/MLS/KeyPackages.hs
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,6 @@ replaceKeyPackages ::
Handler r ()
replaceKeyPackages lusr c (fmap toList -> mSuites) upload = do
assertMLSEnabled
suites <- getCipherSuites mSuites
suites <- validateCipherSuites mSuites upload
lift $ wrapClient (Data.deleteAllKeyPackages (tUnqualified lusr) c suites)
uploadKeyPackages lusr c upload
4 changes: 2 additions & 2 deletions services/brig/src/Brig/Data/MLS/KeyPackage.hs
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ deleteKeyPackages u c suite refs =
deleteQuery = "DELETE FROM mls_key_packages WHERE user = ? AND client = ? AND cipher_suite = ? AND ref in ?"

deleteAllKeyPackages ::
(MonadClient m, MonadUnliftIO m) =>
(MonadClient m, MonadUnliftIO m, Foldable f) =>
UserId ->
ClientId ->
[CipherSuiteTag] ->
f CipherSuiteTag ->
m ()
deleteAllKeyPackages u c suites =
pooledForConcurrentlyN_ 16 suites $ \suite ->
Expand Down
Loading