From efc0eaa434d4d5e55e346d8a477ec95965a39e64 Mon Sep 17 00:00:00 2001 From: Akshay Mankar Date: Tue, 1 Mar 2022 11:43:53 +0100 Subject: [PATCH] Fix: Ensure asset download URL uses S3DownloadEndpoint when provided (#2163) --- changelog.d/5-internal/bump-aeson-amazonka | 2 +- .../tests/cargohold-integration.yaml | 14 +++++--- services/cargohold/src/CargoHold/AWS.hs | 8 ++--- services/cargohold/src/CargoHold/S3.hs | 5 ++- services/cargohold/test/integration/API.hs | 35 ++++++++++++++++++- 5 files changed, 51 insertions(+), 13 deletions(-) diff --git a/changelog.d/5-internal/bump-aeson-amazonka b/changelog.d/5-internal/bump-aeson-amazonka index 7445892209b..b995afa39e9 100644 --- a/changelog.d/5-internal/bump-aeson-amazonka +++ b/changelog.d/5-internal/bump-aeson-amazonka @@ -1 +1 @@ -Bump aeson to v2.0.3.0 and update amazonka fork from upstream repository. (#2153, #2157) +Bump aeson to v2.0.3.0 and update amazonka fork from upstream repository. (#2153, #2157, #2163) diff --git a/charts/cargohold/templates/tests/cargohold-integration.yaml b/charts/cargohold/templates/tests/cargohold-integration.yaml index 3a4d1ed94b7..6decd33e477 100644 --- a/charts/cargohold/templates/tests/cargohold-integration.yaml +++ b/charts/cargohold/templates/tests/cargohold-integration.yaml @@ -23,11 +23,17 @@ spec: - name: "cargohold-config" mountPath: "/etc/wire/cargohold/conf" env: - # these dummy values are necessary for Amazonka's "Discover" + # these values are necessary for Amazonka's "Discover" - name: AWS_ACCESS_KEY_ID - value: "dummy" + valueFrom: + secretKeyRef: + name: cargohold + key: awsKeyId - name: AWS_SECRET_ACCESS_KEY - value: "dummy" + valueFrom: + secretKeyRef: + name: cargohold + key: awsSecretKey - name: AWS_REGION - value: "eu-west-1" + value: "{{ .Values.config.aws.region }}" restartPolicy: Never diff --git a/services/cargohold/src/CargoHold/AWS.hs b/services/cargohold/src/CargoHold/AWS.hs index 803406a5ccb..bacb72ac898 100644 --- a/services/cargohold/src/CargoHold/AWS.hs +++ b/services/cargohold/src/CargoHold/AWS.hs @@ -22,7 +22,7 @@ module CargoHold.AWS ( -- * Monad Env, mkEnv, - useDownloadEndpoint, + amazonkaEnvWithDownloadEndpoint, Amazon, amazonkaEnv, execute, @@ -70,9 +70,9 @@ data Env = Env makeLenses ''Env -- | Override the endpoint in the '_amazonkaEnv' with '_amazonkaDownloadEndpoint'. -useDownloadEndpoint :: Env -> Env -useDownloadEndpoint e = - e & amazonkaEnv %~ AWS.override (setAWSEndpoint (e ^. amazonkaDownloadEndpoint)) +amazonkaEnvWithDownloadEndpoint :: Env -> AWS.Env +amazonkaEnvWithDownloadEndpoint e = + AWS.override (setAWSEndpoint (e ^. amazonkaDownloadEndpoint)) (e ^. amazonkaEnv) setAWSEndpoint :: AWSEndpoint -> AWS.Service -> AWS.Service setAWSEndpoint e = AWS.setEndpoint (_awsSecure e) (_awsHost e) (_awsPort e) diff --git a/services/cargohold/src/CargoHold/S3.hs b/services/cargohold/src/CargoHold/S3.hs index f6917f8730a..f0a34a241f7 100644 --- a/services/cargohold/src/CargoHold/S3.hs +++ b/services/cargohold/src/CargoHold/S3.hs @@ -41,7 +41,7 @@ import Amazonka hiding (Error, ToByteString, (.=)) import Amazonka.S3 import Amazonka.S3.Lens import CargoHold.API.Error -import CargoHold.AWS (amazonkaEnv) +import CargoHold.AWS (amazonkaEnvWithDownloadEndpoint) import qualified CargoHold.AWS as AWS import CargoHold.App hiding (Env, Handler) import CargoHold.Options @@ -215,8 +215,7 @@ signedURL path = do ttl <- view (settings . setDownloadLinkTTL) let req = newGetObject (BucketName b) (ObjectKey . Text.decodeLatin1 $ toByteString' path) signed <- - AWS.execute (AWS.useDownloadEndpoint e) $ - presignURL (e ^. amazonkaEnv) now (Seconds (fromIntegral ttl)) req + presignURL (amazonkaEnvWithDownloadEndpoint e) now (Seconds (fromIntegral ttl)) req toUri signed where toUri x = case parseURI strictURIParserOptions x of diff --git a/services/cargohold/test/integration/API.hs b/services/cargohold/test/integration/API.hs index f5f840ddb13..c1c81a0143e 100644 --- a/services/cargohold/test/integration/API.hs +++ b/services/cargohold/test/integration/API.hs @@ -23,6 +23,7 @@ import API.Util import Bilge hiding (body) import Bilge.Assert import CargoHold.API.Error +import CargoHold.Options (awsS3DownloadEndpoint, optAws) import CargoHold.Types import qualified CargoHold.Types.V3 as V3 import qualified Codec.MIME.Type as MIME @@ -43,6 +44,7 @@ import Data.UUID.V4 import Federator.MockServer import Imports hiding (head) import Network.HTTP.Client (parseUrlThrow) +import qualified Network.HTTP.Client as HTTP import Network.HTTP.Media ((//)) import qualified Network.HTTP.Types as HTTP import Network.Wai.Utilities (Error (label)) @@ -50,6 +52,7 @@ import qualified Network.Wai.Utilities.Error as Wai import Test.Tasty import Test.Tasty.HUnit import TestSetup +import Util.Options import Wire.API.Federation.API.Cargohold import Wire.API.Federation.Component @@ -63,7 +66,8 @@ tests s = test s "download with accept header" testDownloadWithAcceptHeader, test s "tokens" testSimpleTokens, test s "s3-upstream-closed" testSimpleS3ClosedConnectionReuse, - test s "client-compatibility" testUploadCompatibility + test s "client-compatibility" testUploadCompatibility, + test s "download url override" testDownloadURLOverride ], testGroup "remote" @@ -219,6 +223,35 @@ testSimpleS3ClosedConnectionReuse = go >> wait >> go uploadSimple (path "/assets/v3") uid sets part2 !!! const 201 === statusCode +testDownloadURLOverride :: TestM () +testDownloadURLOverride = do + -- This is a .example domain, it shouldn't resolve. But it is also not + -- supposed to be used by cargohold to make connections. + let downloadEndpoint = "external-s3-url.example" + withSettingsOverrides (optAws . awsS3DownloadEndpoint ?~ AWSEndpoint downloadEndpoint True 443) $ do + uid <- liftIO $ Id <$> nextRandom + + -- Upload, should work, shouldn't try to use the S3DownloadEndpoint + let bdy = (applicationText, "Hello World") + uploadRes <- + uploadSimple (path "/assets/v3") uid V3.defAssetSettings bdy +