Fix inaccurate s3 rewrite rule. #1040

ksauzz · 2015-01-06T13:43:07Z

Riak CS doesn't handle some specific characters in object name well.

see: https://gist.github.com/ksauzz/9f0dc13c9b8cb0f2a85e
related to #910

This PR includes:

Fix s3 rewrite rule which results in losing backward compability.
Introduce riak_cs_s3_rewrite_legacy to keep backward compatibility.
Add regression test for riak_cs_s3_rewrite_legacy.

Topics I'd like to discuss

Is using test-python reasonable for regression test ? At least, boto test saved me from my bad codes.

How access log format changes after this patches.

before

172.17.42.1 - - [07/Jan/2015:08:27:07 +0000] "PUT /buckets/test/objects/path1%2Fpath2%2Fte%2Bst.txt HTTP/1.1" 200 0 "" ""

after

127.0.0.1 - - [07/Jan/2015:17:28:23 +0900] "PUT /buckets/test/objects/path1%2Fpath2%2Fte%252Bst.txt HTTP/1.1" 200 0 "" ""

TODO:

Update releasenote about description the change of rewrite rule, and how to keep backward compatibility.

kuenishi · 2015-01-07T02:19:14Z

riak_test/tests/legacy_s3_rewrite_test.erl

+            error({external_client_tests, Reason})
+    end.
+
+execute_cmd(Cmd, Opts) ->


I'd recommend just using rt_cs_dev:cmd/2 instead.

I don't like duplicate code ^^;

rt_cs_dev:cmd/2 doesn't look suitable for this case because it doesn't handle exit code, or output log until the command finishes.I'd like to extract these function to rtcs module.

In master branch of riak_test rtdev:cmd/2 handles the return value. We're just stale.

And also in master branch, rt_cs_dev:cmd/2 does.

kuenishi · 2015-01-07T02:27:00Z

riak_test/tests/legacy_s3_rewrite_test.erl

