rgw: Rework of s3 v2 local authentication code. #10448

pritha-srivastava · 2016-07-27T05:37:10Z

The v2 local auth code has been reworked based on the new new authentication infrastructure.

Signed-off-by: Pritha Srivastava prsrivas@redhat.com

pritha-srivastava · 2016-07-27T05:49:20Z

This PR contains reworked code for s3 v2 local auth.

New classes: RGWS3V2TokenBasedAuthEngine, RGWS3V2TokenExtractor, and RGWS3V2LocalAuthEngine have been introduced.
RGWS3V2TokenBasedAuthEngine has the additional functionality of extracting the other tokens required for local auth.
The RGWS3V2TokenExtractor class can be re-used for LDAP auth code as well.
Both RGWS3V2TokenBasedAuthEngine and RGWS3V2Extractor can be used for V2 Keystone auth as well.
Another alternative would be to make RGWS3V2TokenBasedAuthEngine, derive from RGWAuthEngine (and not from RGWTokenBasedAuthEngine) and it takes care of getting all the tokens including auth_id. The auth engine class can serve as base class for both for local and keystone auth. The extractor will not be needed in this case. And the ldap code in the other PR can remain unchanged. This approach is simple. Please let me know what you think.

theanalyst · 2016-07-27T07:52:55Z

src/rgw/rgw_rest_s3.cc

+    return nullptr;
+  }
+
+  dout(15) << "calculated digest=" << digest << dendl;


could we probably replace dout -> ldout if we have s->cct here?

harmless for now?

Yeah harmless, though we do we have req_info, ie. s->cct here :)

mattbenjamin · 2016-08-03T16:49:43Z

This commit now has conflicts with master, and needs a rebase. Pritha, can you rebase and re-post?

mattbenjamin · 2016-08-03T16:51:52Z

Also, just quickly: does this change really only apply to v2? Will another change be required for v4? If so, can we combine those (I mean, in one PR, not speaking of impl per se)?

pritha-srivastava · 2016-08-04T07:41:27Z

@mattbenjamin : Yes, this change only applies to v2. But I will update this PR with changes for v4 also as you have suggested.

pritha-srivastava · 2016-08-04T07:47:24Z

@mattbenjamin, @rzarzynski : I need some feedback on the design of classes RGWS3V2TokenBasedAuthEngine and RGWS3V2TokenExtractor. Instead of having an extractor class for just the auth id and the other tokens getting extracted in RGWS3V2TokenBasedAuthEngine, I can just have RGWS3V2TokenBasedAuthEngine derive from RGWAuthEngine and not have any extractor class. The RGWS3V2TokenBasedAuthEngine class will take care of extracting all auth tokens (like auth id, signature and expires). This class could be re-used for Keystone Auth as well (and maybe for ldap also.)

pritha-srivastava · 2016-08-04T07:50:09Z

@mattbenjamin : In the old code for ldap auth, the code would fallback to local auth incase ldap auth failed. Is that really intended here? Or should we just send an error to the user in case ldap auth fails (and not proceed to ocal auth)?

rzarzynski · 2016-08-04T11:40:48Z

src/rgw/rgw_rest_s3.h

@@ -689,7 +689,67 @@ class RGWLDAPTokenExtractor : public RGWTokenBasedAuthEngine::Extractor {
  std::string get_token() const override;
 };

-class S3AuthFactory : public RGWRemoteAuthApplier::Factory {
+class RGWS3V2TokenBasedAuthEngine : public RGWTokenBasedAuthEngine {


@pritha-srivastava: I'm not sure whether deriving from RGWTokenBasedAuthEngine is suitable way for AWS auth. The class is targeting those authenticators which are token-based. OpenStack Identity API and Kerberos might be seen as typical examples. It's worth to add that under token I mean a short-living string that can be used multiple times without alternations. A comment on StackOverflow might be helpful as well.

AWSv2 isn't token-based in this sense. Instead of providing a constant string, client needs to send AWSAccessKeyId and Signature according the Amazon's terminology. Signature is varying with each request.

The second solution proposed by you seems to be the good one. However, I wouldn't name the class with anything having Token inside. Also the extractor concept still could be useful. IIRC the AWS auth stuff could be extracted either from HTTP headers or from query string.

pritha-srivastava · 2016-08-04T12:51:36Z

@rzarzynski : Thank you very much for your comments.

pritha-srivastava · 2016-08-05T11:30:45Z

src/rgw/rgw_rest_s3.cc

      ldout(cct, 10) << "ERROR: User id of type: " << user_info.type << " is already present" << dendl;
      return nullptr;
-    }
+    }*/
  }


I will enable the type checking once we have a plan to migrate existing users to the new format.

pritha-srivastava · 2016-08-05T11:39:12Z

Note:

I have introduced a RGWS3V2AuthEngine and RGWS3V2Extractor class which are being used by ldap and local authentication now.
I can define a standalone Extractor class and use aggregation with AuthEngine classes so that the tokens are not parsed again for every new AuthEngine class that is defined - I will explore this in keystone rework.
I have commented out user source type checking for the time being, will enable them once the plan to migrate existing users to the new format is in place.

pritha-srivastava · 2016-08-05T11:44:13Z

@mattbenjamin @rzarzynski : Please review

rzarzynski · 2016-08-05T12:01:46Z

src/rgw/rgw_rest_s3.cc

+
+        public:
+          S3Extractor(std::string access_key_id, std::string signature)
+          : access_key_id(access_key_id),


I would suggest std::moving here to save additional memory allocation/increasing a reference counter in std::string.

pritha-srivastava · 2016-08-08T08:46:56Z

@rzarzynski : Reworked.

rzarzynski · 2016-08-09T11:48:48Z

src/rgw/rgw_rest_s3.cc

@@ -1754,13 +1754,22 @@ int RGWPostObj_ObjStore_S3::get_policy()
 	  s->perm_mask = RGW_PERM_FULL_CONTROL;
 	}
  } else if (ldap.is_applicable()) {
-    auto applier = ldap.authenticate();
-    if (! applier) {
+    try {


mattbenjamin · 2016-08-10T17:14:21Z

@pritha-srivastava I don't think we should just comment out the type checking, since we I believe all agree it should be present (Shilpa raised the issue as a potential bug).

Rather, we should figure out a change following this one which allows dealing with this issue. My first thought on it was:

ensure auth-type field can be listed and updated w/user metadata
provide radosgw-admin cmd to set auth type
-maybe- a policy for handling unassigned auth-type by default (e.g., block, assume local, assume ldap, etc)
@yehudasa thoughts on this?

pritha-srivastava · 2016-08-11T03:35:59Z

@mattbenjamin : My intention was to keep the type checking commented, till we have the radosgw-admin command in place (or whatever will help in adding the user source type field to the existing users). This is to avoid any confusion in case someone wants to use the master branch may see incorrect auth failures. Once the auth rework is done, my plan was to work on the new command and then enable the type checking throughout the code. Let me know what you think.

mattbenjamin · 2016-08-11T14:24:23Z

I think the type checking should be re-enabled. Yehuda has already agreed that the migration rules can come later, so long as we prioritize them. If you are not comfortable with that, I'd rather delay merging this change until we have a migration path.

pritha-srivastava · 2016-08-11T14:29:14Z

@mattbenjamin : My only concern was the migration of the existing users. If that can be deffered, then it is fine with me. The code to add the source user type and to check the type is already there. I'll add/ re-enable them and update the PR.

pritha-srivastava · 2016-08-11T14:39:23Z

@mattbenjamin : I have re-enabled the type checking both for ldap and local auth.

mattbenjamin · 2016-08-11T14:46:30Z

@pritha-srivastava I agree it's kind of annoying either way. I'm honestly responding partly from intuition here. Changing the behavior between pushes and without discussion felt wrong to me. I also don't like adding disabled sections terribly much, but then again, I sometimes do it myself. Here I think we should focus on the real issue that was exposed by this rework, which is the lack of a proper policy governing multiple auth backends. I share Shilpa's intuition that it's not good to allow dynamic backends like ldap to induce privilege escalation, so I've been trying to force us to address it.

mattbenjamin · 2016-08-11T14:49:54Z

@pritha-srivastava I forgot to add. Do you have a sense of what strategies we should take for the migration? I wrote down the ideas I had come up with previously. Do you have other ideas? Let's discuss this in standup.

pritha-srivastava · 2016-08-11T14:55:07Z

@mattbenjamin : The note about disabling the user type check was intended to bring about the discussion, but I will bring it up in the stand-up next time. Sorry about that.

Yes, I did think a little about migrating existing users: We could probably use radosgw-admin modify user command, and give the users an option to add the source type. We could write up a script using this command addressing all users. I am not sure though how many users the end-user will typically have.

mattbenjamin · 2016-08-11T19:12:11Z

@pritha-srivastava I'll merge as soon as the next push re-disabling checks lands.

pritha-srivastava · 2016-08-11T19:37:20Z

@mattbenjamin : Checking teuthology errors, will update the PR once done

pritha-srivastava · 2016-08-11T20:08:18Z

@mattbenjamin : I have updated the PR. But there is still one job in which one of the s3tests timeout randomly because of an IO error. I have run the job twice and each time a new s3 test timed out. I am running it on a different machine now. @cbodley has agreed to check the result and let you know. Please dont merge the PR till then. Thanks!

cbodley · 2016-08-11T21:04:31Z

@pritha-srivastava it hadn't run by the time i left today, but you guys can track progress at http://pulpito.ceph.com/prsrivas-2016-08-11_19:53:53-rgw-wip_s3_v2_local_auth_rework---basic-mira/

The v2 local auth code has been reworked based on the new new authentication infrastructure. Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>

pritha-srivastava assigned pritha-srivastava and mattbenjamin Jul 27, 2016

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from eaa68ad to cf81f4d Compare July 27, 2016 06:10

theanalyst reviewed Jul 27, 2016
View reviewed changes

cbodley added the rgw label Jul 27, 2016

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from cf81f4d to 4b6c263 Compare August 4, 2016 07:38

rzarzynski reviewed Aug 4, 2016
View reviewed changes

rzarzynski mentioned this pull request Aug 4, 2016

[DNM] rgw: Rework of s3 Keystone Authentication code. #10536

Closed

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from 4b6c263 to 52fd837 Compare August 5, 2016 11:26

pritha-srivastava reviewed Aug 5, 2016
View reviewed changes

rzarzynski reviewed Aug 5, 2016
View reviewed changes

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from 52fd837 to d2486e9 Compare August 8, 2016 08:39

rzarzynski reviewed Aug 9, 2016
View reviewed changes

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from d2486e9 to 295657b Compare August 9, 2016 14:50

mattbenjamin changed the title ~~[DNM]rgw: Rework of s3 v2 local authentication code.~~ rgw: Rework of s3 v2 local authentication code. Aug 10, 2016

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from 295657b to fd39aab Compare August 11, 2016 14:33

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from fd39aab to bb94f26 Compare August 11, 2016 20:03

rgw: Rework of s3 v2 local authentication code.

b5b3e5c

The v2 local auth code has been reworked based on the new new authentication infrastructure. Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>

pritha-srivastava force-pushed the wip_s3_v2_localauth_rework branch from bb94f26 to b5b3e5c Compare August 12, 2016 12:26

mattbenjamin merged commit 41aa39f into ceph:master Aug 12, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rgw: Rework of s3 v2 local authentication code. #10448

rgw: Rework of s3 v2 local authentication code. #10448

pritha-srivastava commented Jul 27, 2016

pritha-srivastava commented Jul 27, 2016 •

edited

theanalyst Jul 27, 2016

mattbenjamin Aug 3, 2016

theanalyst Aug 3, 2016

mattbenjamin commented Aug 3, 2016

mattbenjamin commented Aug 3, 2016 •

edited

pritha-srivastava commented Aug 4, 2016

pritha-srivastava commented Aug 4, 2016

pritha-srivastava commented Aug 4, 2016

rzarzynski Aug 4, 2016

pritha-srivastava commented Aug 4, 2016

pritha-srivastava Aug 5, 2016

pritha-srivastava commented Aug 5, 2016 •

edited

pritha-srivastava commented Aug 5, 2016

rzarzynski Aug 5, 2016

pritha-srivastava commented Aug 8, 2016

rzarzynski Aug 9, 2016

mattbenjamin commented Aug 10, 2016 •

edited

pritha-srivastava commented Aug 11, 2016 •

edited

mattbenjamin commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

mattbenjamin commented Aug 11, 2016

mattbenjamin commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016 •

edited

mattbenjamin commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

cbodley commented Aug 11, 2016

rgw: Rework of s3 v2 local authentication code. #10448

rgw: Rework of s3 v2 local authentication code. #10448

Conversation

pritha-srivastava commented Jul 27, 2016

pritha-srivastava commented Jul 27, 2016 • edited

theanalyst Jul 27, 2016

Choose a reason for hiding this comment

mattbenjamin Aug 3, 2016

Choose a reason for hiding this comment

theanalyst Aug 3, 2016

Choose a reason for hiding this comment

mattbenjamin commented Aug 3, 2016

mattbenjamin commented Aug 3, 2016 • edited

pritha-srivastava commented Aug 4, 2016

pritha-srivastava commented Aug 4, 2016

pritha-srivastava commented Aug 4, 2016

rzarzynski Aug 4, 2016

Choose a reason for hiding this comment

pritha-srivastava commented Aug 4, 2016

pritha-srivastava Aug 5, 2016

Choose a reason for hiding this comment

pritha-srivastava commented Aug 5, 2016 • edited

pritha-srivastava commented Aug 5, 2016

rzarzynski Aug 5, 2016

Choose a reason for hiding this comment

pritha-srivastava commented Aug 8, 2016

rzarzynski Aug 9, 2016

Choose a reason for hiding this comment

mattbenjamin commented Aug 10, 2016 • edited

pritha-srivastava commented Aug 11, 2016 • edited

mattbenjamin commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

mattbenjamin commented Aug 11, 2016

mattbenjamin commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016 • edited

mattbenjamin commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

pritha-srivastava commented Aug 11, 2016

cbodley commented Aug 11, 2016

pritha-srivastava commented Jul 27, 2016 •

edited

mattbenjamin commented Aug 3, 2016 •

edited

pritha-srivastava commented Aug 5, 2016 •

edited

mattbenjamin commented Aug 10, 2016 •

edited

pritha-srivastava commented Aug 11, 2016 •

edited

pritha-srivastava commented Aug 11, 2016 •

edited