From 343da68502899a6985bda9faae1515cc47938804 Mon Sep 17 00:00:00 2001
From: Richard Downer <richard.downer@cloudsoftcorp.com>
Date: Wed, 23 Jan 2013 10:38:58 +0000
Subject: [PATCH] Fix CloudStack URL signing for fields with [ chars

Commit 69a8304 caused the CloudStack QuerySigner to generate invalid
signatures where key names contained square brackets, such as in the
"iptonetworklist[N]" field to deployVirtualMachine. The commit changed
the whole query string being URL-encoded, whereas previously the field
values were encoded but the field names were not. The CloudStack API
guide says that values that should be encoded for signing but not field
names, and indeed the commit does cause signatures to be rejected.

This commit reverses the change to QuerySigner.createStringToSign() and
adds a unit test for this case.
---
 .../jclouds/cloudstack/filters/QuerySigner.java    | 14 +++++++++-----
 .../cloudstack/filters/QuerySignerTest.java        | 12 ++++++++++++
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java b/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java
index 8d6cd42348..cb806ee9b1 100644
--- a/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java
+++ b/apis/cloudstack/src/main/java/org/jclouds/cloudstack/filters/QuerySigner.java
@@ -25,18 +25,19 @@ import static com.google.common.io.ByteStreams.readBytes;
 import static org.jclouds.Constants.LOGGER_SIGNATURE;
 import static org.jclouds.crypto.Macs.asByteProcessor;
 import static org.jclouds.http.Uris.uriBuilder;
-import static org.jclouds.http.utils.Queries.encodeQueryLine;
 import static org.jclouds.http.utils.Queries.queryParser;
 import static org.jclouds.util.Strings2.toInputStream;
 
 import java.io.IOException;
 import java.security.InvalidKeyException;
+import java.util.Map;
 
 import javax.annotation.Resource;
 import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import com.google.common.base.Joiner;
 import org.jclouds.crypto.Crypto;
 import org.jclouds.domain.Credentials;
 import org.jclouds.http.HttpException;
@@ -46,12 +47,13 @@ import org.jclouds.http.internal.SignatureWire;
 import org.jclouds.location.Provider;
 import org.jclouds.logging.Logger;
 import org.jclouds.rest.RequestSigner;
+import org.jclouds.util.Strings2;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSortedSet;
 import com.google.common.collect.Multimap;
-import com.google.common.collect.TreeMultimap;
 import com.google.common.io.ByteProcessor;
 
 /**
@@ -116,10 +118,12 @@ public class QuerySigner implements AuthenticationFilter, RequestSigner {
    @VisibleForTesting
    public String createStringToSign(HttpRequest request, Multimap<String, String> decodedParams) {
       utils.logRequest(signatureLog, request, ">>");
-      // like aws, percent encode the canonicalized string without skipping '/' and '?'
-      String queryLine = encodeQueryLine(TreeMultimap.create(decodedParams), ImmutableList.<Character> of());
+      // encode each parameter value first,
+      ImmutableSortedSet.Builder<String> builder = ImmutableSortedSet.naturalOrder();
+      for (Map.Entry<String, String> entry : decodedParams.entries())
+         builder.add(entry.getKey() + "=" + Strings2.urlEncode(entry.getValue()));
       // then, lower case the entire query string
-      String stringToSign = queryLine.toLowerCase();
+      String stringToSign = Joiner.on('&').join(builder.build()).toLowerCase();
       if (signatureWire.enabled())
          signatureWire.output(stringToSign);
 
diff --git a/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/QuerySignerTest.java b/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/QuerySignerTest.java
index 87f2da01e0..05d80e2dc7 100644
--- a/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/QuerySignerTest.java
+++ b/apis/cloudstack/src/test/java/org/jclouds/cloudstack/filters/QuerySignerTest.java
@@ -61,6 +61,18 @@ public class QuerySignerTest {
             "apikey=apikey&command=listzones");
    }
 
+   @Test
+   void testCreateStringToSignWithBrackets() {
+      // This test asserts that key *names* are not URL-encoded - only values
+      // should be encoded, according to "CloudStack API Developer’s Guide".
+      QuerySigner filter = INJECTOR.getInstance(QuerySigner.class);
+
+      assertEquals(
+            filter.createStringToSign(HttpRequest.builder().method("GET")
+                  .endpoint("http://localhost:8080/client/api?command=deployVirtualMachine&iptonetworklist[0].ip=127.0.0.1&iptonetworklist[0].networkid=1").build()),
+            "apikey=apikey&command=deployvirtualmachine&iptonetworklist[0].ip=127.0.0.1&iptonetworklist[0].networkid=1");
+   }
+
    @Test
    void testFilter() {
       QuerySigner filter = INJECTOR.getInstance(QuerySigner.class);