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: Added code for REST APIs for AWS Roles. #12104

Merged
merged 11 commits into from Feb 1, 2017

Conversation

Projects
None yet
5 participants
@pritha-srivastava
Contributor

pritha-srivastava commented Nov 21, 2016

No description provided.

@pritha-srivastava pritha-srivastava changed the title from [DNM] rgw: Added code REST APIs for AWS Roles. to [DNM] rgw: Added code for REST APIs for AWS Roles. Nov 21, 2016

if (!verify_user_permission(s, RGW_PERM_WRITE)) {
return -EACCES;
}

This comment has been minimized.

@cbodley

cbodley Nov 21, 2016

Contributor

most of verify_permission() is duplicated by all of the Role ops. could we factor this into a base class so each op only has to say whether it's a read or write? or maybe have a RGWRoleRead and RGWRoleWrite that the others inherit from?

RGWDeleteRolePolicy_ObjStore() = default;
~RGWDeleteRolePolicy_ObjStore() = default;
};

This comment has been minimized.

@cbodley

cbodley Nov 22, 2016

Contributor

since these ops are all specific to s3, maybe we don't need the usual Op, Op_ObjStore, ObjStore_S3 hierarchy. can we just put them all in a separate rgw_rest_role.cc?

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Nov 23, 2016

Contributor

@cbodley : I have refactored the code and added them as separate commits. Once they are approved by you, i'll rebase the commits to the previous ones.

@mattbenjamin mattbenjamin changed the title from [DNM] rgw: Added code for REST APIs for AWS Roles. to rgw: Added code for REST APIs for AWS Roles. Nov 22, 2016

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 22, 2016

@pritha-srivastava can you rebase when you get a chance? Tx!

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 22, 2016

@pritha-srivastava context: @aemerson suggests merging these first, then adapting to following policy change; Do you anticipate and problems with going in that order?

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Nov 23, 2016

@mattbenjamin : I dont see any problem merging this patch first and then adding policy stuff. Just wanted to point out that the version number will have to be bumped while encode/ decode operations, when we switch to the 'actual' policy.
Casey has a couple of comments on the REST API part. I'll address them and then rebase the PR, and then it will be good to merge.

dump_errno(s);
end_header(s);
}

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Nov 23, 2016

Contributor

@cbodley: Most of send_response() is also the same for all ops (as you have mentioned in your comment above), i'll try to refactor this as well.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Nov 23, 2016

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Nov 23, 2016

@adamemerson : People in the team have suggested to bump the number always when we make changes to a structure, even though it may be before a release, reason being that someone may start using master branch and then the changed structure will cause decode to fail. Just to be on the safe side, so that we don't break someone's existing systems, bumping version number has been recommended. @cbodley @mattbenjamin can comment better on this.

RGWRestRole() = default;
virtual void init(RGWRados *store, struct req_state *s, RGWHandler *h) {
RGWOp::init(store, s, h);
}

This comment has been minimized.

@cbodley

cbodley Nov 28, 2016

Contributor

you should be able to leave out the default ctor and init() function - they'll still be accessible from the base class

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Nov 28, 2016

Contributor

@cbodley: sorry i didnt understand this comment.

This comment has been minimized.

@cbodley

cbodley Nov 28, 2016

Contributor

just saying that these overrides don't do anything other than forward the call to RGWOp. if you remove them from RGWRestRole, you'll get the same behavior

public:
RGWCreateRole() = default;
void execute() override;
int get_params();

This comment has been minimized.

@cbodley

cbodley Nov 28, 2016

Contributor

get_params() is an override also

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Nov 30, 2016

Contributor

@cbodley : get_params() is declared and defined only in CreateRole, DeleteRole etc classes. It is not there in any of the other classes up till RGWOp.

cout << " --assume-role-policy-doc the trust relationship policy document that grants an entity permission to assume the role\n";
cout << " --policy-name name of the policy document\n";
cout << " --policy-doc permission policy document\n";
cout << " --path-prefix path prefix for filtering roles\n";

This comment has been minimized.

@cbodley

cbodley Nov 28, 2016

Contributor

it might be worth putting these options in their own Role options: section, so they're not confused with other commands

