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: validate tenant names during user create. #16442

Merged
merged 3 commits into from Aug 2, 2017

Conversation

Projects
None yet
5 participants
@theanalyst
Member

theanalyst commented Jul 20, 2017

This set of commits does tenant name validation for RGW during
user creation in rgw-admin/adminops REST requests.

  • Move validate_tenant_name under rgw_user and reuse the same
    function during user validation
  • Document that we already allow tenant names to be specified in user
    create APIs and document the response elements as well
  • Introduce a new tenant param in user creation adminops API, we
    however still return EINVAL instead of ERR_INVALID_TENANT, we could
    introduce this as well, this however will require rgw_admin to reraise
    ERR_INVALID_TENANT as EINVAL to keep the cli err return codes consistent

@theanalyst theanalyst requested review from cbodley and pritha-srivastava Jul 20, 2017

@theanalyst

This comment has been minimized.

Member

theanalyst commented Jul 20, 2017

jenkins test this please

@theanalyst

This comment has been minimized.

Member

theanalyst commented Jul 20, 2017

Related to #16418

@@ -1850,6 +1861,12 @@ int RGWUser::check_op(RGWUserAdminOpState& op_state, std::string *err_msg)
return -EINVAL;
}
if (rgw_is_valid_tenant(op_id.tenant)) {

This comment has been minimized.

@theanalyst

theanalyst Jul 20, 2017

Member

we could either return ERR_INVALID_TENANT_NAME as returned by the function and reraise in rgw_admin.cc , or abort early in rgw_rest_user.cc doing the check.

This comment has been minimized.

@cbodley

cbodley Jul 20, 2017

Contributor

not sure exactly what you mean here, but i think this should be:

  ret = rgw_is_valid_tenant(op_id.tenant);
  if (ret < 0) {
    set_err_msg(err_msg, ...);
    return ret;
  }

past that, it doesn't look like rgw_admin.cc or rgw_rest_user.cc need to do anything special?

This comment has been minimized.

@theanalyst

theanalyst Jul 20, 2017

Member

well the only issue with that is that rgw_admin would exit with a not so relevant exit code (instead of -EINVAL)

This comment has been minimized.

@cbodley

cbodley Jul 20, 2017

Contributor

true, it's probably worth mapping that to EINVAL.. but at least we'll print a concise error message

This comment has been minimized.

@theanalyst

theanalyst Jul 20, 2017

Member

ack, will update

@theanalyst theanalyst changed the title from rgw: validate tenant names during user create. to wip: rgw: validate tenant names during user create. Jul 20, 2017

std::string::const_iterator it =
std::find_if_not(t.begin(), t.end(), tench::is_good);
return (it == t.end())? 0: -ERR_INVALID_TENANT_NAME;
return rgw_is_valid_tenant(t);

This comment has been minimized.

@cbodley

cbodley Jul 20, 2017

Contributor

i think we're better off removing RGWHandler_REST::validate_tenant_name altogether, and changing any callers to use the new free function directly

This comment has been minimized.

@theanalyst

theanalyst Jul 20, 2017

Member

ack makes sense

@@ -542,6 +542,17 @@ uint32_t rgw_str_to_perm(const char *str)
return RGW_PERM_INVALID;
}
int rgw_is_valid_tenant(const string& t){

This comment has been minimized.

@cbodley

cbodley Jul 20, 2017

Contributor

i think rgw_validate_tenant_name is a better fit for this interface, since it either returns 0 or -ERR_INVALID_TENANT_NAME. seeing the name is_valid_tenant, i would expect it to return true/false

This comment has been minimized.

@theanalyst
@@ -1850,6 +1861,12 @@ int RGWUser::check_op(RGWUserAdminOpState& op_state, std::string *err_msg)
return -EINVAL;
}
if (rgw_is_valid_tenant(op_id.tenant)) {

This comment has been minimized.

@cbodley

cbodley Jul 20, 2017

Contributor

not sure exactly what you mean here, but i think this should be:

  ret = rgw_is_valid_tenant(op_id.tenant);
  if (ret < 0) {
    set_err_msg(err_msg, ...);
    return ret;
  }

past that, it doesn't look like rgw_admin.cc or rgw_rest_user.cc need to do anything special?

@theanalyst theanalyst changed the title from wip: rgw: validate tenant names during user create. to rgw: validate tenant names during user create. Jul 21, 2017

@theanalyst

This comment has been minimized.

Member

theanalyst commented Jul 23, 2017

test this please

@@ -542,6 +542,17 @@ uint32_t rgw_str_to_perm(const char *str)
return RGW_PERM_INVALID;
}
int rgw_validate_tenant_name(const string& t){

This comment has been minimized.

@cbodley

cbodley Jul 24, 2017

Contributor

nit: space or newline before {

This comment has been minimized.

@theanalyst
int ret = rgw_validate_tenant_name(op_id.tenant);
if (ret) {
set_err_msg(err_msg,
"invalid tenant only alphanumeric & _ characters are allowed");

This comment has been minimized.

@cbodley

cbodley Jul 24, 2017

Contributor

and instead of & here, so it's not confused as a valid character?

This comment has been minimized.

@theanalyst
@@ -108,6 +110,10 @@ void RGWOp_User_Create::execute()
return;
}
if (!tenant_name.empty()){

This comment has been minimized.

@cbodley

cbodley Jul 24, 2017

Contributor

nit: space before {

theanalyst added some commits Jul 19, 2017

rgw_user: validate tenant names when creating user
Also moved `validate_tenant_name` function from RGWHandler_REST to
rgw_user, a new function called `rgw_validate_tenant_name` validates
tenant names. There is special handling in rgw_admin to reraise
-ERR_INVALID_TENANT to -EINVAL

Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
doc: mention that tenant names may be specified in adminops api
We already allow for tenant names to be specified as a part of uid in
the adminops api, mention this. Also mention about ``tenant`` in the
json response we send.

Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
rgw: adminops API now supports tenant param for user creation
Allow `tenant` as a param for user creation API, also document this.
Currently we still return a -ENOENT when an invalid tenant name is
specified, while we could make it return -ERR_INVALID_TENANT, this would
make rgw admin cli not return -ENOENT when an invalid tenant name is
specified.

Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
@theanalyst

This comment has been minimized.

Member

theanalyst commented Jul 24, 2017

@cbodley updated

@cbodley cbodley added the needs-qa label Jul 24, 2017

@yuriw yuriw merged commit 0a3056d into ceph:master Aug 2, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment