diff --git a/changelog.d/5-internal/map-federation-tests b/changelog.d/5-internal/map-federation-tests new file mode 100644 index 0000000000..b53c1449dd --- /dev/null +++ b/changelog.d/5-internal/map-federation-tests @@ -0,0 +1 @@ +Tag integration tests for security audit. \ No newline at end of file diff --git a/services/brig/docs/swagger.md b/services/brig/docs/swagger.md index 63ec978afb..59e6c1c3b8 100644 --- a/services/brig/docs/swagger.md +++ b/services/brig/docs/swagger.md @@ -54,7 +54,7 @@ An error in this category likely indicates an issue with the configuration of fe - **Federation unavailable** (status: 500, label: `federation-not-available`): Federation is configured for this backend, but the local federator cannot be reached. This can be transient, so clients should retry the request. - **Federation not implemented** (status: 500, label: `federation-not-implemented`): Federated behaviour for a certain endpoint is not yet implemented. - - **Federator discovery failed** (status: 500, label: `discovery-failure`): A DNS error occurred during discovery of a remote backend. This can be transient, so clients should retry the request. + - **Federator discovery failed** (status: 400, label: `discovery-failure`): A DNS error occurred during discovery of a remote backend. This can be transient, so clients should retry the request. - **Local federation error** (status: 500, label: `federation-local-error`): An error occurred in the communication between this backend and its local federator. These errors are most likely caused by bugs in the backend, and should be reported as such. ### Remote federation errors diff --git a/services/federator/src/Federator/Discovery.hs b/services/federator/src/Federator/Discovery.hs index 65844e33d8..4148e67c13 100644 --- a/services/federator/src/Federator/Discovery.hs +++ b/services/federator/src/Federator/Discovery.hs @@ -49,7 +49,7 @@ instance AsWai DiscoveryFailure where where (status, label) = case e of DiscoveryFailureSrvNotAvailable _ -> (HTTP.status422, "invalid-domain") - DiscoveryFailureDNSError _ -> (HTTP.status500, "discovery-failure") + DiscoveryFailureDNSError _ -> (HTTP.status400, "discovery-failure") waiErrorDescription :: DiscoveryFailure -> Text waiErrorDescription (DiscoveryFailureSrvNotAvailable msg) = "srv record not found: " <> Text.decodeUtf8 msg diff --git a/services/federator/src/Federator/Validation.hs b/services/federator/src/Federator/Validation.hs index 4d7f35867d..6dc9e21ebd 100644 --- a/services/federator/src/Federator/Validation.hs +++ b/services/federator/src/Federator/Validation.hs @@ -150,6 +150,7 @@ validateDomain :: validateDomain Nothing _ = throw NoClientCertificate validateDomain (Just encodedCertificate) unparsedDomain = do targetDomain <- parseDomain unparsedDomain + ensureCanFederateWith targetDomain -- run discovery to find the hostname of the client federator certificate <- @@ -160,7 +161,7 @@ validateDomain (Just encodedCertificate) unparsedDomain = do unless (any null validationErrors) $ throw $ AuthenticationFailure validationErrors - ensureCanFederateWith targetDomain $> targetDomain + pure targetDomain -- | Match a hostname against the domain names of a certificate. -- diff --git a/services/federator/test/integration/Test/Federator/IngressSpec.hs b/services/federator/test/integration/Test/Federator/IngressSpec.hs index 08d989b647..34b6af1d32 100644 --- a/services/federator/test/integration/Test/Federator/IngressSpec.hs +++ b/services/federator/test/integration/Test/Federator/IngressSpec.hs @@ -46,6 +46,8 @@ import Wire.API.Federation.Domain import Wire.API.User import Wire.Network.DNS.SRV +-- | This module contains tests for the interface between federator and ingress. Ingress is +-- mocked with nginz. spec :: TestEnv -> Spec spec env = do describe "Ingress" $ do @@ -68,7 +70,16 @@ spec env = do responseStatusCode resp `shouldBe` HTTP.status200 actualProfile `shouldBe` (Just expectedProfile) - it "should not be accessible without a client certificate" $ + -- @SF.Federation @TSFI.RESTfulAPI @S2 @S3 @S7 + -- + -- This test was primarily intended to test that federator is using the API right (header + -- name etc.), but it is also effectively testing that federator rejects clients without + -- certificates that have been validated by ingress. + -- + -- We can't test end-to-end here: the TLS termination happens in k8s, and would have to be + -- tested there (and with a good emulation of the concrete configuration of the prod + -- system). + it "rejectRequestsWithoutClientCertIngress" $ runTestFederator env $ do brig <- view teBrig <$> ask user <- randomUser brig @@ -93,6 +104,10 @@ spec env = do expectationFailure "Expected client certificate error, got remote error" Left (RemoteErrorResponse _ status _) -> status `shouldBe` HTTP.status400 +-- FUTUREWORK: ORMOLU_DISABLE +-- @END +-- ORMOLU_ENABLE + runTestSem :: Sem '[Input TestEnv, Embed IO] a -> TestFederator IO a runTestSem action = do e <- ask diff --git a/services/federator/test/integration/Test/Federator/InwardSpec.hs b/services/federator/test/integration/Test/Federator/InwardSpec.hs index da02ec6f45..3ef28b069d 100644 --- a/services/federator/test/integration/Test/Federator/InwardSpec.hs +++ b/services/federator/test/integration/Test/Federator/InwardSpec.hs @@ -39,8 +39,11 @@ import Wire.API.Federation.Domain import Wire.API.User -- FUTUREWORK(federation): move these tests to brig-integration (benefit: avoid duplicating all of the brig helper code) +-- FUTUREWORK(fisx): better yet, reorganize integration tests (or at least the helpers) so +-- they don't spread out over the different sevices. --- | Path covered by this test +-- | This module contains tests for the interface between federator and brig. The tests call +-- federator directly, circumnventing ingress: -- -- +----------+ -- |federator-| +------+--+ @@ -72,6 +75,14 @@ spec env = view teTstOpts hdl <- randomHandle @@ -110,6 +123,10 @@ spec env = (encode hdl) !!! const 403 === statusCode +-- TODO: ORMOLU_DISABLE +-- @END +-- ORMOLU_ENABLE + inwardCallWithHeaders :: (MonadIO m, MonadHttp m, MonadReader TestEnv m, HasCallStack) => ByteString -> @@ -132,8 +149,17 @@ inwardCall :: LBS.ByteString -> m (Response (Maybe LByteString)) inwardCall requestPath payload = do + originDomain :: Text <- cfgOriginDomain <$> view teTstOpts + inwardCallWithOriginDomain (toByteString' originDomain) requestPath payload + +inwardCallWithOriginDomain :: + (MonadIO m, MonadHttp m, MonadReader TestEnv m, HasCallStack) => + ByteString -> + ByteString -> + LBS.ByteString -> + m (Response (Maybe LByteString)) +inwardCallWithOriginDomain originDomain requestPath payload = do Endpoint fedHost fedPort <- cfgFederatorExternal <$> view teTstOpts - originDomain <- cfgOriginDomain <$> view teTstOpts clientCertFilename <- clientCertificate . optSettings . view teOpts <$> ask clientCert <- liftIO $ BS.readFile clientCertFilename post @@ -141,6 +167,6 @@ inwardCall requestPath payload = do . port fedPort . path requestPath . header "X-SSL-Certificate" (HTTP.urlEncode True clientCert) - . header originDomainHeaderName (toByteString' originDomain) + . header originDomainHeaderName originDomain . bytes (toByteString' payload) ) diff --git a/services/federator/test/unit/Test/Federator/InternalServer.hs b/services/federator/test/unit/Test/Federator/InternalServer.hs index db3393c529..b384d0b88f 100644 --- a/services/federator/test/unit/Test/Federator/InternalServer.hs +++ b/services/federator/test/unit/Test/Federator/InternalServer.hs @@ -99,6 +99,9 @@ federatedRequestSuccess = body <- Wai.lazyResponseBody res body @?= "\"bar\"" +-- @SF.Federation @TSFI.RESTfulAPI @S2 @S3 @S7 +-- +-- Refuse to send outgoing request to non-included domain when allowlist is configured. federatedRequestFailureAllowList :: TestTree federatedRequestFailureAllowList = testCase "should not make a call when target domain not in the allowList" $ do @@ -134,3 +137,5 @@ federatedRequestFailureAllowList = . runInputConst settings $ callOutward request eith @?= Left (FederationDenied targetDomain) + +-- @END diff --git a/services/federator/test/unit/Test/Federator/Options.hs b/services/federator/test/unit/Test/Federator/Options.hs index 3bf5930a2d..9034586273 100644 --- a/services/federator/test/unit/Test/Federator/Options.hs +++ b/services/federator/test/unit/Test/Federator/Options.hs @@ -171,7 +171,8 @@ testSettings = assertFailure $ "expected failure for non-existing client certificate, got: " <> show (tlsSettings ^. creds), - testCase "fail on invalid certificate" $ do + -- @SF.Federation @TSFI.RESTfulAPI @S3 @S7 + testCase "failToStartWithInvalidServerCredentials" $ do let settings = defRunSettings "test/resources/unit/invalid.pem" @@ -193,6 +194,7 @@ testSettings = assertFailure $ "expected failure for invalid client certificate, got: " <> show (tlsSettings ^. creds), + -- @END testCase "fail on invalid private key" $ do let settings = defRunSettings diff --git a/services/federator/test/unit/Test/Federator/Response.hs b/services/federator/test/unit/Test/Federator/Response.hs index 8413b9b95a..b8addefb8d 100644 --- a/services/federator/test/unit/Test/Federator/Response.hs +++ b/services/federator/test/unit/Test/Federator/Response.hs @@ -72,7 +72,7 @@ testDiscoveryFailure = throw (DiscoveryFailureDNSError "mock error") body <- Wai.lazyResponseBody resp let merr = Aeson.decode body - Wai.responseStatus resp @?= HTTP.status500 + Wai.responseStatus resp @?= HTTP.status400 fmap Wai.label merr @?= Just "discovery-failure" testRemoteError :: TestTree diff --git a/services/federator/test/unit/Test/Federator/Validation.hs b/services/federator/test/unit/Test/Federator/Validation.hs index 4c4597a6c5..785757927f 100644 --- a/services/federator/test/unit/Test/Federator/Validation.hs +++ b/services/federator/test/unit/Test/Federator/Validation.hs @@ -113,6 +113,9 @@ validateDomainAllowListFailSemantic = $ validateDomain (Just exampleCert) "invalid//.><-semantic-&@-domain" res @?= Left (DomainParseError "invalid//.><-semantic-&@-domain") +-- @SF.Federation @TSFI.RESTfulAPI @S2 @S3 @S7 +-- +-- Refuse to send outgoing request to non-included domain when allowlist is configured. validateDomainAllowListFail :: TestTree validateDomainAllowListFail = testCase "allow list validation" $ do @@ -127,6 +130,8 @@ validateDomainAllowListFail = $ validateDomain (Just exampleCert) "localhost.example.com" res @?= Left (FederationDenied (Domain "localhost.example.com")) +-- @END + validateDomainAllowListSuccess :: TestTree validateDomainAllowListSuccess = testCase "should give parsed domain if in the allow list" $ do @@ -153,6 +158,7 @@ validateDomainCertMissing = $ validateDomain Nothing "foo.example.com" res @?= Left NoClientCertificate +-- @SF.Federation @TSFI.RESTfulAPI @S2 @S3 @S7 validateDomainCertInvalid :: TestTree validateDomainCertInvalid = testCase "should fail if the client certificate is invalid" $ do @@ -164,6 +170,12 @@ validateDomainCertInvalid = $ validateDomain (Just "not a certificate") "foo.example.com" res @?= Left (CertificateParseError "no certificate found") +-- @END + +-- @SF.Federation @TSFI.RESTfulAPI @S3 @S7 +-- +-- Reject request if the infrastructure domain in the client cert does not match the backend +-- domain in the `Wire-origin-domain` header. validateDomainCertWrongDomain :: TestTree validateDomainCertWrongDomain = testCase "should fail if the client certificate has a wrong domain" $ do @@ -176,6 +188,8 @@ validateDomainCertWrongDomain = $ validateDomain (Just exampleCert) "foo.example.com" res @?= Left (AuthenticationFailure (pure [X509.NameMismatch "foo.example.com"])) +-- @END + validateDomainCertCN :: TestTree validateDomainCertCN = testCase "should succeed if the certificate has subject CN but no SAN" $ do diff --git a/services/spar/test-integration/Test/Spar/APISpec.hs b/services/spar/test-integration/Test/Spar/APISpec.hs index af466b1650..7d73042cf0 100644 --- a/services/spar/test-integration/Test/Spar/APISpec.hs +++ b/services/spar/test-integration/Test/Spar/APISpec.hs @@ -482,7 +482,9 @@ specFinalizeLogin = do (cs . fromJust . responseBody $ sparresp) `shouldContain` "wire:sso:error:forbidden" check mkareq mkaresp submitaresp checkresp + -- {- ORMOLU_DISABLE -} -- FUTUREWORK: try a newer release of ormolu? -- @END + -- {- ORMOLU_ENABLE -} context "IdP changes response format" $ do it "treats NameId case-insensitively" $ do