From 21c8e8dd8c8b74f3313b44c277a9cf5969063538 Mon Sep 17 00:00:00 2001
From: Paolo Capriotti <paolo@capriotti.io>
Date: Mon, 19 Jul 2021 10:43:58 +0200
Subject: [PATCH 1/5] Document federation errors
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Co-authored-by: Marko Dimjašević <marko.dimjasevic@wire.com>
---
 services/brig/src/Brig/API/Public.hs | 54 ++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/services/brig/src/Brig/API/Public.hs b/services/brig/src/Brig/API/Public.hs
index f8741a7b84..64ebcece92 100644
--- a/services/brig/src/Brig/API/Public.hs
+++ b/services/brig/src/Brig/API/Public.hs
@@ -163,6 +163,60 @@ Okta will ask you to provide two URLs when you set it up for talking to wireapp:
 #### centrify.com
 
 Centrify allows you to upload the metadata xml document that you get from the `/sso/metadata` end-point.  You can also enter the metadata url and have centrify retrieve the xml, but to guarantee integrity of the setup, the metadata should be copied from the team settings page and pasted into the centrify setup page without any URL indirections.
+
+## Federation errors
+
+Endpoints involving federated calls to other domains can return some extra failure responses, common to all endpoints. Instead of listing them as possible responses for each endpoint, we document them here.
+
+**Note**: when a failure occurs as a result of making a federated RPC to another backend, the error response contains the following extra fields:
+
+ - `domain`: the target backend of the RPC that failed;
+ - `path`: the path of the RPC that failed.
+
+### Local federation errors
+
+An error in this category indicates an issue with configuration of federation on the local backend or with the federation-related content of the current client request.
+
+ - **Federation not enabled** (status: 400, label: `federation-not-enabled`): Federation has not been configured for this backend.
+ - **Federation unavailable** (status: 500, label: `federation-not-available`): Federation is configured for this backend, but the local federator cannot be reached.
+ - **Federation not implemented** (status: 403, label: `federation-not-implemented`): Federated behaviour for a certain endpoint is not yet implemented.
+ - **Remote backend not found** (status: 422, label: `srv-record-not-found`): This backend attempted to contact a backend which does not exist or is not properly configured.
+ - **Federation denied locally** (status: 400, label: `federation-not-allowed`): This backend attempted an RPC to a non-whitelisted backend.
+ - **Federator discovery failed** (status: 500, label: `srv-lookup-dns-error`): A DNS error occurred during discovery of a remote backend.
+
+### Remote federation errors
+
+Errors in this category are returned in case of communication issues between the local backend and a remote one, or if the remote side encountered an error while processing an RPC.
+
+ - **RPC error with code** (status: 533, label: `grpc-error-code`): An RPC to a remote federator resulted in an error. The RPC error code can be found in the error message.
+ - **RPC error with string** (status: 533, label: `grpc-error-string`): An RPC to a remote federator resulted in an error. A description of the error can be found in the error message.
+ - **RPC error** (status: 500, label: `federation-rpc-error`): There was a non-specified error in the RPC from this backend to another backend. Check the error message for more details.
+ - **Connection refused** (status: 521, label: `cannot-connect-to-remote-federator`): The local federator could not connect to a remote one.
+ - **Too much concurrency** (status: 533, label: `too-much-concurrency`): Too many concurrent requests on a remote backend.
+ - **Unknown remote error** (status: 500, label: `unknown-federation-error`): An RPC failed but no specific error was returned by the remote side. Check the error message for more details.
+
+### Backend compatibility errors
+
+An error in this category will be returned when this backend makes an invalid or unsupported RPC to another backend. This can indicate some incompatibility between backends or a backend bug.
+
+ - **Version mismatch** (status: 531): A remote backend is running an unsupported version of the federator.
+ - **Invalid method** / **Streaming not supported** (status: 500, label: `federation-invalid-call`): This backend attempted an invalid RPC to another backend.
+ - **Invalid request** (status: 500, label: `invalid-request-to-federator`): The local federator made an invalid request to a remote one. Check the error message for more details.
+ - **RPC client error** (status: 533, label: `grpc-client-error`): The current federator encountered an error when making an RPC to a remote one.
+ - **Invalid content type** (status: 503, label: `federation-invalid-content-type-header`): An RPC to another backend returned an invalid content type.
+ - **Unsupported content type** (status: 503, label: `federation-unsupported-content-type`): An RPC to another backend returned an unsupported content type.
+ - **Invalid origin domain** (status: 533, label: `invalid-origin-domain`): The current backend attempted an RPC with an invalid origin domain field.
+ - **Forbidden endpoint** (status: 533, label: `forbidden-endpoint`): The current backend attempted an RPC to a forbidden or inaccessible remote endpoint.
+ - **Unknown federation error** (status: 503, label: `unknown-federation-error): The target of an RPC returned an unexpected reponse. Check the error message for more details.
+
+### Authentication errors
+
+The errors in this category relate to authentication or authorization issues between backends.
+
+ - **TLS failure**: (status: 525): An error occurred during the TLS handshake between the local federator and a remote one.
+ - **Invalid certificate** (status: 526): The TLS certificate on the remote end of an RPC is invalid.
+ - **Federation denied remotely** (status: 532): The current backend made an unauthorised request to a remote one.
+
 |]
 
 servantSitemap :: ServerT ServantAPI Handler

From f5549918360c0f23d2d9b465b00c21575a9073e8 Mon Sep 17 00:00:00 2001
From: Paolo Capriotti <paolo@capriotti.io>
Date: Mon, 19 Jul 2021 10:50:29 +0200
Subject: [PATCH 2/5] Update CHANGELOG

---
 CHANGELOG-draft.md | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/CHANGELOG-draft.md b/CHANGELOG-draft.md
index a54edb4366..80d35c4561 100644
--- a/CHANGELOG-draft.md
+++ b/CHANGELOG-draft.md
@@ -18,6 +18,8 @@ THIS FILE ACCUMULATES THE RELEASE NOTES FOR THE UPCOMING RELEASE.
 
 ## Documentation
 
+* Added documentation of federation errors (#1674)
+
 ## Internal changes
 
 * Rewrite the `POST /connections` endpoint to Servant (#1726)

From e99f3cf6608d1ebe65ab15fe5e7f71e1ee9b2ecb Mon Sep 17 00:00:00 2001
From: Paolo Capriotti <paolo@capriotti.io>
Date: Thu, 22 Jul 2021 17:25:30 +0200
Subject: [PATCH 3/5] Remove `InvalidCertificate` federation error

It is currently not so easy to distinguish this particular error from a
generic TLS error (see #1662 for more context).

Since `InvalidCertificate` is never thrown, this PR simply removes it.
Note that this is a breaking change in the federation protobuf.
---
 libs/wire-api-federation/proto/router.proto           | 11 +++++------
 .../src/Wire/API/Federation/Error.hs                  |  1 -
 .../src/Wire/API/Federation/GRPC/Types.hs             |  1 -
 services/brig/src/Brig/API/Public.hs                  |  3 +--
 services/brig/test/unit/Test/Brig/API/Error.hs        |  2 --
 5 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/libs/wire-api-federation/proto/router.proto b/libs/wire-api-federation/proto/router.proto
index c6511364cf..f8f1bd399a 100644
--- a/libs/wire-api-federation/proto/router.proto
+++ b/libs/wire-api-federation/proto/router.proto
@@ -37,12 +37,11 @@ message OutwardError {
     DiscoveryFailed = 1;
     ConnectionRefused = 2;
     TLSFailure = 3;
-    InvalidCertificate = 4;
-    VersionMismatch = 5;
-    FederationDeniedByRemote = 6;
-    FederationDeniedLocally = 7;
-    RemoteFederatorError = 8;
-    InvalidRequest = 9;
+    VersionMismatch = 4;
+    FederationDeniedByRemote = 5;
+    FederationDeniedLocally = 6;
+    RemoteFederatorError = 7;
+    InvalidRequest = 8;
   }
 
   ErrorType type = 1;
diff --git a/libs/wire-api-federation/src/Wire/API/Federation/Error.hs b/libs/wire-api-federation/src/Wire/API/Federation/Error.hs
index bc0ed03bed..21ce8de569 100644
--- a/libs/wire-api-federation/src/Wire/API/Federation/Error.hs
+++ b/libs/wire-api-federation/src/Wire/API/Federation/Error.hs
@@ -157,7 +157,6 @@ federationRemoteError err = Wai.mkError status (LT.fromStrict label) (LT.fromStr
       Proto.DiscoveryFailed -> HTTP.status500
       Proto.ConnectionRefused -> HTTP.Status 521 "Web Server Is Down"
       Proto.TLSFailure -> HTTP.Status 525 "SSL Handshake Failure"
-      Proto.InvalidCertificate -> HTTP.Status 526 "Invalid SSL Certificate"
       Proto.VersionMismatch -> HTTP.Status 531 "Version Mismatch"
       Proto.FederationDeniedByRemote -> HTTP.Status 532 "Federation Denied"
       Proto.FederationDeniedLocally -> HTTP.status400
diff --git a/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs b/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs
index 8beff443ac..a39fa8278e 100644
--- a/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs
+++ b/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs
@@ -119,7 +119,6 @@ data OutwardErrorType
   | DiscoveryFailed
   | ConnectionRefused
   | TLSFailure
-  | InvalidCertificate
   | VersionMismatch
   | FederationDeniedByRemote
   | FederationDeniedLocally
diff --git a/services/brig/src/Brig/API/Public.hs b/services/brig/src/Brig/API/Public.hs
index 64ebcece92..add083e895 100644
--- a/services/brig/src/Brig/API/Public.hs
+++ b/services/brig/src/Brig/API/Public.hs
@@ -213,8 +213,7 @@ An error in this category will be returned when this backend makes an invalid or
 
 The errors in this category relate to authentication or authorization issues between backends.
 
- - **TLS failure**: (status: 525): An error occurred during the TLS handshake between the local federator and a remote one.
- - **Invalid certificate** (status: 526): The TLS certificate on the remote end of an RPC is invalid.
+ - **TLS failure**: (status: 525): An error occurred during the TLS handshake between the local federator and a remote one. This is most likely due to an issue with the certificate on the remote end.
  - **Federation denied remotely** (status: 532): The current backend made an unauthorised request to a remote one.
 
 |]
diff --git a/services/brig/test/unit/Test/Brig/API/Error.hs b/services/brig/test/unit/Test/Brig/API/Error.hs
index 10ee697bb3..d29463984e 100644
--- a/services/brig/test/unit/Test/Brig/API/Error.hs
+++ b/services/brig/test/unit/Test/Brig/API/Error.hs
@@ -58,8 +58,6 @@ testOutwardError =
             assertOutwardErrorStatus Proto.ConnectionRefused 521,
           testCase "when TLS fails" $
             assertOutwardErrorStatus Proto.TLSFailure 525,
-          testCase "when remote certificate is invalid" $
-            assertOutwardErrorStatus Proto.InvalidCertificate 526,
           testCase "when remote returns version mismatch" $
             assertOutwardErrorStatus Proto.VersionMismatch 531,
           testCase "when remote denies federation" $

From 018bfa2f115804ad812abcbda9487fafc5183bd7 Mon Sep 17 00:00:00 2001
From: Paolo Capriotti <paolo@capriotti.io>
Date: Mon, 2 Aug 2021 10:56:39 +0200
Subject: [PATCH 4/5] Remove labels from protobuf errors

---
 libs/wire-api-federation/proto/router.proto   | 12 ++--
 .../src/Wire/API/Federation/Error.hs          | 48 +++++++-------
 .../src/Wire/API/Federation/GRPC/Types.hs     | 25 +++++---
 services/brig/src/Brig/API/Public.hs          |  8 +--
 .../brig/test/unit/Test/Brig/API/Error.hs     | 26 +++-----
 .../federator/src/Federator/InternalServer.hs | 63 ++++++++++++++-----
 .../unit/Test/Federator/InternalServer.hs     | 10 +--
 services/galley/test/integration/API.hs       |  2 +-
 services/galley/test/integration/API/Util.hs  |  2 +-
 9 files changed, 108 insertions(+), 88 deletions(-)

diff --git a/libs/wire-api-federation/proto/router.proto b/libs/wire-api-federation/proto/router.proto
index f8f1bd399a..e3c253dd4b 100644
--- a/libs/wire-api-federation/proto/router.proto
+++ b/libs/wire-api-federation/proto/router.proto
@@ -40,12 +40,13 @@ message OutwardError {
     VersionMismatch = 4;
     FederationDeniedByRemote = 5;
     FederationDeniedLocally = 6;
-    RemoteFederatorError = 7;
-    InvalidRequest = 8;
+    TooMuchConcurrency = 7;
+    GrpcError = 8;
+    InvalidRequest = 9;
   }
 
   ErrorType type = 1;
-  ErrorPayload payload = 2;
+  string msg = 2;
 }
 
 message InwardError {
@@ -62,11 +63,6 @@ message InwardError {
   string msg = 2;
 }
 
-message ErrorPayload {
-  string label = 1;
-  string msg = 2;
-}
-
 // The envelope message which is sent from brig to the Outward service of a local federator
 message FederatedRequest {
   string domain = 1;
diff --git a/libs/wire-api-federation/src/Wire/API/Federation/Error.hs b/libs/wire-api-federation/src/Wire/API/Federation/Error.hs
index 21ce8de569..6aa875b608 100644
--- a/libs/wire-api-federation/src/Wire/API/Federation/Error.hs
+++ b/libs/wire-api-federation/src/Wire/API/Federation/Error.hs
@@ -38,7 +38,11 @@ federationErrorToWai FederationNotImplemented = federationNotImplemented
 federationErrorToWai FederationNotConfigured = federationNotConfigured
 federationErrorToWai (FederationCallFailure failure) = addErrorData $
   case fedFailError failure of
-    FederationClientRPCError msg -> federationRpcError msg
+    FederationClientRPCError msg ->
+      Wai.mkError
+        HTTP.status500
+        "client-rpc-error"
+        (LT.fromStrict msg)
     FederationClientInvalidMethod mth ->
       federationInvalidCall
         ("Unexpected method: " <> LT.fromStrict (T.decodeUtf8 mth))
@@ -117,13 +121,6 @@ federationNotConfigured =
     "federation-not-enabled"
     "no federator configured"
 
-federationRpcError :: Text -> Wai.Error
-federationRpcError msg =
-  Wai.mkError
-    HTTP.status500
-    "federation-rpc-error"
-    (LT.fromStrict msg)
-
 federationUnavailable :: Text -> Wai.Error
 federationUnavailable err =
   Wai.mkError
@@ -144,24 +141,25 @@ federationRemoteInwardError err = Wai.mkError status (LT.fromStrict label) (LT.f
       Proto.IOther -> (unexpectedFederationResponseStatus, "inward-other")
 
 federationRemoteError :: Proto.OutwardError -> Wai.Error
-federationRemoteError err = Wai.mkError status (LT.fromStrict label) (LT.fromStrict msg)
+federationRemoteError err = case Proto.outwardErrorType err of
+  Proto.RemoteNotFound -> mkErr HTTP.status422 "srv-record-not-found"
+  Proto.DiscoveryFailed -> mkErr HTTP.status500 "srv-lookup-dns-error"
+  Proto.ConnectionRefused ->
+    mkErr
+      (HTTP.Status 521 "Web Server Is Down")
+      "cannot-connect-to-remote-federator"
+  Proto.TLSFailure -> mkErr (HTTP.Status 525 "SSL Handshake Failure") "tls-failure"
+  Proto.VersionMismatch -> mkErr (HTTP.Status 531 "Version Mismatch") "version-mismatch"
+  Proto.FederationDeniedByRemote ->
+    mkErr
+      (HTTP.Status 532 "Federation Denied")
+      "federation-denied-remotely"
+  Proto.FederationDeniedLocally -> mkErr HTTP.status400 "federation-not-allowed"
+  Proto.TooMuchConcurrency -> mkErr unexpectedFederationResponseStatus "too-much-concurrency"
+  Proto.GrpcError -> mkErr unexpectedFederationResponseStatus "grpc-error"
+  Proto.InvalidRequest -> mkErr HTTP.status500 "invalid-request-to-federator"
   where
-    decodeError :: Maybe Proto.ErrorPayload -> (Text, Text)
-    decodeError Nothing = ("unknown-federation-error", "Unknown federation error")
-    decodeError (Just (Proto.ErrorPayload label' msg')) = (label', msg')
-
-    (label, msg) = decodeError (Proto.outwardErrorPayload err)
-
-    status = case Proto.outwardErrorType err of
-      Proto.RemoteNotFound -> HTTP.status422
-      Proto.DiscoveryFailed -> HTTP.status500
-      Proto.ConnectionRefused -> HTTP.Status 521 "Web Server Is Down"
-      Proto.TLSFailure -> HTTP.Status 525 "SSL Handshake Failure"
-      Proto.VersionMismatch -> HTTP.Status 531 "Version Mismatch"
-      Proto.FederationDeniedByRemote -> HTTP.Status 532 "Federation Denied"
-      Proto.FederationDeniedLocally -> HTTP.status400
-      Proto.RemoteFederatorError -> unexpectedFederationResponseStatus
-      Proto.InvalidRequest -> HTTP.status500
+    mkErr status label = Wai.mkError status label (LT.fromStrict (Proto.outwardErrorMessage err))
 
 federationInvalidCall :: LText -> Wai.Error
 federationInvalidCall = Wai.mkError HTTP.status500 "federation-invalid-call"
diff --git a/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs b/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs
index a39fa8278e..cd59dd25d4 100644
--- a/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs
+++ b/libs/wire-api-federation/src/Wire/API/Federation/GRPC/Types.hs
@@ -26,6 +26,7 @@ module Wire.API.Federation.GRPC.Types where
 import Data.Domain (Domain (..), domainText, mkDomain)
 import Data.Either.Validation
 import Data.List.NonEmpty (NonEmpty ((:|)))
+import qualified Data.Text as Text
 import Imports
 import Mu.Quasi.GRpc (grpc)
 import Mu.Schema
@@ -101,12 +102,12 @@ instance FromSchema Router "OutwardResponse" OutwardResponse where
 -- https://higherkindness.io/mu-haskell/schema/#mapping-haskell-types
 type OutwardErrorFieldMapping =
   '[ "outwardErrorType" ':-> "type",
-     "outwardErrorPayload" ':-> "payload"
+     "outwardErrorMessage" ':-> "msg"
    ]
 
 data OutwardError = OutwardError
   { outwardErrorType :: OutwardErrorType,
-    outwardErrorPayload :: Maybe ErrorPayload
+    outwardErrorMessage :: Text
   }
   deriving (Typeable, Show, Eq, Generic)
   deriving (Arbitrary) via (GenericUniform OutwardError)
@@ -122,18 +123,12 @@ data OutwardErrorType
   | VersionMismatch
   | FederationDeniedByRemote
   | FederationDeniedLocally
-  | RemoteFederatorError
+  | TooMuchConcurrency
+  | GrpcError
   | InvalidRequest
   deriving (Typeable, Show, Eq, Generic, ToSchema Router "OutwardError.ErrorType", FromSchema Router "OutwardError.ErrorType")
   deriving (Arbitrary) via (GenericUniform OutwardErrorType)
 
-data ErrorPayload = ErrorPayload
-  { label :: Text,
-    msg :: Text
-  }
-  deriving (Typeable, Show, Eq, Generic, ToSchema Router "ErrorPayload", FromSchema Router "ErrorPayload")
-  deriving (Arbitrary) via (GenericUniform ErrorPayload)
-
 -- See mu-haskell Custom Mapping documentation here:
 -- https://higherkindness.io/mu-haskell/schema/#mapping-haskell-types
 type InwardErrorFieldMapping =
@@ -189,6 +184,16 @@ data ValidatedFederatedRequest = ValidatedFederatedRequest
   }
   deriving (Typeable, Eq, Show)
 
+showFederatedRequestValidationError :: FederatedRequestValidationError -> Text
+showFederatedRequestValidationError (InvalidDomain msg) = "invalid domain: " <> Text.pack msg
+showFederatedRequestValidationError RequestMissing = "federation request is missing"
+
+showFederatedRequestValidationErrors :: NonEmpty FederatedRequestValidationError -> Text
+showFederatedRequestValidationErrors =
+  Text.intercalate "; "
+    . map showFederatedRequestValidationError
+    . toList
+
 validateFederatedRequest :: FederatedRequest -> Validation (NonEmpty FederatedRequestValidationError) ValidatedFederatedRequest
 validateFederatedRequest FederatedRequest {..} = do
   vDomain <- validateDomain
diff --git a/services/brig/src/Brig/API/Public.hs b/services/brig/src/Brig/API/Public.hs
index add083e895..2d8b16851d 100644
--- a/services/brig/src/Brig/API/Public.hs
+++ b/services/brig/src/Brig/API/Public.hs
@@ -188,11 +188,10 @@ An error in this category indicates an issue with configuration of federation on
 
 Errors in this category are returned in case of communication issues between the local backend and a remote one, or if the remote side encountered an error while processing an RPC.
 
- - **RPC error with code** (status: 533, label: `grpc-error-code`): An RPC to a remote federator resulted in an error. The RPC error code can be found in the error message.
- - **RPC error with string** (status: 533, label: `grpc-error-string`): An RPC to a remote federator resulted in an error. A description of the error can be found in the error message.
- - **RPC error** (status: 500, label: `federation-rpc-error`): There was a non-specified error in the RPC from this backend to another backend. Check the error message for more details.
- - **Connection refused** (status: 521, label: `cannot-connect-to-remote-federator`): The local federator could not connect to a remote one.
  - **Too much concurrency** (status: 533, label: `too-much-concurrency`): Too many concurrent requests on a remote backend.
+ - **gRPC error** (status: 533, label: `grpc-error`): The current federator encountered an error when making an RPC to a remote one. Check the error message for more details.
+ - **Client RPC error** (status: 500, label: `client-rpc-error`): There was a non-specified error when making a request to another backend. Check the error message for more details.
+ - **Connection refused** (status: 521, label: `cannot-connect-to-remote-federator`): The local federator could not connect to a remote one.
  - **Unknown remote error** (status: 500, label: `unknown-federation-error`): An RPC failed but no specific error was returned by the remote side. Check the error message for more details.
 
 ### Backend compatibility errors
@@ -202,7 +201,6 @@ An error in this category will be returned when this backend makes an invalid or
  - **Version mismatch** (status: 531): A remote backend is running an unsupported version of the federator.
  - **Invalid method** / **Streaming not supported** (status: 500, label: `federation-invalid-call`): This backend attempted an invalid RPC to another backend.
  - **Invalid request** (status: 500, label: `invalid-request-to-federator`): The local federator made an invalid request to a remote one. Check the error message for more details.
- - **RPC client error** (status: 533, label: `grpc-client-error`): The current federator encountered an error when making an RPC to a remote one.
  - **Invalid content type** (status: 503, label: `federation-invalid-content-type-header`): An RPC to another backend returned an invalid content type.
  - **Unsupported content type** (status: 503, label: `federation-unsupported-content-type`): An RPC to another backend returned an unsupported content type.
  - **Invalid origin domain** (status: 533, label: `invalid-origin-domain`): The current backend attempted an RPC with an invalid origin domain field.
diff --git a/services/brig/test/unit/Test/Brig/API/Error.hs b/services/brig/test/unit/Test/Brig/API/Error.hs
index d29463984e..18b21af635 100644
--- a/services/brig/test/unit/Test/Brig/API/Error.hs
+++ b/services/brig/test/unit/Test/Brig/API/Error.hs
@@ -32,7 +32,7 @@ testFedError =
       testCase "when federation call fails due to requesting streaming" $
         assertFedErrorStatus (mkFailure FederationClientStreamingUnsupported) 500,
       testCase "when federation call fails due to discovery failure" $ do
-        let outwardErr = FederationClientOutwardError (Proto.OutwardError Proto.DiscoveryFailed Nothing)
+        let outwardErr = FederationClientOutwardError (Proto.OutwardError Proto.DiscoveryFailed "discovery failed")
         assertFedErrorStatus (mkFailure outwardErr) 500,
       testCase "when federation call fails due to decode failure" $
         assertFedErrorStatus (mkFailure (FederationClientServantError (Servant.DecodeFailure "some failure" emptyRes))) 533,
@@ -64,23 +64,17 @@ testOutwardError =
             assertOutwardErrorStatus Proto.FederationDeniedByRemote 532,
           testCase "when local federator denies federation" $
             assertOutwardErrorStatus Proto.FederationDeniedLocally 400,
-          testCase "when remote federator errors" $
-            assertOutwardErrorStatus Proto.RemoteFederatorError 533,
+          testCase "when there is too much concurrency" $
+            assertOutwardErrorStatus Proto.TooMuchConcurrency 533,
+          testCase "when gRPC fails" $
+            assertOutwardErrorStatus Proto.GrpcError 533,
           testCase "when federator returns invalid request" $
             assertOutwardErrorStatus Proto.InvalidRequest 500
         ],
-      testGroup "label & message" $
-        [ testCase "when error payload is specified" $ do
-            let outwardErr = Proto.OutwardError Proto.TLSFailure . Just $ Proto.ErrorPayload "an-interesting-label" "very interesting message"
-                waiErr = federationRemoteError outwardErr
-            assertEqual "label should be copied" (Wai.label waiErr) "an-interesting-label"
-            assertEqual "message should be copied" (Wai.message waiErr) "very interesting message",
-          testCase "when error payload is not specified" $ do
-            let outwardErr = Proto.OutwardError Proto.TLSFailure Nothing
-                waiErr = federationRemoteError outwardErr
-            assertEqual "label should be set as unknown" (Wai.label waiErr) "unknown-federation-error"
-            assertEqual "message should be set as unknown" (Wai.message waiErr) "Unknown federation error"
-        ]
+      testCase "error message" $ do
+        let outwardErr = Proto.OutwardError Proto.TLSFailure "something went wrong"
+            waiErr = federationRemoteError outwardErr
+        assertEqual "message should be copied" (Wai.message waiErr) "something went wrong"
     ]
 
 mkFailure :: FederationClientError -> FederationError
@@ -93,7 +87,7 @@ assertFedErrorStatus err sts = assertEqual ("http status should be " <> show sts
 
 assertOutwardErrorStatus :: HasCallStack => Proto.OutwardErrorType -> Int -> IO ()
 assertOutwardErrorStatus errType sts =
-  assertEqual ("http status should be " <> show sts) (HTTP.statusCode . Wai.code . federationRemoteError $ Proto.OutwardError errType Nothing) sts
+  assertEqual ("http status should be " <> show sts) (HTTP.statusCode . Wai.code . federationRemoteError $ Proto.OutwardError errType mempty) sts
 
 statusFor :: FederationError -> Int
 statusFor = HTTP.statusCode . errorStatus . fedError
diff --git a/services/federator/src/Federator/InternalServer.hs b/services/federator/src/Federator/InternalServer.hs
index 49d77c860b..b73b669b1f 100644
--- a/services/federator/src/Federator/InternalServer.hs
+++ b/services/federator/src/Federator/InternalServer.hs
@@ -38,15 +38,19 @@ import Mu.GRpc.Client.Record (GRpcReply (..))
 import Mu.GRpc.Server (msgProtoBuf, runGRpcAppTrans)
 import Mu.Server (ServerError, ServerErrorIO, SingleServerT, singleService)
 import qualified Mu.Server as Mu
+import Network.HTTP2.Client.Exceptions (ClientError (..))
+import Network.TLS
 import Polysemy
 import qualified Polysemy.Error as Polysemy
 import Polysemy.IO (embedToMonadIO)
 import qualified Polysemy.Reader as Polysemy
 import Polysemy.TinyLog (TinyLog)
 import qualified Polysemy.TinyLog as Log
+import Wire.API.Federation.GRPC.Client (GrpcClientErr (..))
 import Wire.API.Federation.GRPC.Types
 import Wire.Network.DNS.Effect (DNSLookup)
 import qualified Wire.Network.DNS.Effect as Lookup
+import Wire.Network.DNS.SRV (SrvTarget (..))
 
 callOutward :: Members '[Remote, Polysemy.Reader RunSettings] r => FederatedRequest -> Sem r OutwardResponse
 callOutward req = do
@@ -55,9 +59,9 @@ callOutward req = do
       allowedRemote <- federateWith (vDomain vReq)
       if allowedRemote
         then mkRemoteResponse <$> discoverAndCall vReq
-        else pure $ mkOutwardErr FederationDeniedLocally "federation-not-allowed" ("federating with domain [" <> domainText (vDomain vReq) <> "] is not allowed (see federator configuration)")
+        else pure $ mkOutwardErr FederationDeniedLocally ("federating with domain [" <> domainText (vDomain vReq) <> "] is not allowed (see federator configuration)")
     Failure errs ->
-      pure $ mkOutwardErr InvalidRequest "invalid-request-to-federator" ("validation failed with: " <> Text.pack (show errs))
+      pure $ mkOutwardErr InvalidRequest ("validation failed with: " <> Text.pack (show errs))
 
 -- FUTUREWORK(federation): Make these errors less stringly typed
 mkRemoteResponse :: Either RemoteError (GRpcReply InwardResponse) -> OutwardResponse
@@ -67,26 +71,51 @@ mkRemoteResponse reply =
       OutwardResponseBody res
     Right (GRpcOk (InwardResponseError err)) -> OutwardResponseInwardError err
     Right (GRpcTooMuchConcurrency _) ->
-      mkOutwardErr RemoteFederatorError "too-much-concurrency" "Too much concurrency"
-    Right (GRpcErrorCode grpcErr) ->
-      mkOutwardErr RemoteFederatorError "grpc-error-code" ("code=" <> Text.pack (show grpcErr))
-    Right (GRpcErrorString grpcErr) ->
-      mkOutwardErr RemoteFederatorError "grpc-error-string" ("error=" <> Text.pack grpcErr)
-    Right (GRpcClientError clientErr) ->
-      mkOutwardErr RemoteFederatorError "grpc-client-error" ("error=" <> Text.pack (show clientErr))
+      mkOutwardErr TooMuchConcurrency "Too much concurrency"
+    Right (GRpcErrorCode code) ->
+      mkOutwardErr GrpcError ("code: " <> Text.pack (show code))
+    Right (GRpcErrorString msg) ->
+      mkOutwardErr GrpcError (Text.pack msg)
+    Right (GRpcClientError EarlyEndOfStream) -> mkOutwardErr GrpcError "Early end of stream"
     Left (RemoteErrorDiscoveryFailure domain err) ->
       case err of
         LookupErrorSrvNotAvailable _srvDomain ->
-          mkOutwardErr RemoteNotFound "srv-record-not-found" ("domain=" <> domainText domain)
+          mkOutwardErr RemoteNotFound ("domain=" <> domainText domain)
         LookupErrorDNSError dnsErr ->
-          mkOutwardErr DiscoveryFailed "srv-lookup-dns-error" ("domain=" <> domainText domain <> "; error=" <> Text.decodeUtf8 dnsErr)
-    Left (RemoteErrorClientFailure srvTarget cltErr) ->
-      mkOutwardErr RemoteFederatorError "cannot-connect-to-remote-federator" ("target=" <> Text.pack (show srvTarget) <> "; error=" <> Text.pack (show cltErr))
-    Left (RemoteErrorTLSException srvTarget exc) ->
-      mkOutwardErr TLSFailure "tls-failure" ("Failed to establish TLS session with remote:  target=" <> Text.pack (show srvTarget) <> "; exception=" <> Text.pack (show exc))
+          mkOutwardErr DiscoveryFailed ("domain=" <> domainText domain <> " error=" <> Text.decodeUtf8 dnsErr)
+    Left (RemoteErrorClientFailure (SrvTarget host port) (GrpcClientErr reason)) ->
+      mkOutwardErr
+        GrpcError
+        (reason <> " target=" <> Text.decodeUtf8 host <> ":" <> Text.pack (show port))
+    Left (RemoteErrorTLSException (SrvTarget host port) exc) ->
+      mkOutwardErr
+        TLSFailure
+        ( "Failed to establish TLS session with remote (target="
+            <> Text.decodeUtf8 host
+            <> ":"
+            <> Text.pack (show port)
+            <> "): "
+            <> showTLSException exc
+        )
 
-mkOutwardErr :: OutwardErrorType -> Text -> Text -> OutwardResponse
-mkOutwardErr typ label msg = OutwardResponseError $ OutwardError typ (Just $ ErrorPayload label msg)
+showTLSException :: TLSException -> Text
+showTLSException (Terminated _ reason err) = Text.pack reason <> ": " <> showTLSError err
+showTLSException (HandshakeFailed err) = Text.pack "handshake failed: " <> showTLSError err
+showTLSException ConnectionNotEstablished = Text.pack "connection not established"
+
+showTLSError :: TLSError -> Text
+showTLSError (Error_Misc msg) = Text.pack msg
+showTLSError (Error_Protocol (msg, _, _)) = "protocol error: " <> Text.pack msg
+showTLSError (Error_Certificate msg) = "certificate error: " <> Text.pack msg
+showTLSError (Error_HandshakePolicy msg) = "handshake policy error: " <> Text.pack msg
+showTLSError Error_EOF = "end-of-file error"
+showTLSError (Error_Packet msg) = "packet error: " <> Text.pack msg
+showTLSError (Error_Packet_unexpected actual expected) =
+  "unexpected packet: " <> Text.pack expected <> ", " <> "got " <> Text.pack actual
+showTLSError (Error_Packet_Parsing msg) = "packet parsing error: " <> Text.pack msg
+
+mkOutwardErr :: OutwardErrorType -> Text -> OutwardResponse
+mkOutwardErr typ msg = OutwardResponseError $ OutwardError typ msg
 
 outward :: (Members '[Remote, Polysemy.Error ServerError, Polysemy.Reader RunSettings] r) => SingleServerT info Outward (Sem r) _
 outward = singleService (Mu.method @"call" callOutward)
diff --git a/services/federator/test/unit/Test/Federator/InternalServer.hs b/services/federator/test/unit/Test/Federator/InternalServer.hs
index 6b691de7c5..1d30b640cf 100644
--- a/services/federator/test/unit/Test/Federator/InternalServer.hs
+++ b/services/federator/test/unit/Test/Federator/InternalServer.hs
@@ -26,7 +26,7 @@ import Federator.Options (AllowedDomains (..), FederationStrategy (..), RunSetti
 import Federator.Remote (Remote, RemoteError (RemoteErrorDiscoveryFailure))
 import Imports
 import Mu.GRpc.Client.Record
-import Network.HTTP2.Client (TooMuchConcurrency (TooMuchConcurrency))
+import qualified Network.HTTP2.Client as HTTP2
 import Polysemy (embed, runM)
 import qualified Polysemy.Reader as Polysemy
 import Test.Federator.Options (noClientCertSettings)
@@ -78,7 +78,7 @@ federatedRequestFailureTMC :: TestTree
 federatedRequestFailureTMC =
   testCase "should respond with error when facing GRpcTooMuchConcurrency" $
     runM . evalMock @Remote @IO $ do
-      mockDiscoverAndCallReturns @IO (const $ pure (Right (GRpcTooMuchConcurrency (TooMuchConcurrency 2))))
+      mockDiscoverAndCallReturns @IO (const $ pure (Right (GRpcTooMuchConcurrency (HTTP2.TooMuchConcurrency 2))))
       let federatedRequest = FederatedRequest validDomainText (Just validLocalPart)
 
       res <- mock @Remote @IO . Polysemy.runReader noClientCertSettings $ callOutward federatedRequest
@@ -87,7 +87,7 @@ federatedRequestFailureTMC =
       let expectedCall = ValidatedFederatedRequest (Domain validDomainText) validLocalPart
       embed $ do
         assertEqual "one remote call should be made" [expectedCall] actualCalls
-        assertResponseErrorWithType RemoteFederatorError res
+        assertResponseErrorWithType TooMuchConcurrency res
 
 federatedRequestFailureErrCode :: TestTree
 federatedRequestFailureErrCode =
@@ -102,7 +102,7 @@ federatedRequestFailureErrCode =
       let expectedCall = ValidatedFederatedRequest (Domain validDomainText) validLocalPart
       embed $ do
         assertEqual "one remote call should be made" [expectedCall] actualCalls
-        assertResponseErrorWithType RemoteFederatorError res
+        assertResponseErrorWithType GrpcError res
 
 federatedRequestFailureErrStr :: TestTree
 federatedRequestFailureErrStr =
@@ -117,7 +117,7 @@ federatedRequestFailureErrStr =
       let expectedCall = ValidatedFederatedRequest (Domain validDomainText) validLocalPart
       embed $ do
         assertEqual "one remote call should be made" [expectedCall] actualCalls
-        assertResponseErrorWithType RemoteFederatorError res
+        assertResponseErrorWithType GrpcError res
 
 federatedRequestFailureNoRemote :: TestTree
 federatedRequestFailureNoRemote =
diff --git a/services/galley/test/integration/API.hs b/services/galley/test/integration/API.hs
index 2470983a01..5d5f62169c 100644
--- a/services/galley/test/integration/API.hs
+++ b/services/galley/test/integration/API.hs
@@ -2013,7 +2013,7 @@ testBulkGetQualifiedConvs = do
           case F.domain fedReq of
             d | d == domainText remoteDomainA -> success $ GetConversationsResponse [mockConversationA]
             d | d == domainText remoteDomainB -> success $ GetConversationsResponse [mockConversationB]
-            d | d == domainText remoteDomainC -> pure . F.OutwardResponseError $ F.OutwardError F.DiscoveryFailed Nothing
+            d | d == domainText remoteDomainC -> pure . F.OutwardResponseError $ F.OutwardError F.DiscoveryFailed "discovery failed"
             _ -> assertFailure $ "Unrecognized domain: " <> show fedReq
       )
       (listConvsV2 alice req)
diff --git a/services/galley/test/integration/API/Util.hs b/services/galley/test/integration/API/Util.hs
index d98e3ad618..3e00bd962a 100644
--- a/services/galley/test/integration/API/Util.hs
+++ b/services/galley/test/integration/API/Util.hs
@@ -2123,7 +2123,7 @@ makeFedRequestToServant originDomain server fedRequest =
       if Test.simpleStatus response == status200
         then pure (F.OutwardResponseBody (cs (Test.simpleBody response)))
         else do
-          pure (F.OutwardResponseError (F.OutwardError F.RemoteFederatorError (Just (F.ErrorPayload "mock-error" (cs (Test.simpleBody response))))))
+          pure (F.OutwardResponseError (F.OutwardError F.GrpcError (cs (Test.simpleBody response))))
 
     toRequestWithoutBody :: F.Request -> Wai.Request
     toRequestWithoutBody req =

From 13ae50864b6fd97fb3add6d6381d860728d93ae4 Mon Sep 17 00:00:00 2001
From: Paolo Capriotti <paolo@capriotti.io>
Date: Wed, 8 Sep 2021 13:52:57 +0200
Subject: [PATCH 5/5] Improve federation error descriptions

Also suggest client behaviour in some cases.
---
 services/brig/src/Brig/API/Public.hs | 34 ++++++++++++++++------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/services/brig/src/Brig/API/Public.hs b/services/brig/src/Brig/API/Public.hs
index 2d8b16851d..e2ce8904ba 100644
--- a/services/brig/src/Brig/API/Public.hs
+++ b/services/brig/src/Brig/API/Public.hs
@@ -168,27 +168,34 @@ Centrify allows you to upload the metadata xml document that you get from the `/
 
 Endpoints involving federated calls to other domains can return some extra failure responses, common to all endpoints. Instead of listing them as possible responses for each endpoint, we document them here.
 
+For errors that are more likely to be transient, we suggest clients to retry whatever request resulted in the error. Transient errors are indicated explicitly below.
+
 **Note**: when a failure occurs as a result of making a federated RPC to another backend, the error response contains the following extra fields:
 
  - `domain`: the target backend of the RPC that failed;
  - `path`: the path of the RPC that failed.
 
+### Domain errors
+
+Errors in this category result from trying to communicate with a backend that is considered non-existent or invalid. They can result from invalid user input or client issues, but they can also be a symptom of misconfiguration in one or multiple backends.
+
+ - **Remote backend not found** (status: 422, label: `srv-record-not-found`): This backend attempted to contact a backend which does not exist or is not properly configured. For the most part, clients can consider this error equivalent to a domain not existing, although it should be noted that certain mistakes in the DNS configuration on a remote backend can lead to the backend not being recognized, and hence to this error. It is therefore not advisable to take any destructive action upon encountering this error, such as deleting remote users from conversations.
+ - **Federation denied locally** (status: 400, label: `federation-not-allowed`): This backend attempted an RPC to a non-whitelisted backend. Similar considerations as for the previous error apply.
+
 ### Local federation errors
 
-An error in this category indicates an issue with configuration of federation on the local backend or with the federation-related content of the current client request.
+An error in this category likely indicates an issue with configuration of federation on the local backend. Possibly transient errors are indicated explicitly below.
 
- - **Federation not enabled** (status: 400, label: `federation-not-enabled`): Federation has not been configured for this backend.
- - **Federation unavailable** (status: 500, label: `federation-not-available`): Federation is configured for this backend, but the local federator cannot be reached.
+ - **Federation not enabled** (status: 400, label: `federation-not-enabled`): Federation has not been configured for this backend. This will happen if a federation-aware client tries to talk to a backend for which federation is disabled, or if federation was disabled on the backend after reaching a federation-specific state (e.g. conversations with remote users). There is no way to cleanly recover from these errors at this point.
+ - **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: 403, label: `federation-not-implemented`): Federated behaviour for a certain endpoint is not yet implemented.
- - **Remote backend not found** (status: 422, label: `srv-record-not-found`): This backend attempted to contact a backend which does not exist or is not properly configured.
- - **Federation denied locally** (status: 400, label: `federation-not-allowed`): This backend attempted an RPC to a non-whitelisted backend.
- - **Federator discovery failed** (status: 500, label: `srv-lookup-dns-error`): A DNS error occurred during discovery of a remote backend.
+ - **Federator discovery failed** (status: 500, label: `srv-lookup-dns-error`): A DNS error occurred during discovery of a remote backend. This can be transient, so clients should retry the request.
+ - **Too much concurrency** (status: 533, label: `too-much-concurrency`): Too many concurrent requests from this backend. This can be transient, so clients should retry the request.
 
 ### Remote federation errors
 
