Skip to content
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: fix Swift container naming rules. #13992

Merged
merged 1 commit into from Nov 2, 2017
Merged

Conversation

robbat2
Copy link
Contributor

@robbat2 robbat2 commented Mar 16, 2017

Swift containers are declared in the upstream documentation as 1-256
bytes of UTF-8, and the only forbidden character is '/'.

We did not previously enforce the no-slash rule.

See-Also: https://docs.openstack.org/developer/swift/api/object_api_v1_overview.html
Fixes: http://tracker.ceph.com/issues/19264
Backport: hammer, jewel
Signed-off-by: Robin H. Johnson robin.johnson@dreamhost.com

@oritwas
Copy link
Member

oritwas commented Mar 16, 2017

@robbat2 , can you add a test for this in swift test please

@@ -2228,6 +2228,8 @@ int RGWHandler_REST_SWIFT::validate_bucket_name(const string& bucket)
for (int i = 0; i < len; ++i, ++s) {
if (*(unsigned char *)s == 0xff)
return -ERR_INVALID_BUCKET_NAME;
if (*(unsigned char *)s == '/')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robbat2 how about putting this check in RGWHandler_REST::validate_bucket_name? S3 also needs this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The S3 path doesn't call RGWHandler_REST::validate_bucket_name, but I did add it for the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I will add RGWHandler_REST::validate_bucket_name to S3

Swift containers are declared in the upstream documentation as 1-256
bytes of UTF-8, and the only forbidden character is '/'.

We did not previously enforce the no-slash rule.

Also ensure that the common validate_bucket_name does not permit '/' or
0xFF either (it's also banned in S3).

See-Also: https://docs.openstack.org/developer/swift/api/object_api_v1_overview.html
Fixes: http://tracker.ceph.com/issues/19264
Backport: hammer, jewel
Signed-off-by: Robin H. Johnson <robin.johnson@dreamhost.com>
@oritwas
Copy link
Member

oritwas commented Mar 30, 2017

jenkins test this please

@cbodley
Copy link
Contributor

cbodley commented Apr 4, 2017

@cbodley
Copy link
Contributor

cbodley commented Apr 4, 2017

jenkins test this please

if (*(unsigned char *)s == '/')
return -ERR_INVALID_BUCKET_NAME;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use cpp style as

for (auto c : bucket) {
    if ( c == 0xff ||
         c == '/')
       return -ERR_INVALID_BUCKET_NAME;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason I suppose, my style just shows how I learnt C++ ~20 years ago.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a suggestion, This code looks good to me 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do prefer the ranged-for loop, with the small adjustment of

for (unsigned char c : bucket) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably drop the whole C++ -> C string conversion and just do a string.find() or probably even using std::any_of

@oritwas
Copy link
Member

oritwas commented Apr 6, 2017

jenkins test this please

@theanalyst
Copy link
Member

@robbat2 I reraised this and made it configurable at #18219 which addresses another issue when "/" end up being used in existing rgw

@yuriw
Copy link
Contributor

yuriw commented Nov 1, 2017

test this please

@yuriw
Copy link
Contributor

yuriw commented Nov 1, 2017

@yuriw yuriw merged commit 3c52884 into ceph:master Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants