ec2: pad instance params for reliable sorting

ec2: pad instance params for reliable sorting

url.Values and the params map passed to sign must be sorted identically, or the signature won't match.

https://code.launchpad.net/~dave-cheney/goamz/001-fix-termination-sorting/+merge/128629

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-10-09 05:30:57]

Please take a look.

[dave_cheney.net, 2012-10-09 05:32:31]

On 2012/10/09 05:30:57, dfc wrote:
> Please take a look.

This is a quick fix. I could spend more time looking into how url.Values' and
the sign params map are sorted, but I think the results wouldn't be
significantly better. Amazon are happy to accept 0 padded instance numbers for
the termination request.

[niemeyer, 2012-10-09 11:35:54]

Thanks a lot for looking at this Dave. A few comments:

https://codereview.appspot.com/6642048/diff/1/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/6642048/diff/1/ec2/ec2.go#newcode364
ec2/ec2.go:364: params[fmt.Sprintf("InstanceId.%03d", i+1)] = id
That looks surprisingly dodgy of Amazon.

A few things:

1) Can we have a test that reproduces the problem and shows the fix works and
continues to work? Note that you don't need to actually create N resources. If
you address a non-existing resource, we'll get a different error in case the
resource is not found or the resource doesn't exist.

2) We would have to fix *all* of the methods so that they build the query in
this way. I really doubt this issue is specific to terminate. Instead of doing
that and waiting for the next time we screw up, though, I suggest building a
sortable slice type next to sign that considers the termination of the key in a
.N pattern and sorts it appropriately.

3) Have you checked what boto does? Does it pad the number as well when we
provide more than 9 items to it, does it break, or does it do something else?

[dave_cheney.net, 2012-10-09 11:46:21]

Yup - it was a quick fix. I will investigate more tomorrow.

With respect to the other .n methods, I was wondering about that, but
didn't have  chance to review their signatures.

For me, the proper fix is to create the entire URL as a string, then
sign that string, rather than passing the component parts of the url,
params, etc, to two separate functions, and assuming/hoping they
compute the URL in the same way. It is not sufficient for the two URLs
to be logically the same, they must be identical for the signing to
work. That is where I think things fell apart.

On Tue, Oct 9, 2012 at 10:35 PM,  <n13m3y3r@gmail.com> wrote:
> Thanks a lot for looking at this Dave. A few comments:
>
>
> https://codereview.appspot.com/6642048/diff/1/ec2/ec2.go
> File ec2/ec2.go (right):
>
> https://codereview.appspot.com/6642048/diff/1/ec2/ec2.go#newcode364
> ec2/ec2.go:364: params[fmt.Sprintf("InstanceId.%03d", i+1)] = id
> That looks surprisingly dodgy of Amazon.
>
> A few things:
>
> 1) Can we have a test that reproduces the problem and shows the fix
> works and continues to work? Note that you don't need to actually create
> N resources. If you address a non-existing resource, we'll get a
> different error in case the resource is not found or the resource
> doesn't exist.
>
> 2) We would have to fix *all* of the methods so that they build the
> query in this way. I really doubt this issue is specific to terminate.
> Instead of doing that and waiting for the next time we screw up, though,
> I suggest building a sortable slice type next to sign that considers the
> termination of the key in a .N pattern and sorts it appropriately.
>
> 3) Have you checked what boto does? Does it pad the number as well when
> we provide more than 9 items to it, does it break, or does it do
> something else?
>
> https://codereview.appspot.com/6642048/

[niemeyer, 2012-10-09 12:17:44]

On 2012/10/09 11:46:21, dfc wrote:
> For me, the proper fix is to create the entire URL as a string, then
> sign that string, rather than passing the component parts of the url,
> params, etc, to two separate functions, and assuming/hoping they
> compute the URL in the same way. It is not sufficient for the two 

I probably don't understand what you mean here. There's a single sign function,
which does exactly what you describe above. If the sorting is wrong, it should
be fixed.

[dave_cheney.net, 2012-10-25 08:05:23]

yes, i have fixed the sorting, in sign, which everyone uses.

[niemeyer, 2012-10-25 08:31:32]

On 2012/10/25 08:05:23, dfc wrote:
> yes, i have fixed the sorting, in sign, which everyone uses.

Not in this CL, at least.

[dave_cheney.net, 2012-10-25 08:35:14]

On 2012/10/25 08:05:23, dfc wrote:
> yes, i have fixed the sorting, in sign, which everyone uses.

aww man, where has my fix gone.

[dave_cheney.net, 2012-10-25 08:37:45]

On 2012/10/25 08:35:14, dfc wrote:
> On 2012/10/25 08:05:23, dfc wrote:
> > yes, i have fixed the sorting, in sign, which everyone uses.
> 
> aww man, where has my fix gone.

Sorry, abandoned and reproposed on https://codereview.appspot.com/6734043