diff --git a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala index 22bf6b44742..c0503975b31 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala @@ -1,5 +1,6 @@ package cromwell.filesystems.blob +import akka.http.scaladsl.model.Uri import com.azure.storage.blob.nio.AzureBlobFileAttributes import com.google.common.net.UrlEscapers import cromwell.core.path.{NioPath, Path, PathBuilder} @@ -21,6 +22,8 @@ object BlobPathBuilder { s"Malformed Blob URL for this builder: The endpoint $endpoint doesn't contain the expected host string '{SA}.blob.core.windows.net/'" def invalidBlobContainerMessage(endpoint: EndpointURL) = s"Malformed Blob URL for this builder: Could not parse container" + val externalToken = + "Rejecting pre-signed SAS URL so that filesystem selection falls through to HTTP filesystem" def parseURI(string: String): Try[URI] = Try(URI.create(UrlEscapers.urlFragmentEscaper().escape(string))) def parseStorageAccount(uri: URI): Try[StorageAccountName] = uri.getHost .split("\\.") @@ -50,19 +53,34 @@ object BlobPathBuilder { def validateBlobPath(string: String): BlobPathValidation = { val blobValidation = for { testUri <- parseURI(string) - testEndpoint = EndpointURL(testUri.getScheme + "://" + testUri.getHost()) - testStorageAccount <- parseStorageAccount(testUri) + testEndpoint = EndpointURL(testUri.getScheme + "://" + testUri.getHost) + _ <- parseStorageAccount(testUri) testContainer = testUri.getPath.split("/").find(_.nonEmpty) - isBlobHost = testUri.getHost().contains(blobHostnameSuffix) && testUri.getScheme().contains("https") - blobPathValidation = (isBlobHost, testContainer) match { - case (true, Some(container)) => + isBlobHost = testUri.getHost.contains(blobHostnameSuffix) && testUri.getScheme.contains("https") + hasToken = hasSasToken(string) + blobPathValidation = (isBlobHost, testContainer, hasToken) match { + case (true, Some(container), false) => ValidBlobPath(testUri.getPath.replaceFirst("/" + container, ""), BlobContainerName(container), testEndpoint) - case (false, _) => UnparsableBlobPath(new MalformedURLException(invalidBlobHostMessage(testEndpoint))) - case (true, None) => UnparsableBlobPath(new MalformedURLException(invalidBlobContainerMessage(testEndpoint))) + case (false, _, false) => + UnparsableBlobPath(new MalformedURLException(invalidBlobHostMessage(testEndpoint))) + case (true, None, false) => + UnparsableBlobPath(new MalformedURLException(invalidBlobContainerMessage(testEndpoint))) + case (_, _, true) => + UnparsableBlobPath(new IllegalArgumentException(externalToken)) } } yield blobPathValidation blobValidation recover { case t => UnparsableBlobPath(t) } get } + + private def hasSasToken(uri: Uri) = { + // These keys are required for all SAS tokens. + // https://learn.microsoft.com/en-us/rest/api/storageservices/create-service-sas#construct-a-service-sas + val SignedVersionKey = "sv" + val SignatureKey = "sig" + + val query = uri.query().toMap + query.isDefinedAt(SignedVersionKey) && query.isDefinedAt(SignatureKey) + } } class BlobPathBuilder()(private val fsm: BlobFileSystemManager) extends PathBuilder { diff --git a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala index fbff8e47431..9093141054f 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -27,6 +27,23 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { } } + it should "reject a path that is otherwise valid, but has a preexisting SAS token" in { + import cromwell.filesystems.blob.BlobPathBuilder.UnparsableBlobPath + + // The `.asInstanceOf[UnparsableBlobPath].errorMessage.getMessage` malarkey is necessary + // because Java exceptions compare by reference, while strings are by value + + val sasBlob = "https://lz304a1e79fd7359e5327eda.blob.core.windows.net/sc-705b830a-d699-478e-9da6-49661b326e77" + + "?sv=2021-12-02&spr=https&st=2023-12-13T20%3A27%3A55Z&se=2023-12-14T04%3A42%3A55Z&sr=c&sp=racwdlt&sig=blah&rscd=foo" + BlobPathBuilder.validateBlobPath(sasBlob).asInstanceOf[UnparsableBlobPath].errorMessage.getMessage should equal( + UnparsableBlobPath( + new IllegalArgumentException( + "Rejecting pre-signed SAS URL so that filesystem selection falls through to HTTP filesystem" + ) + ).errorMessage.getMessage + ) + } + it should "provide a readable error when getting an illegal nioPath" in { val endpoint = BlobPathBuilderSpec.buildEndpoint("storageAccount") val container = BlobContainerName("container") @@ -39,8 +56,9 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { testException should contain(exception) } - private def testBlobNioStringCleaning(input: String, expected: String) = - BlobPath.cleanedNioPathString(input) shouldBe expected + // The following tests use the `centaurtesting` account injected into CI. They depend on access to the + // container specified below. You may need to log in to az cli locally to get them to pass. + private val subscriptionId: SubscriptionId = SubscriptionId(UUID.fromString("62b22893-6bc1-46d9-8a90-806bb3cce3c9")) it should "clean the NIO path string when it has a garbled http protocol" in { testBlobNioStringCleaning( @@ -69,10 +87,6 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { "" ) } - - // The following tests use the `centaurtesting` account injected into CI. They depend on access to the - // container specified below. You may need to log in to az cli locally to get them to pass. - private val subscriptionId: SubscriptionId = SubscriptionId(UUID.fromString("62b22893-6bc1-46d9-8a90-806bb3cce3c9")) private val endpoint: EndpointURL = BlobPathBuilderSpec.buildEndpoint("centaurtesting") private val container: BlobContainerName = BlobContainerName("test-blob") @@ -82,6 +96,9 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { new BlobPathBuilder()(fsm) } + private def testBlobNioStringCleaning(input: String, expected: String) = + BlobPath.cleanedNioPathString(input) shouldBe expected + it should "read md5 from small files <5g" in { val builder = makeBlobPathBuilder() val evalPath = "/testRead.txt" diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 3e40fc85cdd..241260329c6 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -499,7 +499,7 @@ object Dependencies { List("scalatest", "mysql", "mariadb", "postgresql") .map(name => "com.dimafeng" %% s"testcontainers-scala-$name" % testContainersScalaV % Test) - val blobFileSystemDependencies: List[ModuleID] = azureDependencies ++ wsmDependencies + val blobFileSystemDependencies: List[ModuleID] = azureDependencies ++ wsmDependencies ++ akkaHttpDependencies val s3FileSystemDependencies: List[ModuleID] = junitDependencies