From d62787b199bfc86fff32c4afc0e62277548cff7a Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 15 Dec 2023 16:28:27 -0500 Subject: [PATCH 1/4] Detect external SAS tokens --- .../filesystems/blob/BlobPathBuilder.scala | 32 +++++++++++++++---- .../blob/BlobPathBuilderSpec.scala | 15 +++++++++ project/Dependencies.scala | 2 +- 3 files changed, 41 insertions(+), 8 deletions(-) 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..9555a4b777d 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) => + UnparsableBlobPath(new IllegalArgumentException(externalToken)) + 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))) } } 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 = "v" + 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..9af46b46aca 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,21 @@ 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") 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 From 69dbacd416d76e95c876d4dce7ceb005ab09cc13 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 15 Dec 2023 16:28:47 -0500 Subject: [PATCH 2/4] Typo --- .../main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 9555a4b777d..9bda1477224 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala @@ -75,7 +75,7 @@ object BlobPathBuilder { 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 = "v" + val SignedVersionKey = "sv" val SignatureKey = "sig" val query = uri.query().toMap From 5c959b7546471be0825701584eca810d23ee463e Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 15 Dec 2023 16:42:09 -0500 Subject: [PATCH 3/4] Format --- .../filesystems/blob/BlobPathBuilderSpec.scala | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) 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 9af46b46aca..9093141054f 100644 --- a/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala +++ b/filesystems/blob/src/test/scala/cromwell/filesystems/blob/BlobPathBuilderSpec.scala @@ -37,7 +37,9 @@ class BlobPathBuilderSpec extends AnyFlatSpec with Matchers with MockSugar { "?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") + new IllegalArgumentException( + "Rejecting pre-signed SAS URL so that filesystem selection falls through to HTTP filesystem" + ) ).errorMessage.getMessage ) } @@ -54,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( @@ -84,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") @@ -97,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" From 985bda36f1935224a9a552767ef216447423f987 Mon Sep 17 00:00:00 2001 From: Adam Nichols Date: Fri, 15 Dec 2023 16:49:12 -0500 Subject: [PATCH 4/4] Happy case should come first --- .../scala/cromwell/filesystems/blob/BlobPathBuilder.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 9bda1477224..c0503975b31 100644 --- a/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala +++ b/filesystems/blob/src/main/scala/cromwell/filesystems/blob/BlobPathBuilder.scala @@ -59,14 +59,14 @@ object BlobPathBuilder { isBlobHost = testUri.getHost.contains(blobHostnameSuffix) && testUri.getScheme.contains("https") hasToken = hasSasToken(string) blobPathValidation = (isBlobHost, testContainer, hasToken) match { - case (_, _, true) => - UnparsableBlobPath(new IllegalArgumentException(externalToken)) case (true, Some(container), false) => ValidBlobPath(testUri.getPath.replaceFirst("/" + container, ""), BlobContainerName(container), 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