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: continuation of the auth rework #12893

Merged
merged 48 commits into from Mar 26, 2017

Conversation

Projects
None yet
4 participants
@rzarzynski
Copy link
Contributor

rzarzynski commented Jan 12, 2017

Marking as DNM because:

  • S3 errors codes should be differentiated - now we're mostly returning just AccessDenied while s3-tests require more detailed info (eg. SignatureDoesNotMatch),
  • some auth engines (rgw::auth::s3::LDAPEngine, rgw::auth::swift::SignedTokenEngine, rgw::auth::swift::ExternalTokenEngine and so on) need to be tested,
  • rebase to master is necessary.

CC: @mattbenjamin, @cbodley, @pritha-srivastava.

@rzarzynski rzarzynski added the rgw label Jan 12, 2017

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-auth-rework-cont-2 branch from 1c49025 to 4045423 Jan 12, 2017

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Jan 18, 2017

@pritha-srivastava: I've added ea6c1d1. Now the s3-tests look much better.

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-auth-rework-cont-2 branch 2 times, most recently from 376c9fb to c88c357 Feb 3, 2017

@rzarzynski rzarzynski changed the title [DNM] rgw: continuation of the auth rework rgw: continuation of the auth rework Feb 7, 2017

@mattbenjamin mattbenjamin self-assigned this Mar 16, 2017

@mattbenjamin mattbenjamin self-requested a review Mar 16, 2017

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

mattbenjamin commented Mar 16, 2017

@rzarzynski I think merge is relatively trivial, but we should still rebase?

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

mattbenjamin commented Mar 16, 2017

git@github.com:linuxbox2/ceph.git
rzarzynski-wip-rgw-auth-rework-cont-2

}

/* Allow only the reasonable combintations - returning just Completer
* without ccompanying IdentityApplier is strictly prohibited! */

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: typo.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.


public:
enum class Status {
/* Engine does */

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: this must be documented.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

* it the one that failed wasn't the last one. */
SUFFICIENT,

/* */

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: document this.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

typedef std::function<uint32_t(const aclspec_t&)> acl_strategy_t;

protected:
CephContext * const cct;

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: move the * sign.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.


static_assert(std::is_base_of<rgw::auth::IdentityApplier,
DerefedDecorateeT>::value,
"DecorateeT must be a subclass of rgw::io::RestfulClient");

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: oops, wrong assert message.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

* what we store internally: pointer to a decorated object versus the whole
* object itself. */
template <typename T = void,
typename std::enable_if<

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

Hmm, std::enable_if<> & friends have a a great potential to confuse people. Maybe just adding SFINAE (or similar keyword) to the comment above could make googling easier.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

DecoratedApplier<T>::load_acct_info(user_info);
} else {
/* Compatibility mechanism for multi-tenancy. For more details refer to
* load_acct_info method of RGWRemoteAuthApplier. */

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: update RGWRemoteAuthApplier.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

/* This will be initialized on the first call to this method. In C++11 it's
* also thread-safe. */
static const struct RolesCacher {
RolesCacher(CephContext * const cct) {

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: move the * sign.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

}

using RGWValidateKeystoneToken
= rgw::keystone::Service::RGWValidateKeystoneToken;

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

This looks ugly. Maybe something like:

  using RGWValidateKeystoneToken = \
    rgw::keystone::Service::RGWValidateKeystoneToken;

This comment has been minimized.

Copy link
@cbodley

cbodley Mar 16, 2017

Contributor

if you're not changing the name itself, just:

using rgw::keystone::Service::RGWValidateKeystoneToken;

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

This is much, much better! I've completely forgotten about this possibility. Thanks, Casey!

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

This must be kept as rgw::keystone::Service is not a namespace but a class. :-(

/work/ceph-4/src/rgw/rgw_auth_keystone.cc: In member function ‘boost::optional<rgw::keystone::TokenEnvelope> rgw::auth::keystone::TokenEngine::get_from_keystone(const string&) const’:
/work/ceph-4/src/rgw/rgw_auth_keystone.cc:65:33: error: ‘rgw::keystone::Service’ is not a namespace
   using rgw::keystone::Service::RGWValidateKeystoneToken;

I will add a comment with the explanation.


/* create json credentials request body */
JSONFormatter credentials(false);
credentials.open_object_section("");

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

TODO: clean-up to a separated function.

void KeystoneAdminTokenRequestVer2::dump(Formatter * const f) const
/* This utility function shouldn't conflict with the overload of std::to_string
* provided by string_ref since Boost 1.54 as it's defined outside of the std
* namespace. I hope we'll remove it soon - just after merging the Matt's PR

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

The PR is merged. :-)

if (!s->auth_identity->is_admin_of(src_policy.get_owner().get_id()) &&
!src_policy.verify_permission(*s->auth_identity, s->perm_mask,
if (! s->auth.identity->is_admin_of(src_policy.get_owner().get_id()) &&
! src_policy.verify_permission(*s->auth.identity, s->perm_mask,
RGW_PERM_READ)) {

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME: indentation.

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

ldout(s->cct, 0) << "No S3 access key found!" << dendl;
err_msg = "Missing access key";
return -EINVAL;
}
string received_signature_str;
if (!part_str("signature", &received_signature_str)) {
if (!part_str("signature", &s->auth.s3_postobj_creds.signature)) {

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME:

if (! part_str("signature", &s->auth.s3_postobj_creds.signature)) {

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

{
using acct_privilege_t = RGWRemoteAuthApplier::AuthInfo::acct_privilege_t;
using acct_privilege_t = \

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 16, 2017

Author Contributor

FIXME:

using rgw::auth::RemoteApplier::AuthInfo::acct_privilege_t;

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE: commented that the short form of using can't be used here.

@mattbenjamin

This comment has been minimized.

Copy link
Contributor

mattbenjamin commented Mar 16, 2017

@rzarzynski are any of the above changes more than cosmetic? RGWRemoteAuthApplier? What is the scope of missing behavior?

* of the request. Typical use case is verifying message integrity
* in AWS Auth v4 and browser uploads (RGWPostObj).
*
* The authentication process consists two steps:

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Mar 16, 2017

Contributor

"consists of"

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

* in AWS Auth v4 and browser uploads (RGWPostObj).
*
* The authentication process consists two steps:
* - Engine::authenticate() supposed to be called before *initiating*

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Mar 16, 2017

Contributor

s/supposed/should be/

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

* - Engine::authenticate() supposed to be called before *initiating*
* any modifications to RADOS store that are related to an operation
* a client wants to perform (RGWOp::execute).
* - Completer::complete() supposed to be called, if completer has been

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Mar 16, 2017

Contributor

ditto

/* Success of an engine injected with the SUFFICIENT specifier ends
* the whole authentication process successfully. However, failure
* doesn't abort it - there will be fall-back to following engine
* it the one that failed wasn't the last. */

This comment has been minimized.

Copy link
@mattbenjamin

mattbenjamin Mar 16, 2017

Contributor

s/it/if/

This comment has been minimized.

Copy link
@rzarzynski

rzarzynski Mar 21, 2017

Author Contributor

DONE.

rzarzynski added some commits Jan 11, 2017

rgw: move ACL Strategies to the newer auth framework.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: remove the parts of auth framework that aren't necessary anymore.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: unify handling S3's ::authorize_v2 and ::get_policy methods.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: implement descriptive authentication failure reasons.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: refactor error handling in rgw::auth::keystone.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: RGWOp is responsible now for the authentication process.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: improve logs in rgw::auth::Strategy.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: improve logs in the RGWAccessControlPolicy class.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-auth-rework-cont-2 branch from c88c357 to f8ec625 Mar 24, 2017

rzarzynski added some commits Feb 7, 2017

rgw: fix appending '\0' in the rgw::auth::LocalApplier::to_str().
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: RGWPostObj_ObjStore_S3 doesn't instantiate auth strategy for eac…
…h request anymore.

Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: parametrize the implicit tenancy of rgw::auth::RemoteApplier.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: implement SwiftAnonymousEngine.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: the S3's local v2 auth engine becomes a fallback conditionally.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>
rgw: implement the dynamic reconfiguration of auth strategies.
Signed-off-by: Radoslaw Zarzynski <rzarzynski@mirantis.com>

@rzarzynski rzarzynski force-pushed the rzarzynski:wip-rgw-auth-rework-cont-2 branch from f8ec625 to 9888ec3 Mar 24, 2017

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Mar 24, 2017

@mattbenjamin: just pushed the rebased branch with fixes squashed in. I will schedule a Teuthology run soon.

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Mar 25, 2017

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Mar 26, 2017

@mattbenjamin: because of the very long queue on smithi, I've scheduled the additional run on mira. Generally speaking the results look good. Following unrelated failures were found:

  • one job failed because [WRN] message from mon.0 was stamped 0.600861s in the future, clocks not synchronized,
  • one job thrown in s3-tests because of the well-known 100 Continue issue on Apache and FastCGI front-end,
  • in one job RadosGW instance finished due to a multi-site issue: 2017-03-25 22:00:58.828106 309d4700 0 ERROR: failed to clone shard, completion_mgr.get_next() returned ret=-125. Errno 125 is ECANCELED,
  • one job is red due because No summary info found for user: foo. I think I saw this message couple of times on very different branches,
  • two jobs failed as Valgrind detected the well-known problems with md_config_t.
@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Mar 26, 2017

@mattbenjamin: the run on smithi finished very recently and it's even greener. Only following non-related issues:

  • two jobs failed on Apache with FastCGI in testChunkedPut (test.functional.tests.TestFileUTF8) of the Swift's tests. The same affects master so we don't have a regression here,
  • one job failed as Valgrind detected the well-known problem with md_config_t.

@mattbenjamin mattbenjamin merged commit b894dc6 into ceph:master Mar 26, 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
@mattbenjamin

This comment has been minimized.

Copy link
Contributor

mattbenjamin commented Mar 26, 2017

@rzarzynski ack, well done

@cbodley

This comment has been minimized.

Copy link
Contributor

cbodley commented Mar 27, 2017

great work on this @rzarzynski! thanks for being patient with us in our design critique, i think it turned out really well

@rzarzynski

This comment has been minimized.

Copy link
Contributor Author

rzarzynski commented Mar 27, 2017

@mattbenjamin, @cbodley: Guys, it's really nice hear but let's wait a bit. We still have AWSv4 to address. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.