@@ -21,6 +21,8 @@ int rgw_put_system_obj(RGWRados *rgwstore, rgw_bucket& bucket, const string& oid
int rgw_get_system_obj(RGWRados *rgwstore, RGWObjectCtx& obj_ctx, rgw_bucket& bucket, const string& key, bufferlist& bl,
RGWObjVersionTracker *objv_tracker, real_time *pmtime, map<string, bufferlist> *pattrs = NULL,
rgw_cache_entry_info *cache_info = NULL);
int rgw_delete_system_obj(RGWRados *rgwstore, rgw_bucket& bucket, string& oid,
RGWObjVersionTracker *objv_tracker);

This comment has been minimized.

@cbodley

cbodley Nov 28, 2016

Contributor

can oid be const string& here?

@mattbenjamin

This comment has been minimized.

Contributor

mattbenjamin commented Nov 28, 2016

@pritha-srivastava @cbodley these suggestions seem worthwhile, other than that, I think we just need to prove nothing has broken to merge this shortly?

@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 28, 2016

@mattbenjamin agreed, these certainly aren't blockers

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Nov 29, 2016

On second thought, so we don't have to old an old encode version forever, just ping me when you're ready and I'll grab this branch, stick my stuff on it, and hook in the profile work?

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Nov 29, 2016

@adamemerson : I'm struggling with some build issues at the moment. I have to incorporate some of Casey's comments. I'll do that and let you know.

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Nov 30, 2016

@cbodley : I have incorporated your comments

@cbodley

This comment has been minimized.

Contributor

cbodley commented Nov 30, 2016

@adamemerson i'd like to see this merge asap so we don't have to keep rebasing

regarding the pains of backward-compatible decoding, we should be able to avoid that by removing the policy-as-string stuff from @pritha-srivastava's v1 format of RGWRole encode/decode

then when your work is ready, you can just add it on top in the v2 format

thoughts?

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Nov 30, 2016

If we can just pull the string out entirely and save having to own an unused encode/decode version forever I approve entirely.

Laotian Generals Train Mercenaries.

@aemerson

This comment has been minimized.

aemerson commented Nov 30, 2016

FYI I'm unsubscribing as I was mentioned in this thread accidentally, I have nothing to do with this project.

::encode(path, bl);
::encode(creation_date, bl);
::encode(trust_policy, bl);
::encode(perm_policy_map, bl);

This comment has been minimized.

@cbodley

cbodley Nov 30, 2016

Contributor

@pritha-srivastava i assume that trust_policy and perm_policy_map are the strings that @adamemerson will replace later - do you see any issue with removing them from v1 encode/decode, so he can add them to v2 later without worrying about backward compatibility?

This comment has been minimized.

@adamemerson

adamemerson Nov 30, 2016

Contributor

I'd like to not have a v1/v2 at all from this. If the choice is between it going in now and having to take another version before it's ever practically used and me rebasing it a few times as part of my work to keep it up to master, I'd prefer the second.

This comment has been minimized.

@cbodley

cbodley Nov 30, 2016

Contributor

there is really no cost to bumping the version number, as long as you're only adding to an existing format.. when your work is ready, the only diff would be:

   void encode(bufferlist& bl) const {
-    ENCODE_START(1, 1, bl);
+    ENCODE_START(2, 1, bl);
     ::encode(id, bl);
     ::encode(name, bl);
     ::encode(path, bl);
     ::encode(creation_date, bl);
+    ::encode(trust_policy, bl);
+    ::encode(perm_policy_map, bl);
     ENCODE_FINISH(bl);
   }
 
   void decode(bufferlist::iterator& bl) {
-    DECODE_START(1, bl);
+    DECODE_START(2, bl);
     ::decode(id, bl);
     ::decode(name, bl);
     ::decode(path, bl);
     ::decode(creation_date, bl);
+    if (struct_v >= 2) {
+      ::decode(trust_policy, bl);
+      ::decode(perm_policy_map, bl);
+    }
     DECODE_FINISH(bl);
   }

This comment has been minimized.

@adamemerson

adamemerson Nov 30, 2016

Contributor

All right. It seems untidy and pointless for something that hasn't been released but not that bad.

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Dec 1, 2016

@cbodley @adamemerson : There are places where the input permission document is being read and is being passed as a string to the constructor. These places will also need changes.
Rebased the PR.

id = uuid_str;
//arn
arn = "arn:aws:iam::role" + path + name;

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 1, 2016

Contributor

@adamemerson : A Role also has an ARN which I have coded up to be something like the above. Does this look fine? Or do I need to change this?

This comment has been minimized.

@adamemerson

adamemerson Dec 1, 2016

Contributor

I think it's mostly fine. One thing Marcus and I had talked about off and on was whether we wanted to use the same namespaces/identifiers as Amazon. (e.g. whether we wanted to use arn:aws: as our namespace prefix.)

I had been leaning toward keeping all our identifiers the same since it improves compatibility and makes copying policies from one place to the other much easier.

The main argument against is that we aren't actually Amazon. I know Marcus at one point suggested making things like that configurable. I'm not sure if there's much benefit to that. (If we have a single IDP backing both AWS STS and Ceph RGW's STS having them both use different ARNs seems a huge negative.)

Anyway, this is a ramble. I think what you have is perfectly fine, but we might want to discuss it tomorrow on the RGW standup just to air the idea out and get feedback in case there's something I'm not thinking of.

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Dec 2, 2016

Contributor

@adamemerson : I took a look at the IAM Policy Elements link that you shared with me. It looks like ARN plays an important role while deducing permissions for an entity and it will be worthwhile to discuss the format of the ARN with the team for the reasons mentioned above, and come to a conclusion.
Are you planning to attend the standup today(Friday) to discuss this. Because I usually dont attend stand-ups on Fridays (its too late for me here), but if you plan to discuss this today, I will attend. Please let me know. Thanks.

This comment has been minimized.

@adamemerson

adamemerson Dec 2, 2016

Contributor

Sorry, I caught your answer too late to answer. I'll be at the RGW Core Standup on Monday and we can talk about it then. I think I might just post to ceph-devel and ASK if anyone would find the ability to change it useful.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Jan 24, 2017

@pritha-srivastava Can you please rebase and repush this?

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Jan 25, 2017

@adamemerson : I have rebased the PR, but I see build failures which aren't related to the code in the PR.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Jan 25, 2017

Give it another shot when it's convenient? I get the impressin that sometimes gitbuilder is just out to lunch and needs to be prodded until it's working.

Also, did you run an RGW test suite against this at some point?

pritha-srivastava added some commits Oct 5, 2016

rgw: Adding code for Role creation, deletion and reading.
A new class RGWRole for AWS roles has been added.
Also, code for role create. delete and get for radosgw-admin has been added.

Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: Added code for manipulation of inline permission policies attach…
…ed to roles.

Implemented radosgw-admin commands for creating/ updating/ deleting/ reading/
and listing of permission policies attached to roles.

Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: Added code for role modify.
Adding code for role modify, which can be used to update
the assume role policy document of a role.

Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Added code for listing roles for a path prefix.
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
Replaced gmtime with gmtime_r.
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: Added code for CreateRole, GetRole, UpdateAssumeRolePolicy, List…
…Roles and DeleteRole REST APIs.

Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: Added code for PutRolePolicy, GetRolePolicy, ListRolePolicies an…
…d DeleteRolePolicy REST APIs.

Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: Refactored code for Create/Get/Modify/Delete/List Role(s) REST A…
…PIs.

Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: Refactored code for Put/Get/List/Delete RolePolicy REST API.
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: Added a separate section for Roles options in radosgw-admin help.
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
rgw: ARN generation based on uid for Roles.
Signed-off-by: Pritha Srivastava <prsrivas@redhat.com>
@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Jan 27, 2017

@adamemerson : I rebased again, and this time the build finished successfully. No, I haven't run teuthology against this PR, do you want me to that? I have scheduled a run.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Jan 27, 2017

Yes please. Run teuthology against this and if it doesn't break anything, I'll merge it in.

@pritha-srivastava

This comment has been minimized.

Contributor

pritha-srivastava commented Jan 30, 2017

@adamemerson : The teuthology results are here: http://qa-proxy.ceph.com/teuthology/prsrivas-2017-01-27_12:47:17-rgw-wip_sts_role_rest---basic-smithi/755464/teuthology.log

There is one valgrind failure which is in one of the osds. I discussed this with @cbodley , and he told this can be ignored. All other tests have passed.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Jan 31, 2017

Thank you. Sorry to ask for another change this late, but can you add a string for another policy? We aren't going to use it YET, but eventually we're going to want both the access-policy and the assume-role policy and having another string thrown in there that gets encoded/decoded will keep us from having to version things later.

string arn;
string creation_date;
string trust_policy;
map<string, string> perm_policy_map;

This comment has been minimized.

@pritha-srivastava

pritha-srivastava Feb 1, 2017

Contributor

@adamemerson : I have already added a string to hold the assume-role-policy (trust_policy here) and a map of permission policies (access policies). A role can have more than one access policies attached and also the operations for access policies are such that we need an association between the policy name and the actual policy.

Is there anything else that you want me to add.

@adamemerson

This comment has been minimized.

Contributor

adamemerson commented Feb 1, 2017

Sorry about that, I didn't realize we could have multiple access policies. Everything looks good to me, then.

@adamemerson adamemerson merged commit 1a61a65 into ceph:master Feb 1, 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

adamemerson added a commit that referenced this pull request Feb 1, 2017

Merge pull request #12104 from pritha-srivastava/wip_sts_role_rest
rgw: Added code for REST APIs for AWS Roles.

Reviewed-by: Adam C. Emerson <aemerson@redhat.com>
Reviewed-by: Casey Bodley <cbodley@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment