New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: don't return skew time error in pre-signed url #13354

Merged
merged 1 commit into from Mar 14, 2017

Conversation

Projects
None yet
5 participants
@Liuchang0812
Contributor

Liuchang0812 commented Feb 10, 2017

rgw: don't return skew time in pre-signed url
Fixes: http://tracker.ceph.com/issues/18828

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>
@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 10, 2017

@yehudasa do you mind taking a look, thanks

@Liuchang0812 Liuchang0812 changed the title from rgw: don't return skew time in pre-signed url to rgw: don't return skew time error in pre-signed url Feb 10, 2017

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 10, 2017

I have tested it in my radosgw env, I generated a pre-signed URL that its expiry is 90000, more than our skew_time(7min). :

http://rgw-test.us-east-1.s3.amazonaws.com:8000/5m.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Expires=9000&X-Amz-SignedHeaders=host&X-Amz-Signature=4abed2f1622cdb5d9528f4392e8315cbb998c5c84aca81b50f60467ed87abd03&X-Amz-Date=20170210T121602Z&X-Amz-Credential=0343ADSQIMU3FIGK4LLT%2F20170210%2Fus-east-1%2Fs3%2Faws4_request`

I could download this file by this URL as

➜  build git:(wip-18828) ✗ wget "http://rgw-test.us-east-1.s3.amazonaws.com:8000/5m.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Expires=9000&X-Amz-SignedHeaders=host&X-Amz-Signature=4abed2f1622cdb5d9528f4392e8315cbb998c5c84aca81b50f60467ed87abd03&X-Amz-Date=20170210T121602Z&X-Amz-Credential=0343ADSQIMU3FIGK4LLT%2F20170210%2Fus-east-1%2Fs3%2Faws4_request" -O 1.txt
--2017-02-10 20:58:31--  http://rgw-test.us-east-1.s3.amazonaws.com:8000/5m.txt?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Expires=9000&X-Amz-SignedHeaders=host&X-Amz-Signature=4abed2f1622cdb5d9528f4392e8315cbb998c5c84aca81b50f60467ed87abd03&X-Amz-Date=20170210T121602Z&X-Amz-Credential=0343ADSQIMU3FIGK4LLT%2F20170210%2Fus-east-1%2Fs3%2Faws4_request
Resolving rgw-test.us-east-1.s3.amazonaws.com (rgw-test.us-east-1.s3.amazonaws.com)... 127.0.0.1
Connecting to rgw-test.us-east-1.s3.amazonaws.com (rgw-test.us-east-1.s3.amazonaws.com)|127.0.0.1|:8000... connected.
HTTP request sent, awaiting response... 200 OK
Length: 5242880 (5.0M) [application/octet-stream]
Saving to: ‘1.txt’

1.txt                                              100%[================================================================================================================>]   5.00M  --.-KB/s    in 0.01s   

2017-02-10 20:58:32 (395 MB/s) - ‘1.txt’ saved [5242880/5242880]

And, some logs I added for debug:

2017-02-10 20:58:31.994389 7fb475067700 20 get authorization from request params 
2017-02-10 20:58:31.994536 7fb475067700 20 NOTICE: now = 1486731511, now_req = 1486728962, exp = 9000
2017-02-10 20:58:31.994543 7fb475067700 20 request time is not skew.             

so, 1486731511 - 1486728962 = 42min, 42min > RGW_AUTH_GRACE_MINS(15min). it works.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 10, 2017

Sorry for compiling error, I will fix it soon

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 11, 2017

Fixed

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 12, 2017

I would like to add some unittest cases. Which files is proper to add unittest? @yehudasa @mattbenjamin

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Feb 15, 2017

@Liuchang0812 the right place would be in our s3-tests, I think

@mattbenjamin mattbenjamin self-assigned this Feb 15, 2017

@@ -3587,12 +3591,12 @@ int RGW_Auth_S3::authorize_v4(RGWRados *store, struct req_state *s, bool force_b
return -EPERM;
s->aws4_auth->expires = s->info.args.get("X-Amz-Expires");
if (s->aws4_auth->expires.size() != 0) {
if (!s->aws4_auth->expires.empty()) {

This comment has been minimized.

@mattbenjamin

mattbenjamin Feb 15, 2017

Contributor

yeah, prefer this

/* X-Amz-Expires provides the time period, in seconds, for which
the generated presigned URL is valid. The minimum value
you can set is 1, and the maximum is 604800 (seven days) */
time_t exp = atoll(s->aws4_auth->expires.c_str());
if ((exp < 1) || (exp > 604800)) {
if ((exp < 1) || (exp > 7*24*60*60)) {

This comment has been minimized.

@mattbenjamin

mattbenjamin Feb 15, 2017

Contributor

this is harmless, but not sure if it's preferable as the comment above elucidates the magic value, and so does AWS documentation [1]

[1] http://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html

This comment has been minimized.

@Liuchang0812

Liuchang0812 Feb 16, 2017

Contributor

need i reset this change?

@mattbenjamin

quibbles aside (log levels?), this looks good to me

@cbodley

This comment has been minimized.

Contributor

cbodley commented Feb 16, 2017

teuthology run was clean (only failures were selinux related)

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 22, 2017

@mattbenjamin @cbodley is it ok to be merged, please tell me if need do anything.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 22, 2017

@mattbenjamin I have looked at s3-tests, but if we create a test-case in s3-tests, we must sleep more then 15 minutes and then visit the pre-sign url. is it ok? it makes test time too long.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Feb 22, 2017

is there no way to give a custom Date header to boto to fake it?

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 22, 2017

@cbodley thanks for your suggestion. I will have a try.

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Feb 23, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 9, 2017

@Liuchang0812 we'd like to merge this, even though the boto changes are pending. could you open a pull request against https://github.com/ceph/s3-tests that uses that fixed branch of boto?

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Mar 10, 2017

@cbodley roger that

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Mar 11, 2017

@cbodley could I add a git sub-module in s3-tests to use my boto branch? I tried to fix branch in requirements.txt, but it seems doesn't work.

@cbodley

This comment has been minimized.

Contributor

cbodley commented Mar 13, 2017

@Liuchang0812 i don't think we need to deal with the dependency for now. just push the s3tests part, and we can move forward once the boto change gets in

@Liuchang0812

This comment has been minimized.

Contributor

Liuchang0812 commented Mar 14, 2017

@cbodley I pushed a PR to s3-tests ceph/s3-tests#155

@cbodley cbodley merged commit c952e85 into ceph:master Mar 14, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Jun 3, 2017

Noting for posterity that http://tracker.ceph.com/issues/18829 claims that this PR fixes it, although the commit message says it fixes only http://tracker.ceph.com/issues/18828

Thus, the jewel backports of http://tracker.ceph.com/issues/18828 and http://tracker.ceph.com/issues/18829 were resolved by cherry-picking just this one commit. Hopefully that is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment