rgw: Rework of s3 LDAP Authentication code. #10307
Conversation
|
Things pending:
|
| { | ||
| //This is based on the assumption that the default acl strategy in | ||
| // get_perms_from_aclspec, will take care. Extra acl spec is not required. | ||
| return nullptr; |
There was a problem hiding this comment.
I think that -should- be true
|
At a high level, this looks very good. Class definitions can go wherever, we need to support building with and without LDAP support. The code not removed is the actual auth hooks? Does this code actually run, then? I think the current ACL handling is correct, but :) |
|
@mattbenjamin : This code runs. I replaced the logic in authorize_v2 with something similar to what is there for the swift authorize method and tested it, though I removed the test code before creating the PR. |
|
@mattbenjamin : The code not removed is the piece which is there in authorize_v2. That bit can be removed when we have all the S3 AuthEngines in place. And the hooks like RGW_Auth_S3::init and init_impl and the respective variables. |
|
@mattbenjamin : The code to build with and without ldap support is in rgw_ldap.h ( #if defined(HAVE_OPENLDAP)) . Is this what you were looking for or you meant something else? |
pritha-srivastava
force-pushed
the
wip_s3_auth_rework
branch
from
03dbe27
to
1d18215
Compare
|
Added code for specifying 'type' with the user. Also replaced some parts of the old code with the new AuthEngine and AuthApplier pieces. |
| @@ -149,6 +149,11 @@ void RGWRemoteAuthApplier::create_account(const rgw_user& acct_user, | |||
| { | |||
| rgw_user new_acct_user = acct_user; | |||
|
|
|||
| if (!info.acct_type.empty()) { | |||
|
The only significant point from my review is, I think, maybe preferring a non-string (perhaps an enumeration type) for account_type? Past that, is this ready to test? |
pritha-srivastava
force-pushed
the
wip_s3_auth_rework
branch
from
1d18215
to
9fbcf41
Compare
|
@mattbenjamin : Reworked the 'type' related code based on your comment. Also, replaced other pieces of 'old' ldap code with the new one. And now have completely removed the old ldap code. This is ready to be tested. |
| enum RGWUserType | ||
| { | ||
| TYPE_NONE=0, | ||
| TYPE_KEYSTONE, |
There was a problem hiding this comment.
@pritha-srivastava I'd put rgw first, then keystone and then ldap. Also, would make the value explicit (even though probably redundant)
pritha-srivastava
force-pushed
the
wip_s3_auth_rework
branch
from
9fbcf41
to
66d51b3
Compare
pritha-srivastava
force-pushed
the
wip_s3_auth_rework
branch
from
66d51b3
to
1229058
Compare
|
@yehudasa : I have made changes related to the user source type. Can you please re-review. |
| @@ -427,6 +427,7 @@ void RGWUserInfo::dump(Formatter *f) const | |||
| encode_json("bucket_quota", bucket_quota, f); | |||
| encode_json("user_quota", user_quota, f); | |||
| encode_json("temp_url_keys", temp_url_keys, f); | |||
| encode_json("type", type, f); | |||
There was a problem hiding this comment.
@pritha-srivastava maybe json encode this enum as a string, not as an int (e.g., look at rgw_meta_sync_info)
The LDAP authentication code has been reworked based on the new authentication infrastructure. Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
pritha-srivastava
force-pushed
the
wip_s3_auth_rework
branch
from
1229058
to
bcdc2df
Compare
|
This will need a rebase due to refactoring for rebind(). I can do it, if you like? |
|
I've verified the LDAP auth behavior, including rebind. The one potential problem is related to the new stored user type: external-auth users already present in a cluster (e.g., RHCS 2.0) will be unable to authenticate. I worked around this by removing said users. We need a ruling on whether we need any accommodation (e.g., set user type?) |
mattbenjamin
changed the title
The LDAP authentication code has been reworked based
on the new authentication infrastructure.
Signed-off-by: Pritha Srivastava prsrivas@redhat.com