From a4d75e927dd88fb810f5210de6a70b8571426cdd Mon Sep 17 00:00:00 2001 From: Konstantin Kirillov Date: Thu, 22 Sep 2016 11:33:42 +0300 Subject: [PATCH 1/5] Fix generation of signed urls for blobs containing spaces and other special chars --- .../src/main/java/com/google/cloud/storage/StorageImpl.java | 3 ++- .../test/java/com/google/cloud/storage/StorageImplTest.java | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java index c882ec6eb585..8e8209e486f0 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java @@ -55,6 +55,7 @@ import com.google.common.hash.Hashing; import com.google.common.io.BaseEncoding; import com.google.common.primitives.Ints; +import com.google.common.net.UrlEscapers; import java.io.ByteArrayInputStream; import java.io.InputStream; @@ -524,7 +525,7 @@ public URL signUrl(BlobInfo blobInfo, long duration, TimeUnit unit, SignUrlOptio if (blobInfo.name().startsWith("/")) { path.setLength(path.length() - 1); } - path.append(blobInfo.name()); + path.append(UrlEscapers.urlPathSegmentEscaper().escape(blobInfo.name())); stBuilder.append(path); try { byte[] signatureBytes = authCredentials.sign(stBuilder.toString().getBytes(UTF_8)); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java index 3296bf58154f..927b97afe487 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java @@ -50,6 +50,7 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.io.BaseEncoding; +import com.google.common.net.UrlEscapers; import org.easymock.Capture; import org.easymock.EasyMock; @@ -1252,16 +1253,17 @@ public void testSignUrlLeadingSlash() throws NoSuchAlgorithmException, InvalidKe ServiceAccountAuthCredentials.createFor(ACCOUNT, privateKey); storage = options.toBuilder().authCredentials(authCredentials).build().service(); URL url = storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); + String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName); String stringUrl = url.toString(); String expectedUrl = new StringBuilder("https://storage.googleapis.com/").append(BUCKET_NAME1) - .append(blobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") + .append(escapedBlobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") .append(42L + 1209600).append("&Signature=").toString(); assertTrue(stringUrl.startsWith(expectedUrl)); String signature = stringUrl.substring(expectedUrl.length()); StringBuilder signedMessageBuilder = new StringBuilder(); signedMessageBuilder.append(HttpMethod.GET).append("\n\n\n").append(42L + 1209600).append("\n/") - .append(BUCKET_NAME1).append(blobName); + .append(BUCKET_NAME1).append(escapedBlobName); Signature signer = Signature.getInstance("SHA256withRSA"); signer.initVerify(publicKey); From cd7e858f987c154c1432a6ce1b1bb2098c49e7c6 Mon Sep 17 00:00:00 2001 From: Konstantin Kirillov Date: Thu, 22 Sep 2016 16:20:12 +0300 Subject: [PATCH 2/5] Added test for special characters in url signing --- .../google/cloud/storage/StorageImplTest.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java index 927b97afe487..b65a6f8f806e 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java @@ -1301,6 +1301,39 @@ public void testSignUrlWithOptions() throws NoSuchAlgorithmException, InvalidKey URLDecoder.decode(signature, UTF_8.name())))); } + @Test + public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException, InvalidKeyException, + SignatureException, UnsupportedEncodingException { + // List of chars under test were taken from https://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters + char[] specialChars = new char[]{'!','#','$','&','\'','(',')','*','+',',',':',';','=','?','@','[',']'}; + EasyMock.replay(storageRpcMock); + ServiceAccountAuthCredentials authCredentials = + ServiceAccountAuthCredentials.createFor(ACCOUNT, privateKey); + storage = options.toBuilder().authCredentials(authCredentials).build().service(); + + for (char specialChar : specialChars) { + String blobName = "/a" + specialChar + "b"; + URL url = storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); + String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName); + String stringUrl = url.toString(); + String expectedUrl = new StringBuilder("https://storage.googleapis.com/").append(BUCKET_NAME1) + .append(escapedBlobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") + .append(42L + 1209600).append("&Signature=").toString(); + assertTrue(stringUrl.startsWith(expectedUrl)); + String signature = stringUrl.substring(expectedUrl.length()); + + StringBuilder signedMessageBuilder = new StringBuilder(); + signedMessageBuilder.append(HttpMethod.GET).append("\n\n\n").append(42L + 1209600).append("\n/") + .append(BUCKET_NAME1).append(escapedBlobName); + + Signature signer = Signature.getInstance("SHA256withRSA"); + signer.initVerify(publicKey); + signer.update(signedMessageBuilder.toString().getBytes(UTF_8)); + assertTrue(signer.verify(BaseEncoding.base64().decode( + URLDecoder.decode(signature, UTF_8.name())))); + } + } + @Test public void testGetAllArray() { BlobId blobId1 = BlobId.of(BUCKET_NAME1, BLOB_NAME1); From 35daa5c2b9680bf9a0fdcfead0ff15613efa90ad Mon Sep 17 00:00:00 2001 From: Konstantin Kirillov Date: Thu, 22 Sep 2016 17:05:42 +0300 Subject: [PATCH 3/5] Formatting rules applied --- .../google/cloud/storage/StorageImplTest.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java index b65a6f8f806e..3d97090917eb 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java @@ -1302,10 +1302,12 @@ public void testSignUrlWithOptions() throws NoSuchAlgorithmException, InvalidKey } @Test - public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException, InvalidKeyException, - SignatureException, UnsupportedEncodingException { - // List of chars under test were taken from https://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters - char[] specialChars = new char[]{'!','#','$','&','\'','(',')','*','+',',',':',';','=','?','@','[',']'}; + public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException, + InvalidKeyException, SignatureException, UnsupportedEncodingException { + // List of chars under test were taken from + // https://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters + char[] specialChars = + new char[]{'!','#','$','&','\'','(',')','*','+',',',':',';','=','?','@','[',']'}; EasyMock.replay(storageRpcMock); ServiceAccountAuthCredentials authCredentials = ServiceAccountAuthCredentials.createFor(ACCOUNT, privateKey); @@ -1313,7 +1315,8 @@ public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException for (char specialChar : specialChars) { String blobName = "/a" + specialChar + "b"; - URL url = storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); + URL url = + storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName); String stringUrl = url.toString(); String expectedUrl = new StringBuilder("https://storage.googleapis.com/").append(BUCKET_NAME1) @@ -1323,8 +1326,8 @@ public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException String signature = stringUrl.substring(expectedUrl.length()); StringBuilder signedMessageBuilder = new StringBuilder(); - signedMessageBuilder.append(HttpMethod.GET).append("\n\n\n").append(42L + 1209600).append("\n/") - .append(BUCKET_NAME1).append(escapedBlobName); + signedMessageBuilder.append(HttpMethod.GET).append("\n\n\n").append(42L + 1209600) + .append("\n/").append(BUCKET_NAME1).append(escapedBlobName); Signature signer = Signature.getInstance("SHA256withRSA"); signer.initVerify(publicKey); From 66b83a0ac90d85e690bf3c1d4ff10b3367db6fd7 Mon Sep 17 00:00:00 2001 From: Konstantin Kirillov Date: Thu, 22 Sep 2016 17:16:19 +0300 Subject: [PATCH 4/5] Remove extra spaces --- .../com/google/cloud/storage/StorageImplTest.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java index 3d97090917eb..bfcf28bc12e6 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java @@ -1303,37 +1303,37 @@ public void testSignUrlWithOptions() throws NoSuchAlgorithmException, InvalidKey @Test public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException, - InvalidKeyException, SignatureException, UnsupportedEncodingException { + InvalidKeyException, SignatureException, UnsupportedEncodingException { // List of chars under test were taken from // https://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters char[] specialChars = new char[]{'!','#','$','&','\'','(',')','*','+',',',':',';','=','?','@','[',']'}; EasyMock.replay(storageRpcMock); ServiceAccountAuthCredentials authCredentials = - ServiceAccountAuthCredentials.createFor(ACCOUNT, privateKey); + ServiceAccountAuthCredentials.createFor(ACCOUNT, privateKey); storage = options.toBuilder().authCredentials(authCredentials).build().service(); for (char specialChar : specialChars) { String blobName = "/a" + specialChar + "b"; URL url = - storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); + storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName); String stringUrl = url.toString(); String expectedUrl = new StringBuilder("https://storage.googleapis.com/").append(BUCKET_NAME1) - .append(escapedBlobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") - .append(42L + 1209600).append("&Signature=").toString(); + .append(escapedBlobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") + .append(42L + 1209600).append("&Signature=").toString(); assertTrue(stringUrl.startsWith(expectedUrl)); String signature = stringUrl.substring(expectedUrl.length()); StringBuilder signedMessageBuilder = new StringBuilder(); signedMessageBuilder.append(HttpMethod.GET).append("\n\n\n").append(42L + 1209600) - .append("\n/").append(BUCKET_NAME1).append(escapedBlobName); + .append("\n/").append(BUCKET_NAME1).append(escapedBlobName); Signature signer = Signature.getInstance("SHA256withRSA"); signer.initVerify(publicKey); signer.update(signedMessageBuilder.toString().getBytes(UTF_8)); assertTrue(signer.verify(BaseEncoding.base64().decode( - URLDecoder.decode(signature, UTF_8.name())))); + URLDecoder.decode(signature, UTF_8.name())))); } } From 0c65bcf093b63abd1d75b7ae3b7ddc37e6c44aa1 Mon Sep 17 00:00:00 2001 From: Konstantin Kirillov Date: Fri, 28 Oct 2016 10:00:01 +0300 Subject: [PATCH 5/5] Regression in storage url signing of objects with slashes in names fixed (#1346) --- .../com/google/cloud/storage/StorageImpl.java | 2 +- .../google/cloud/storage/StorageImplTest.java | 35 +++++++++++++++++-- 2 files changed, 34 insertions(+), 3 deletions(-) diff --git a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java index 8e8209e486f0..7ec682d10f42 100644 --- a/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java +++ b/google-cloud-storage/src/main/java/com/google/cloud/storage/StorageImpl.java @@ -525,7 +525,7 @@ public URL signUrl(BlobInfo blobInfo, long duration, TimeUnit unit, SignUrlOptio if (blobInfo.name().startsWith("/")) { path.setLength(path.length() - 1); } - path.append(UrlEscapers.urlPathSegmentEscaper().escape(blobInfo.name())); + path.append(UrlEscapers.urlFragmentEscaper().escape(blobInfo.name())); stBuilder.append(path); try { byte[] signatureBytes = authCredentials.sign(stBuilder.toString().getBytes(UTF_8)); diff --git a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java index bfcf28bc12e6..bad4a710c204 100644 --- a/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java +++ b/google-cloud-storage/src/test/java/com/google/cloud/storage/StorageImplTest.java @@ -1253,7 +1253,7 @@ public void testSignUrlLeadingSlash() throws NoSuchAlgorithmException, InvalidKe ServiceAccountAuthCredentials.createFor(ACCOUNT, privateKey); storage = options.toBuilder().authCredentials(authCredentials).build().service(); URL url = storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); - String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName); + String escapedBlobName = UrlEscapers.urlFragmentEscaper().escape(blobName); String stringUrl = url.toString(); String expectedUrl = new StringBuilder("https://storage.googleapis.com/").append(BUCKET_NAME1) .append(escapedBlobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") @@ -1317,7 +1317,7 @@ public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException String blobName = "/a" + specialChar + "b"; URL url = storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); - String escapedBlobName = UrlEscapers.urlPathSegmentEscaper().escape(blobName); + String escapedBlobName = UrlEscapers.urlFragmentEscaper().escape(blobName); String stringUrl = url.toString(); String expectedUrl = new StringBuilder("https://storage.googleapis.com/").append(BUCKET_NAME1) .append(escapedBlobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") @@ -1337,6 +1337,37 @@ public void testSignUrlForBlobWithSpecialChars() throws NoSuchAlgorithmException } } + @Test + public void testSignUrlForBlobWithSlashes() throws NoSuchAlgorithmException, + InvalidKeyException, SignatureException, UnsupportedEncodingException { + EasyMock.replay(storageRpcMock); + ServiceAccountAuthCredentials authCredentials = + ServiceAccountAuthCredentials.createFor(ACCOUNT, privateKey); + storage = options.toBuilder().authCredentials(authCredentials).build().service(); + + String blobName = "/foo/bar/baz #%20other cool stuff.txt"; + URL url = + storage.signUrl(BlobInfo.builder(BUCKET_NAME1, blobName).build(), 14, TimeUnit.DAYS); + + String escapedBlobName = UrlEscapers.urlFragmentEscaper().escape(blobName); + String stringUrl = url.toString(); + String expectedUrl = new StringBuilder("https://storage.googleapis.com/").append(BUCKET_NAME1) + .append(escapedBlobName).append("?GoogleAccessId=").append(ACCOUNT).append("&Expires=") + .append(42L + 1209600).append("&Signature=").toString(); + assertTrue(stringUrl.startsWith(expectedUrl)); + String signature = stringUrl.substring(expectedUrl.length()); + + StringBuilder signedMessageBuilder = new StringBuilder(); + signedMessageBuilder.append(HttpMethod.GET).append("\n\n\n").append(42L + 1209600) + .append("\n/").append(BUCKET_NAME1).append(escapedBlobName); + + Signature signer = Signature.getInstance("SHA256withRSA"); + signer.initVerify(publicKey); + signer.update(signedMessageBuilder.toString().getBytes(UTF_8)); + assertTrue(signer.verify(BaseEncoding.base64().decode( + URLDecoder.decode(signature, UTF_8.name())))); + } + @Test public void testGetAllArray() { BlobId blobId1 = BlobId.of(BUCKET_NAME1, BLOB_NAME1);