-Errors in this category are returned in case of communication issues between the local backend and a remote one, or if the remote side encountered an error while processing an RPC.
+Errors in this category are returned in case of communication issues between the local backend and a remote one, or if the remote side encountered an error while processing an RPC. Some errors in this category might be caused by incorrect client behaviour or wrong user input. All of these errors can be transient, so clients should retry the request that caused them.
 
- - **Too much concurrency** (status: 533, label: `too-much-concurrency`): Too many concurrent requests on a remote backend.
  - **gRPC error** (status: 533, label: `grpc-error`): The current federator encountered an error when making an RPC to a remote one. Check the error message for more details.
  - **Client RPC error** (status: 500, label: `client-rpc-error`): There was a non-specified error when making a request to another backend. Check the error message for more details.
  - **Connection refused** (status: 521, label: `cannot-connect-to-remote-federator`): The local federator could not connect to a remote one.
@@ -196,24 +203,23 @@ Errors in this category are returned in case of communication issues between the
 
 ### Backend compatibility errors
 
-An error in this category will be returned when this backend makes an invalid or unsupported RPC to another backend. This can indicate some incompatibility between backends or a backend bug.
+An error in this category will be returned when this backend makes an invalid or unsupported RPC to another backend. This can indicate some incompatibility between backends or a backend bug. These errors are unlikely to be transient, so retrying requests is *not* advised.
 
  - **Version mismatch** (status: 531): A remote backend is running an unsupported version of the federator.
