Allow using vhost style addressing for S3 (#2955)

Path style is not supported for newer buckets, more info: https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/ All object storage providers (like MinIO, ScalityRing, etc) might not work with vhost style addressing, so this change introduces a new configuration option in cargohold as aws.s3AddressingStyle to choose the addressing style. Other changes: * Move wire-server from using forked version of amazonka to upstream HEAD. The option to choose S3 Addressing Style has been implemented in brendanhay/amazonka#832 * Makefile: Skip schema migrations for packages without DB This allows running something like `make ci package=cargohold` even if cargohold doesn't produce a cargohold-schema executable.
wireapp · Dec 29, 2022 · 0b99889 · 0b99889
1 parent 3f9c17e
commit 0b99889
Show file tree

Hide file tree

Showing 20 changed files with 173 additions and 60 deletions.
diff --git a/Makefile b/Makefile
@@ -266,8 +266,11 @@ ifeq ($(package), all)
 	./dist/galley-schema --keyspace galley_test --replication-factor 1
 	./dist/gundeck-schema --keyspace gundeck_test --replication-factor 1
 	./dist/spar-schema --keyspace spar_test --replication-factor 1
-else
+# How this check works: https://stackoverflow.com/a/9802777
+else ifeq ($(package), $(filter $(package),brig galley gundeck spar))
 	$(EXE_SCHEMA) --keyspace $(package)_test --replication-factor 1
+else
+	@echo No schema migrations for $(package)
 endif
 
 

diff --git a/changelog.d/2-features/vhost-addressing-for-s3 b/changelog.d/2-features/vhost-addressing-for-s3
@@ -0,0 +1,3 @@
+Allow vhost style addressing for S3 as path style is not supported for newer buckets.
+
+More info: https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/
diff --git a/charts/cargohold/templates/configmap.yaml b/charts/cargohold/templates/configmap.yaml
@@ -28,6 +28,9 @@ data:
       {{- if .s3Compatibility }}
       s3Compatibility: {{ .s3Compatibility }}
       {{- end }}
+      {{- if .s3AddressingStyle }}
+      s3AddressingStyle: {{ .s3AddressingStyle }}
+      {{- end }}
       {{ if .cloudFront }}
       cloudFront:
         domain: {{ .cloudFront.domain }}

diff --git a/docs/src/how-to/install/configuration-options.rst b/docs/src/how-to/install/configuration-options.rst
@@ -1058,3 +1058,49 @@ The table assumes the following:
 * When backend level config says that this feature is disabled, the list of domains is ignored.
 * When team level feature is disabled, the accompanying domains are ignored.
 
+S3 Addressing Style
+-------------------
+
+S3 can either by addressed in path style, i.e.
+`https://<s3-endpoint>/<bucket-name>/<object>`, or vhost style, i.e.
+`https://<bucket-name>.<s3-endpoint>/<object>`. AWS's S3 offering has deprecated
+path style addressing for S3 and completely disabled it for buckets created
+after 30 Sep 2020:
+https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/
+
+However other object storage providers (specially self-deployed ones like MinIO)
+may not support vhost style addressing yet (or ever?). Users of such buckets
+should configure this option to "path":
+
+.. code:: yaml
+
+    cargohold:
+      aws:
+        s3AddressingStyle: path
+
+Installations using S3 service provided by AWS, should use "auto", this option
+will ensure that vhost style is only used when it is possible to construct a
+valid hostname from the bucket name and the bucket name doesn't contain a '.'.
+Having a '.' in the bucket name causes TLS validation to fail, hence it is not
+used by default:
+
+.. code:: yaml
+
+    cargohold:
+      aws:
+        s3AddressingStyle: auto
+
+
+Using "virtual" as an option is only useful in situations where vhost style
+addressing must be used even if it is not possible to construct a valid hostname
+from the bucket name or the S3 service provider can ensure correct certificate
+is issued for bucket which contain one or more '.'s in the name:
+
+.. code:: yaml
+
+    cargohold:
+      aws:
+        s3AddressingStyle: virtual
+
+When this option is unspecified, wire-server defaults to path style addressing
+to ensure smooth transition for older deployments.
diff --git a/libs/types-common-aws/default.nix b/libs/types-common-aws/default.nix
@@ -4,6 +4,7 @@
 # dependencies are added or removed.
 { mkDerivation
 , amazonka
+, amazonka-core
 , amazonka-sqs
 , base
 , base64-bytestring
@@ -27,6 +28,7 @@ mkDerivation {
   src = gitignoreSource ./.;
   libraryHaskellDepends = [
     amazonka
+    amazonka-core
     amazonka-sqs
     base
     base64-bytestring

diff --git a/libs/types-common-aws/src/AWS/Util.hs b/libs/types-common-aws/src/AWS/Util.hs
@@ -18,15 +18,16 @@
 module AWS.Util where
 
 import qualified Amazonka as AWS
+import qualified Amazonka.Data.Time as AWS
 import Data.Time
 import Imports
 
 readAuthExpiration :: AWS.Env -> IO (Maybe NominalDiffTime)
 readAuthExpiration env = do
   authEnv <-
-    case runIdentity (AWS.envAuth env) of
+    case runIdentity (AWS.auth env) of
       AWS.Auth authEnv -> pure authEnv
       AWS.Ref _ ref -> do
         readIORef ref
   now <- getCurrentTime
-  pure $ (`diffUTCTime` now) . AWS.fromTime <$> AWS._authExpiration authEnv
+  pure $ (`diffUTCTime` now) . AWS.fromTime <$> AWS.expiration authEnv
diff --git a/libs/types-common-aws/types-common-aws.cabal b/libs/types-common-aws/types-common-aws.cabal
@@ -75,6 +75,7 @@ library
   ghc-prof-options:   -fprof-auto-exported
   build-depends:
       amazonka
+    , amazonka-core
     , amazonka-sqs
     , base               >=4    && <5
     , base64-bytestring  >=1.0

diff --git a/nix/haskell-pins.nix b/nix/haskell-pins.nix
@@ -93,9 +93,9 @@ let
     };
     amazonka = {
       src = fetchgit {
-        url = "https://github.com/wireapp/amazonka";
-        rev = "7ced54b0396296307b9871d293cc0ac161e5743d";
-        sha256 = "0md658m32zrvzc8nljn58r8iw4rqxpihgdnqrhl8vnmkq6i9np51";
+        url = "https://github.com/brendanhay/amazonka";
+        rev = "cfe2584aef0b03c86650372d362c74f237925d8c";
+        sha256 = "sha256-ss8IuIN0BbS6LMjlaFmUdxUqQu+IHsA8ucsjxXJwbyg=";
       };
       packages = {
         amazonka = "lib/amazonka";

diff --git a/services/brig/brig.cabal b/services/brig/brig.cabal
@@ -183,10 +183,11 @@ library
 
   build-depends:
       aeson                      >=2.0.1.0
-    , amazonka                   >=1.3.7
-    , amazonka-dynamodb          >=1.3.7
-    , amazonka-ses               >=1.3.7
-    , amazonka-sqs               >=1.3.7
+    , amazonka                   >=2
+    , amazonka-core              >=2
+    , amazonka-dynamodb          >=2
+    , amazonka-ses               >=2
+    , amazonka-sqs               >=2
     , async                      >=2.1
     , attoparsec                 >=0.12
     , auto-update                >=0.1

diff --git a/services/brig/default.nix b/services/brig/default.nix
@@ -5,6 +5,7 @@
 { mkDerivation
 , aeson
 , amazonka
+, amazonka-core
 , amazonka-dynamodb
 , amazonka-ses
 , amazonka-sqs
@@ -168,6 +169,7 @@ mkDerivation {
   libraryHaskellDepends = [
     aeson
     amazonka
+    amazonka-core
     amazonka-dynamodb
     amazonka-ses
     amazonka-sqs

diff --git a/services/brig/src/Brig/AWS.hs b/services/brig/src/Brig/AWS.hs
@@ -47,6 +47,7 @@ where
 
 import Amazonka (AWSRequest, AWSResponse)
 import qualified Amazonka as AWS
+import qualified Amazonka.Data.Text as AWS
 import qualified Amazonka.DynamoDB as DDB
 import qualified Amazonka.SES as SES
 import qualified Amazonka.SES.Lens as SES
@@ -122,13 +123,13 @@ mkEnv lgr opts emailOpts mgr = do
     mkAwsEnv g ses dyn sqs = do
       baseEnv <-
         AWS.newEnv AWS.discover
-          <&> maybe id AWS.configure ses
-          <&> maybe id AWS.configure dyn
-          <&> AWS.configure sqs
+          <&> maybe id AWS.configureService ses
+          <&> maybe id AWS.configureService dyn
+          <&> AWS.configureService sqs
       pure $
         baseEnv
-          { AWS.envLogger = awsLogger g,
-            AWS.envManager = mgr
+          { AWS.logger = awsLogger g,
+            AWS.manager = mgr
           }
     awsLogger g l = Logger.log g (mapLevel l) . Logger.msg . toLazyByteString
     mapLevel AWS.Info = Logger.Info
@@ -226,10 +227,10 @@ sendMail m = do
       -- after the fact.
       AWS.ServiceError se
         | se
-            ^. AWS.serviceStatus
+            ^. AWS.serviceError_status
             == status400
             && "Invalid domain name"
-            `Text.isPrefixOf` AWS.toText (se ^. AWS.serviceCode) ->
+            `Text.isPrefixOf` AWS.toText (se ^. AWS.serviceError_code) ->
             throwM SESInvalidDomain
       _ -> throwM (GeneralError x)
 
@@ -268,7 +269,7 @@ canRetry :: MonadIO m => Either AWS.Error a -> m Bool
 canRetry (Right _) = pure False
 canRetry (Left e) = case e of
   AWS.TransportError (HttpExceptionRequest _ ResponseTimeout) -> pure True
-  AWS.ServiceError se | se ^. AWS.serviceCode == AWS.ErrorCode "RequestThrottled" -> pure True
+  AWS.ServiceError se | se ^. AWS.serviceError_code == AWS.ErrorCode "RequestThrottled" -> pure True
   _ -> pure False
 
 retry5x :: (Monad m) => RetryPolicyM m

diff --git a/services/brig/src/Brig/Data/Client.hs b/services/brig/src/Brig/Data/Client.hs
@@ -49,6 +49,7 @@ module Brig.Data.Client
 where
 
 import qualified Amazonka as AWS
+import qualified Amazonka.Data.Text as AWS
 import qualified Amazonka.DynamoDB as AWS
 import qualified Amazonka.DynamoDB.Lens as AWS
 import Bilge.Retry (httpHandlers)
@@ -567,7 +568,7 @@ withOptLock u c ma = go (10 :: Int)
             run = execCatch e cmd >>= either handleErr (pure . conv)
             handlers = httpHandlers ++ [const $ EL.handler_ AWS._ConditionalCheckFailedException (pure True)]
             policy = limitRetries 3 <> exponentialBackoff 100000
-            handleErr (AWS.ServiceError se) | se ^. AWS.serviceCode == AWS.ErrorCode "ProvisionedThroughputExceeded" = do
+            handleErr (AWS.ServiceError se) | se ^. AWS.serviceError_code == AWS.ErrorCode "ProvisionedThroughputExceeded" = do
               Metrics.counterIncr (Metrics.path "client.opt_lock.provisioned_throughput_exceeded") m
               pure Nothing
             handleErr _ = pure Nothing

diff --git a/services/cargohold/src/CargoHold/AWS.hs b/services/cargohold/src/CargoHold/AWS.hs
@@ -70,9 +70,10 @@ data Env = Env
 makeLenses ''Env
 
 -- | Override the endpoint in the '_amazonkaEnv' with '_amazonkaDownloadEndpoint'.
+-- TODO: Choose the correct s3 addressing style
 amazonkaEnvWithDownloadEndpoint :: Env -> AWS.Env
 amazonkaEnvWithDownloadEndpoint e =
-  AWS.override (setAWSEndpoint (e ^. amazonkaDownloadEndpoint)) (e ^. amazonkaEnv)
+  AWS.overrideService (setAWSEndpoint (e ^. amazonkaDownloadEndpoint)) (e ^. amazonkaEnv)
 
 setAWSEndpoint :: AWSEndpoint -> AWS.Service -> AWS.Service
 setAWSEndpoint e = AWS.setEndpoint (_awsSecure e) (_awsHost e) (_awsPort e)
@@ -100,16 +101,17 @@ mkEnv ::
   Logger ->
   -- | S3 endpoint
   AWSEndpoint ->
+  AWS.S3AddressingStyle ->
   -- | Endpoint for downloading assets (for the external world)
   AWSEndpoint ->
   -- | Bucket
   Text ->
   Maybe CloudFrontOpts ->
   Manager ->
   IO Env
-mkEnv lgr s3End s3Download bucket cfOpts mgr = do
+mkEnv lgr s3End s3AddrStyle s3Download bucket cfOpts mgr = do
   let g = Logger.clone (Just "aws.cargohold") lgr
-  e <- mkAwsEnv g (setAWSEndpoint s3End S3.defaultService)
+  e <- mkAwsEnv g (setAWSEndpoint s3End (S3.defaultService & AWS.service_s3AddressingStyle .~ s3AddrStyle))
   cf <- mkCfEnv cfOpts
   pure (Env g bucket e s3Download cf)
   where
@@ -118,11 +120,11 @@ mkEnv lgr s3End s3Download bucket cfOpts mgr = do
     mkAwsEnv g s3 = do
       baseEnv <-
         AWS.newEnv AWS.discover
-          <&> AWS.configure s3
+          <&> AWS.configureService s3
       pure $
         baseEnv
-          { AWS.envLogger = awsLogger g,
-            AWS.envManager = mgr
+          { AWS.logger = awsLogger g,
+            AWS.manager = mgr
           }
     awsLogger g l = Logger.log g (mapLevel l) . Log.msg . toLazyByteString
     mapLevel AWS.Info = Logger.Info
@@ -222,7 +224,7 @@ canRetry :: MonadIO m => Either AWS.Error a -> m Bool
 canRetry (Right _) = pure False
 canRetry (Left e) = case e of
   AWS.TransportError (HttpExceptionRequest _ ResponseTimeout) -> pure True
-  AWS.ServiceError se | se ^. AWS.serviceCode == AWS.ErrorCode "RequestThrottled" -> pure True
+  AWS.ServiceError se | se ^. AWS.serviceError_code == AWS.ErrorCode "RequestThrottled" -> pure True
   _ -> pure False
 
 retry5x :: (Monad m) => RetryPolicyM m

diff --git a/services/cargohold/src/CargoHold/App.hs b/services/cargohold/src/CargoHold/App.hs
@@ -46,6 +46,7 @@ module CargoHold.App
   )
 where
 
+import Amazonka (S3AddressingStyle (S3AddressingStylePath))
 import Bilge (Manager, MonadHttp, RequestId (..), newManager, withResponse)
 import qualified Bilge
 import Bilge.RPC (HasRequestId (..))
@@ -97,9 +98,10 @@ newEnv o = do
   pure $ Env ama met lgr mgr def o loc
 
 initAws :: AWSOpts -> Logger -> Manager -> IO AWS.Env
-initAws o l = AWS.mkEnv l (o ^. awsS3Endpoint) downloadEndpoint (o ^. awsS3Bucket) (o ^. awsCloudFront)
+initAws o l = AWS.mkEnv l (o ^. awsS3Endpoint) addrStyle downloadEndpoint (o ^. awsS3Bucket) (o ^. awsCloudFront)
   where
     downloadEndpoint = fromMaybe (o ^. awsS3Endpoint) (o ^. awsS3DownloadEndpoint)
+    addrStyle = maybe S3AddressingStylePath unwrapS3AddressingStyle (o ^. awsS3AddressingStyle)
 
 initHttpManager :: Maybe S3Compatibility -> IO Manager
 initHttpManager s3Compat =

diff --git a/services/cargohold/src/CargoHold/Options.hs b/services/cargohold/src/CargoHold/Options.hs
@@ -20,6 +20,7 @@
 
 module CargoHold.Options where
 
+import Amazonka (S3AddressingStyle (..))
 import qualified CargoHold.CloudFront as CF
 import Control.Lens hiding (Level)
 import Data.Aeson (FromJSON (..), withText)
@@ -45,8 +46,48 @@ deriveFromJSON toOptionFieldName ''CloudFrontOpts
 
 makeLenses ''CloudFrontOpts
 
+newtype OptS3AddressingStyle = OptS3AddressingStyle
+  { unwrapS3AddressingStyle :: S3AddressingStyle
+  }
+  deriving (Show)
+
+instance FromJSON OptS3AddressingStyle where
+  parseJSON =
+    withText "S3AddressingStyle" $
+      fmap OptS3AddressingStyle . \case
+        "auto" -> pure S3AddressingStyleAuto
+        "path" -> pure S3AddressingStylePath
+        "virtual" -> pure S3AddressingStyleVirtual
+        other -> fail $ "invalid S3AddressingStyle: " <> show other
+
 data AWSOpts = AWSOpts
   { _awsS3Endpoint :: !AWSEndpoint,
+    -- | S3 can either by addressed in path style, i.e.
+    -- https://<s3-endpoint>/<bucket-name>/<object>, or vhost style, i.e.
+    -- https://<bucket-name>.<s3-endpoint>/<object>. AWS's S3 offering has
+    -- deprecated path style addressing for S3 and completely disabled it for
+    -- buckets created after 30 Sep 2020:
+    -- https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/
+    --
+    -- However other object storage providers (specially self-deployed ones like
+    -- MinIO) may not support vhost style addressing yet (or ever?). Users of
+    -- such buckets should configure this option to "path".
+    --
+    -- Installations using S3 service provided by AWS, should use "auto", this
+    -- option will ensure that vhost style is only used when it is possible to
+    -- construct a valid hostname from the bucket name and the bucket name
+    -- doesn't contain a '.'. Having a '.' in the bucket name causes TLS
+    -- validation to fail, hence it is not used by default.
+    --
+    -- Using "virtual" as an option is only useful in situations where vhost
+    -- style addressing must be used even if it is not possible to construct a
+    -- valid hostname from the bucket name or the S3 service provider can ensure
+    -- correct certificate is issued for bucket which contain one or more '.'s
+    -- in the name.
+    --
+    -- When this option is unspecified, we default to path style addressing to
+    -- ensure smooth transition for older deployments.
+    _awsS3AddressingStyle :: !(Maybe OptS3AddressingStyle),
     -- | S3 endpoint for generating download links. Useful if Cargohold is configured to use
     -- an S3 replacement running inside the internal network (in which case internally we
     -- would use one hostname for S3, and when generating an asset link for a client app, we

diff --git a/services/cargohold/src/CargoHold/S3.hs b/services/cargohold/src/CargoHold/S3.hs
@@ -36,7 +36,7 @@ module CargoHold.S3
   )
 where
 
-import Amazonka hiding (Error, ToByteString, (.=))
+import Amazonka hiding (Error)
 import Amazonka.S3
 import Amazonka.S3.Lens
 import CargoHold.API.Error
@@ -145,7 +145,7 @@ downloadV3 ::
   ExceptT Error App (ConduitM () ByteString (ResourceT IO) ())
 downloadV3 (s3Key . mkKey -> key) = do
   env <- view aws
-  pure . flattenResourceT $ _streamBody . view getObjectResponse_body <$> AWS.execStream env req
+  pure . flattenResourceT $ view (getObjectResponse_body . _ResponseBody) <$> AWS.execStream env req
   where
     req :: Text -> GetObject
     req b =
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		Allow vhost style addressing for S3 as path style is not supported for newer buckets.

		More info: https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/