Fix/create temp url escaping by Lirt · Pull Request #1994 · gophercloud/gophercloud

Lirt · 2020-07-24T15:08:16Z

For #1993

Fix issue with generating temporary URLs, which are invalid when conatiner or object name contains characters, which are subject to query escaping.

- #1993 - URL MUST NOT be encoded prior to hashing - From Swift docs - Do not URL-encode the path when you generate the HMAC-SHA1 signature Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Lirt · 2020-07-24T15:10:27Z

For tests to work, time.Now() in function CreateTempURL must always return consistent value https://github.com/gophercloud/gophercloud/blob/master/openstack/objectstorage/v1/objects/requests.go#L467.

I know that time cannot be mocked, but there are other techniques that could be done. Can you help me with this @kayrus?

Also let me know what you think about this approach to unit test. If you think we can test this easier, I can rewrite it.

Edit: Also I can add one more test with slashes in object name (which caused the original issue)

openstack/objectstorage/v1/objects/testing/requests_test.go

- This test should generate URL with consistent signature and expiry and then be asserted Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

theopenlab-ci · 2020-07-24T17:02:10Z

Build succeeded.

gophercloud-unittest : SUCCESS in 4m 07s
gophercloud-acceptance-test : SUCCESS in 1h 39m 10s
gophercloud-acceptance-test-ironic : SUCCESS in 46m 01s

kayrus · 2020-07-24T17:27:37Z

For tests to work, time.Now() in function CreateTempURL must always return consistent value https://github.com/gophercloud/gophercloud/blob/master/openstack/objectstorage/v1/objects/requests.go#L467.

I'd split this func and use the major logic func in unit tests.

I wonder how terraform functional tests passed this, and noticed that object name doesn't contain the slash: https://github.com/terraform-providers/terraform-provider-openstack/blob/152286f9ae3d016cb38d53ae2c0d1920d967d368/openstack/resource_openstack_objectstorage_tempurl_v1_test.go#L14
In addition it would be great to have functional tests in gophercloud as well.

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

This gives enough flexibility to stub time and perform unit tests Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Lirt · 2020-07-24T21:46:24Z

For tests to work, time.Now() in function CreateTempURL must always return consistent value https://github.com/gophercloud/gophercloud/blob/master/openstack/objectstorage/v1/objects/requests.go#L467.

I'd split this func and use the major logic func in unit tests.

I'm sorry, but I don't see way how to split it and use it without adding new argument for time to be able to stub it.

What I came up after some research is allowing to change function of time at package level (https://labs.yulrizka.com/en/stubbing-time-dot-now-in-golang/) - f4b9e4b.

Feel free to update the MR with how you see it.

…added Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

coveralls · 2020-07-24T22:38:08Z

Coverage increased (+0.1%) to 79.512% when pulling c93e05f on Lirt:fix/create-temp-url-escaping into bd999d0 on gophercloud:master.

theopenlab-ci · 2020-07-25T01:16:33Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 49s
gophercloud-acceptance-test : FAILURE in 2h 48m 34s
gophercloud-acceptance-test-ironic : SUCCESS in 51m 56s

openstack/objectstorage/v1/objects/testing/requests_test.go

kayrus · 2020-07-25T06:09:12Z

openstack/objectstorage/v1/objects/requests.go

 	}
 	duration := time.Duration(opts.TTL) * time.Second
-	expiry := time.Now().Add(duration).Unix()
+	expiry := timeNow().Add(duration).Unix()


I wonder how did it work before without UTC()

Good question, I didn't think about that. I was reading docs, and it says Unix() should be consistent in any way ⬇️.

Unix() The result does not depend on the location associated with t. Unix-like operating systems
https://golang.org/pkg/time/#Time.Unix

So I can actually revert back to Now() if it looks better for you.

https://play.golang.org/p/so0mPFXWY7m

Not sure what should be used. @jtopjian please advice.

Can we resolve as discussed below?
It was commited here 9a500de

theopenlab-ci · 2020-07-25T07:47:42Z

Build succeeded.

gophercloud-unittest : SUCCESS in 2m 59s
gophercloud-acceptance-test : SUCCESS in 1h 42m 18s
gophercloud-acceptance-test-ironic : SUCCESS in 28m 35s

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

theopenlab-ci · 2020-07-25T11:05:34Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 19s
gophercloud-acceptance-test : SUCCESS in 2h 25m 38s
gophercloud-acceptance-test-ironic : SUCCESS in 34m 38s

openstack/objectstorage/v1/objects/testing/requests_test.go

theopenlab-ci · 2020-07-25T16:33:03Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 57s
gophercloud-acceptance-test : SUCCESS in 2h 20m 11s
gophercloud-acceptance-test-ironic : SUCCESS in 46m 48s

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

theopenlab-ci · 2020-07-27T12:42:01Z

Build succeeded.