+
+-module(legacy_s3_rewrite_test).
+
+%% @doc `riak_test' module for testing object get behavior.


This is a stale comment from copy source.

kuenishi · 2015-01-07T02:52:54Z

I believe you should update riak_cs_config:api/0 to handle riak_cs_s3_rewrite_legacy, or it does no harm to riak_cs_wm_common:maybe_create_user/6 ?

kuenishi · 2015-01-07T02:53:53Z

I also believe it is a good opportunity to write eqc tests on rewrite. If you can't afford time for now, I can do it later.

kuenishi · 2015-01-07T03:30:21Z

riak_test/tests/legacy_s3_rewrite_test.erl

+
+-define(TEST_BUCKET, "legacy-s3-rewrite-test").
+
+confirm() ->


I'd be good if this test includes the exact case where the problem happened, like:

put key "foo bar" with content "foo bar"

put key "foo+bar" with content "foo+bar"

read "foo bar" and "foo+bar" and verify they have different content

Maybe it's about testing legacy code, so I'd say this is enough to test old behaviour.

kuenishi · 2015-01-07T03:39:00Z

Memo: to preserve old path translation behaviour, users should update app.config as {rewrite_module, riak_cs_s3_rewrite_legacy} from default when updating from 1.5.3 or older.

kuenishi · 2015-01-07T03:41:50Z

After upgraded to the latest version and not to preserve old odd behaviour, new key for the old object put as "foo+bar", is "foo bar".

shino · 2015-01-08T02:17:17Z

By removing URL decode at rewrite module, bucket part of path style
access is never decoded.

For example, using GET Bucket request (client can URL encode any characters in path part, %74 is t):

s3curl.pl --id cs --get -- -x 127.0.0.1:15018 -i -s 'http://s3.amazonaws.com/%74est2'

Current release/1.5 branch responds with 200 OK and contents:

HTTP/1.1 200 OK
Server: Riak CS
Date: Thu, 08 Jan 2015 02:14:39 GMT
Content-Type: application/xml
Content-Length: 253

<?xml version="1.0" encoding="UTF-8"?><ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Name>test2</Name><Prefix></Prefix><Marker></Marker><MaxKeys>1000</MaxKeys><Delimiter></Delimiter><IsTruncated>false</IsTruncated></ListBucketResult>%

This branch (at commit d1c3d02) responds with error:

HTTP/1.1 404 Object Not Found
Server: Riak CS
Date: Thu, 08 Jan 2015 02:14:08 GMT
Content-Type: application/xml
Content-Length: 185

<?xml version="1.0" encoding="UTF-8"?><Error><Code>NoSuchBucket</Code><Message>The specified bucket does not exist.</Message><Resource>/%74est2</Resource><RequestId></RequestId></Error>%

kuenishi · 2015-01-09T01:52:01Z

As for @shino 's comment, we'll leave it for bucket names bug just fix object keys. If legacy behaviour preferred, just use legacy rewrite module.

shino · 2015-01-09T01:54:07Z

@kuenishi The bug was fixed by 40609c5 (if I understand correctly).

kuenishi · 2015-01-09T01:57:02Z

ah 🙏

kuenishi · 2015-01-09T02:17:25Z

All riak_test has passed as usual. test-riak_cs is still screwed, although.

kuenishi · 2015-01-09T02:18:36Z

kuenishi · 2015-01-13T01:26:50Z

quote state     RAW    escaped(1)  escaped(2)    example

                 *                               'baz/foo bar'        'baz/foo+bar'
client app       +--------+                      'baz/foo%20bar'      'baz/foo%2Bbar'
                          |
HTTP                      |
                          |
webmachine                |
before rewrite            |
rewrite                   +------------+         'baz%2Ffoo%2520bar'  'baz%2Ffoo%252Bbar'
after rewrite                          |
                                       |
extract_key      +---------------------+
                 |
                 v
                 *                               'baz/foo bar'        'baz/foo+bar'

shino · 2015-01-13T01:35:51Z

Note: The above diagram is only on objects part of the path in case of "virtual host style" access. buckets part in "path style" and subresource part are different, not doubly escaped at any point of time.

Fix inaccurate s3 rewrite rule. Reviewed-by: kuenishi

ksauzz · 2015-01-13T03:49:53Z

@borshop merge

shino · 2015-01-13T05:11:56Z

This also fixed #977 which described the case of % character and subsequent [0-9a-zA-Z]{2} as well as + sign (in client application layer).

Basho-JIRA · 2015-01-14T00:44:28Z

Reopening to get on sprint backlog - will close again shortly.

_[posted via JIRA by Deborah Rakow]_

Basho-JIRA · 2015-01-14T00:45:42Z

Re-closing after adding to sprint

_[posted via JIRA by Deborah Rakow]_

Conflicts: riak_test/src/rtcs.erl

ksauzz added 5 commits December 26, 2014 12:24

fix rewriting path inaccurately.

b762609

add src/riak_cs_s3_rewrite_legacy for backward compatibility.

3210e1f

extract test codes from riak_cs_s3_rewrite modules.

69305b1

fix wrong x-rcs-rewrite-path.

3bfdf3f

add riak_test for legacy s3 rewrite module.

d1c3d02

kuenishi reviewed Jan 7, 2015
View reviewed changes

ksauzz changed the title ~~WIP: fix inaccurate s3 rewrite rule.~~ Fix inaccurate s3 rewrite rule. Jan 7, 2015

kuenishi reviewed Jan 7, 2015
View reviewed changes

kuenishi added a commit that referenced this pull request Jan 7, 2015

Add eqc test on URL quote/unquote maze and regression for #1040

c9d0212

shino added this to the 1.5.4 milestone Jan 7, 2015

shino added the Bug label Jan 7, 2015

fix missing legacy s3 rewrite module on rewrite module list.

9c137e6

ksauzz and others added 7 commits January 8, 2015 11:17

remove a stall comment

886b2bb

Normalize urlencoded path-style bucketname.

40609c5

Add eqc test on URL quote/unquote maze and regression for #1040

976f335

Small eqc optimization: numtests to testing_time and parallelization

6c9953b

Fix wrong export in test

8d044a1

Add tests on path style

0aefa63

Fix comments

b144eed

kuenishi closed this Jan 9, 2015

kuenishi reopened this Jan 9, 2015

ksauzz added 2 commits January 12, 2015 21:27

extract functions to execute a external command on riak_test.

950ac69

remove unused code.

f4c58a4

Add minor description

b4cc578

Add minor description

1b9847b

borshop added a commit that referenced this pull request Jan 13, 2015

Merge pull request #1040 from basho/bugfix/rewriting-path-inaccurately

cc73f8c

Fix inaccurate s3 rewrite rule. Reviewed-by: kuenishi

borshop merged commit 1b9847b into release/1.5 Jan 13, 2015

shino mentioned this pull request Jan 13, 2015

Keys are doubly URL decoded, possibly key collision and list error #977

Closed

ksauzz mentioned this pull request Jan 13, 2015

Improvement to handle irregular strings as keyname. #910

Closed

ksauzz deleted the bugfix/rewriting-path-inaccurately branch January 13, 2015 06:58

Basho-JIRA added the JIRA: Closed label Jan 13, 2015

Basho-JIRA added JIRA: In Progress JIRA: Resolved and removed JIRA: Closed JIRA: In Progress labels Jan 14, 2015

Basho-JIRA added JIRA: Closed and removed JIRA: Resolved labels Jan 14, 2015

kuenishi added a commit that referenced this pull request Jan 15, 2015

Rewrite description on #1040

c3597b7

kuenishi added a commit that referenced this pull request Jan 16, 2015

Merge commit 'cc73f8c36ebb76bc22c' (pull request #1040) into develop

541d30e

Conflicts: riak_test/src/rtcs.erl

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix inaccurate s3 rewrite rule. #1040

Fix inaccurate s3 rewrite rule. #1040

ksauzz commented Jan 6, 2015

kuenishi Jan 7, 2015

kuenishi Jan 9, 2015

ksauzz Jan 12, 2015

kuenishi Jan 13, 2015

kuenishi Jan 13, 2015

kuenishi Jan 7, 2015

kuenishi commented Jan 7, 2015

kuenishi commented Jan 7, 2015

kuenishi Jan 7, 2015

kuenishi Jan 9, 2015

kuenishi commented Jan 7, 2015

kuenishi commented Jan 7, 2015

shino commented Jan 8, 2015

kuenishi commented Jan 9, 2015

shino commented Jan 9, 2015

kuenishi commented Jan 9, 2015

kuenishi commented Jan 9, 2015

kuenishi commented Jan 9, 2015

kuenishi commented Jan 13, 2015

shino commented Jan 13, 2015

ksauzz commented Jan 13, 2015

shino commented Jan 13, 2015

Basho-JIRA commented Jan 14, 2015

Basho-JIRA commented Jan 14, 2015


		-module(legacy_s3_rewrite_test).

		%% @doc `riak_test' module for testing object get behavior.


		-define(TEST_BUCKET, "legacy-s3-rewrite-test").

		confirm() ->

Fix inaccurate s3 rewrite rule. #1040

Fix inaccurate s3 rewrite rule. #1040

Conversation

ksauzz commented Jan 6, 2015

This PR includes:

Topics I'd like to discuss

How access log format changes after this patches.

TODO:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuenishi commented Jan 7, 2015

kuenishi commented Jan 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuenishi commented Jan 7, 2015

kuenishi commented Jan 7, 2015

shino commented Jan 8, 2015

kuenishi commented Jan 9, 2015

shino commented Jan 9, 2015

kuenishi commented Jan 9, 2015

kuenishi commented Jan 9, 2015

kuenishi commented Jan 9, 2015

kuenishi commented Jan 13, 2015

shino commented Jan 13, 2015

ksauzz commented Jan 13, 2015

shino commented Jan 13, 2015

Basho-JIRA commented Jan 14, 2015

Basho-JIRA commented Jan 14, 2015