- - **Invalid method** / **Streaming not supported** (status: 500, label: `federation-invalid-call`): This backend attempted an invalid RPC to another backend.
+ - **Invalid method** / **Streaming not supported** (status: 500, label: `federation-invalid-call`): There was an error in the communication between a service on this backend and the local federator.
  - **Invalid request** (status: 500, label: `invalid-request-to-federator`): The local federator made an invalid request to a remote one. Check the error message for more details.
  - **Invalid content type** (status: 503, label: `federation-invalid-content-type-header`): An RPC to another backend returned an invalid content type.
  - **Unsupported content type** (status: 503, label: `federation-unsupported-content-type`): An RPC to another backend returned an unsupported content type.
  - **Invalid origin domain** (status: 533, label: `invalid-origin-domain`): The current backend attempted an RPC with an invalid origin domain field.
  - **Forbidden endpoint** (status: 533, label: `forbidden-endpoint`): The current backend attempted an RPC to a forbidden or inaccessible remote endpoint.
- - **Unknown federation error** (status: 503, label: `unknown-federation-error): The target of an RPC returned an unexpected reponse. Check the error message for more details.
+ - **Unknown federation error** (status: 503, label: `unknown-federation-error`): The target of an RPC returned an unexpected reponse. Check the error message for more details.
 
 ### Authentication errors
 
-The errors in this category relate to authentication or authorization issues between backends.
+The errors in this category relate to authentication or authorization issues between backends. These errors are unlikely to be transient, so retrying requests is *not* advised.
 
  - **TLS failure**: (status: 525): An error occurred during the TLS handshake between the local federator and a remote one. This is most likely due to an issue with the certificate on the remote end.
- - **Federation denied remotely** (status: 532): The current backend made an unauthorised request to a remote one.
-
+ - **Federation denied remotely** (status: 532): The current backend made an unauthorized request to a remote one.
 |]
 
 servantSitemap :: ServerT ServantAPI Handler