gophercloud-unittest : SUCCESS in 3m 53s
gophercloud-acceptance-test : SUCCESS in 1h 34m 54s
gophercloud-acceptance-test-ironic : SUCCESS in 29m 14s

Lirt · 2020-07-27T14:07:45Z

Had to fix some issues with Time packaging, but it works now correctly. Question is if this way is OK for you maintainers. As I said I didn't find better solution yet, but I am open to suggestions.

I will also add 1 integration test, to cover this function better, but I think it will take some time, since it will be my first.

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

theopenlab-ci · 2020-07-30T02:14:08Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 36s
gophercloud-acceptance-test : TIMED_OUT in 3h 01m 48s
gophercloud-acceptance-test-ironic : SUCCESS in 50m 27s

kayrus · 2020-07-30T06:36:05Z

acceptance/openstack/objectstorage/v1/objects_test.go

+
+		resp, err := http.Get(objURLs[i])
+		th.AssertNoErr(t, err)
+		defer resp.Body.Close()


not critical, but better to call resp.Body.Close() at the end of the loop. otherwise go will close the connection only when the test function exits.

Fixed in 22628a7

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

kayrus · 2020-07-30T07:28:41Z

openstack/objectstorage/v1/objects/requests.go

 	Split string
 }

+type nowFuncT func() time.Time


What if add the date into the CreateTempURLOpts struct? If it is nil, then a default time.Now() is used. If not, the provided value is used. This will avoid hacking around the ResetClockImplementation

See here for example: https://github.com/gophercloud/gophercloud/blob/v0.12.0/openstack/identity/v3/extensions/ec2tokens/requests.go#L74
https://github.com/gophercloud/gophercloud/blob/v0.12.0/openstack/identity/v3/extensions/ec2tokens/requests.go#L268

Sounds good to me

Please check 9a500de

Notice I didn't use pointer to time.Time because as documentation says:

Programs using times should typically store and pass them as values, not pointers. That is, time variables and struct fields should be of type time.Time, not *time.Time.

But I used isZero() to check for null value.

https://golang.org/pkg/time/#Time

I looks much cleaner when passing time as opts :-)

- This way is less hacky than packaging time Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

theopenlab-ci · 2020-07-30T11:54:21Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 44s
gophercloud-acceptance-test : TIMED_OUT in 3h 01m 28s
gophercloud-acceptance-test-ironic : SUCCESS in 52m 42s

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

theopenlab-ci · 2020-07-30T16:59:11Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 59s
gophercloud-acceptance-test : FAILURE in 2h 53m 07s
gophercloud-acceptance-test-ironic : SUCCESS in 50m 14s

Lirt · 2020-08-03T08:27:39Z

Acceptance test for the function was successful, but some other failed, doing recheck.

Lirt · 2020-08-03T08:29:06Z

recheck

theopenlab-ci · 2020-08-03T10:07:33Z

Build failed.

gophercloud-unittest : SUCCESS in 3m 45s
gophercloud-acceptance-test : FAILURE in 1h 37m 13s
gophercloud-acceptance-test-ironic : SUCCESS in 20m 20s

Lirt · 2020-08-03T10:36:43Z

@kayrus Can you please look at the pipeline? I'm not sure if I should just run recheck, or there is something wrong. I think it is failing on something different than my code.

2020-08-03 10:06:35.176116 | ubuntu-bionic | --- SKIP: TestMessageDelete (0.19s)
2020-08-03 10:06:35.176174 | ubuntu-bionic |     messages_test.go:91: No messages were found
2020-08-03 10:06:35.176219 | ubuntu-bionic | PASS
2020-08-03 10:06:35.177855 | ubuntu-bionic | ok  	github.com/gophercloud/gophercloud/acceptance/openstack/sharedfilesystems/v2/messages	0.531s
2020-08-03 10:06:35.201255 | ubuntu-bionic | + ./script/acceptancetest:main:101         :   [[ 0 != 0 ]]
2020-08-03 10:06:35.204759 | ubuntu-bionic | + ./script/acceptancetest:main:107         :   [[ -n 1 ]]
2020-08-03 10:06:35.206948 | ubuntu-bionic | + ./script/acceptancetest:main:108         :   exit 1
2020-08-03 10:06:39.396148 | ubuntu-bionic | ERROR
2020-08-03 10:06:39.397095 | ubuntu-bionic | {
2020-08-03 10:06:39.397221 | ubuntu-bionic |   "delta": "0:45:12.849894",
2020-08-03 10:06:39.397348 | ubuntu-bionic |   "end": "2020-08-03 10:06:38.006579",
2020-08-03 10:06:39.397430 | ubuntu-bionic |   "msg": "non-zero return code",
2020-08-03 10:06:39.397508 | ubuntu-bionic |   "rc": 1,
2020-08-03 10:06:39.397628 | ubuntu-bionic |   "start": "2020-08-03 09:21:25.156685"
2020-08-03 10:06:39.397715 | ubuntu-bionic | }

Lirt · 2020-08-03T15:14:25Z

recheck

theopenlab-ci · 2020-08-03T17:54:16Z

Build failed.

gophercloud-unittest : SUCCESS in 4m 01s
gophercloud-acceptance-test : FAILURE in 2h 37m 26s
gophercloud-acceptance-test-ironic : SUCCESS in 50m 44s

jtopjian · 2020-08-03T19:56:50Z

The acceptance test failure should be fixed when #2000 is merged.

Lirt · 2020-08-03T20:00:41Z

@jtopjian Thank you.

I'm pretty much done with this MR. You can do review when you have time for it.

jtopjian · 2020-08-05T02:54:45Z

@Lirt Thanks! This looks good to me.

@kayrus are you OK with this?

kayrus · 2020-08-05T07:33:12Z

@jtopjian LGTM as well. @Lirt, thanks a lot for your contribution.

Lirt · 2020-08-05T07:53:24Z

Thank you for review and help with code @jtopjian and @kayrus.

jtopjian

LGTM - thank you!

Remove container and object escaping from CreateTempURL

0350222

- #1993 - URL MUST NOT be encoded prior to hashing - From Swift docs - Do not URL-encode the path when you generate the HMAC-SHA1 signature Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Lirt commented Jul 24, 2020

View reviewed changes

openstack/objectstorage/v1/objects/testing/requests_test.go Outdated Show resolved Hide resolved

Implement unit test for CreateTempURL

f30ec80

- This test should generate URL with consistent signature and expiry and then be asserted Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Lirt added 4 commits July 24, 2020 22:37

refactoring: Statically define HTTP port in test

793ef20

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Allow stubbing of time on package level for object-storage

f4b9e4b

This gives enough flexibility to stub time and perform unit tests Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Stub time to static value in test for function CreateTempURL

8ae1f56

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Use complex object name with pseudo hierarchical path

9c9e64d

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Update expected test results in accounts where TempUrlKey secret was …

591120e

…added Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

kayrus reviewed Jul 25, 2020

View reviewed changes

openstack/objectstorage/v1/objects/testing/requests_test.go Outdated Show resolved Hide resolved

kayrus reviewed Jul 25, 2020

View reviewed changes

Use different persistent port as in other tests to avoid collisions

de49aa2

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

kayrus reviewed Jul 25, 2020

View reviewed changes

openstack/objectstorage/v1/objects/testing/requests_test.go Show resolved Hide resolved

Reset clock implementation in stubbed clock

43384b5

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Add acceptance test for CreateTempURL

6fc330b

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

kayrus reviewed Jul 30, 2020

View reviewed changes

Lirt added 2 commits July 30, 2020 09:09

Close response body manually rather than deferred

22628a7

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Return byte array from bytes buffer in comparison

7c65d55

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

kayrus reviewed Jul 30, 2020

View reviewed changes

Allow testing of CreateTempURL by passing time.Time as opts

9a500de

- This way is less hacky than packaging time Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

Use assert deep equals to compare complex objects such as byte arrays

c93e05f

Signed-off-by: Ondrej Vasko <ondrej.vaskoo@gmail.com>

jtopjian approved these changes Aug 5, 2020

View reviewed changes

jtopjian merged commit e4a74d0 into gophercloud:master Aug 5, 2020

Conversation

Lirt commented Jul 24, 2020

Uh oh!

Lirt commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

theopenlab-ci bot commented Jul 24, 2020

Uh oh!

kayrus commented Jul 24, 2020

Uh oh!

Lirt commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Jul 25, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lirt Jul 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented Jul 25, 2020

Uh oh!

theopenlab-ci bot commented Jul 25, 2020

Uh oh!

Uh oh!

theopenlab-ci bot commented Jul 25, 2020

Uh oh!

theopenlab-ci bot commented Jul 27, 2020

Uh oh!

Lirt commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theopenlab-ci bot commented Jul 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lirt Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lirt Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci bot commented Jul 30, 2020

Uh oh!

theopenlab-ci bot commented Jul 30, 2020

Uh oh!

Lirt commented Aug 3, 2020

Uh oh!

Lirt commented Aug 3, 2020

Uh oh!

theopenlab-ci bot commented Aug 3, 2020

Uh oh!

Lirt commented Aug 3, 2020

Uh oh!

Lirt commented Aug 3, 2020

Uh oh!

theopenlab-ci bot commented Aug 3, 2020

Uh oh!

Lirt commented Jul 24, 2020 •

edited

Loading

Lirt commented Jul 24, 2020 •

edited

Loading

coveralls commented Jul 24, 2020 •

edited

Loading

Lirt Jul 25, 2020 •

edited

Loading

Lirt commented Jul 27, 2020 •

edited

Loading

Lirt Jul 30, 2020 •

edited

Loading

Lirt Jul 30, 2020 •

edited

Loading