add support for AssumeRole provider

[hardbyte]

This PR adds a assume_role api to mino-py allowing users to generate temporary credentials using the AssumeRole STS API. It has to modify how signatures are generated to allow signing requests for other services e.g., "sts".

I'm assuming the functional python API should be replaced - but let me know.

Further discussion original feature request in #871.

Server side API documentation:

• https://github.com/minio/minio/blob/master/docs/sts/assume-role.md
• https://docs.aws.amazon.com/STS/latest/APIReference/API_AssumeRole.html

[hardbyte]

Is this too invasive a change to review/consider?

I could submit a PR that just modifies sign_v4 adding the optional service name. But what I'm really after is how you want the user facing API to work for using assume role.

[Praveenrajmani]

Thanks for the PR @hardbyte.
Let me drop few implementation details for your reference.

We can do this in a similar manner as the other Providers, We could create an interface compatible class to Provider.

class Provider(object):

    __metaclass__ = ABCMeta

    @abstractmethod
    def retrieve(self):
        pass

    @abstractmethod
    def is_expired(self):
        pass

It should honor these functions to be a valid Provider. Check credentials.py file for these details

[Praveenrajmani]

Please check aws_iam.py for similar implementation @hardbyte

[hardbyte]

Thanks for the review. It sounds like sub-classing Provider is worth keeping - with some modifications. But I'm getting the feeling you don't like my approach of returning a new Provider subclass from assume_role and you would have the users directly initialize the new Provider subclass? If so I'll go ahead and implement that.

My guess is the interface will be clunkier than my currently proposed functional one though. Thinking through a couple of made up user stories:

1 As a backend developer, I want to be able to generate temporary, restricted object store credentials to allow my front end clients to upload directly to the object store using an s3 compatible client library. In particular I want to easily serialize the new credentials.

2 As a client library developer, I want to easily be able to instantiate a minio-py client using credentials supplied to me in serialized form.

3 As someone else, at times I want to create minio clients with restricted & temporary credentials.

2 is already fully supported using the minio constructor so can be safely ignored.

In 1 and 3 I think it would feel most natural for the api to be minio_client.create_temporary_credentials(policy, ...), or my current functional api:

from minio import assume_role
credentials = assume_role(minio_client, policy, ...)

Where the credentials are an instance of Credentials with the new AssumeRoleProvider.

For 1 the user serializes via getting the credential's Value (e.g. credentials.get().secret_key etc).

For 3 the user then creates a new minio client with the newly returned credentials:

restricted_client = Minio(endpoint, credentials=credentials, ...)

If instead the minio-py API is simply an AssumeRoleProvider the user would have to:

from minio.credentials import AssumeRoleProvider

provider = AssumeRoleProvider(minio_client, policy, ...)
credentials  = provider.retrieve()

Everything else being the same from there. Still I feel this Provider api is more clunky to expose to the end user. To be sure, I'm happy to implement either way, but thought it'd be worth discussing.

Other options I was thinking about include minio_client.assume_role returning a new minio client that "assumed" the new role. But I don't like that as I believe many users would mistakenly believe that the minio_client had assumed the role.

[Praveenrajmani]

@hardbyte The benefit of using the Provider class is that we could chain the providers sequentially so that if one service is down/denies it will automatically query the next service

We should follow the same across all the providers. As this is pretty much the agreed standard for all the targets.

If you need to use the other way, you could use it in application side for your convenience.

[Praveenrajmani]

This example should be changed. It should use AssumeRoleProvider

[Praveenrajmani]

We could remove this as it is not recommended to pass clients to functions, Let us follow the Provider class everywhere

[Praveenrajmani]

Can you fix the conflicts @hardbyte

[hardbyte]

I'll rebase and resolve the conflicts now and adjust the example to use the AssumeRoleProvider directly.

I'd appreciate your thoughts about how this could be used by clients though. I realize you're thinking about it like another credentials provider for the minio client (boto does this too and I'm all for supporting this use case), however for most end users it will instead be used to get credentials to give to someone else (another service, a front end client etc). Please see my (made up) user stories earlier in the thread. Boto supports this lower level api too by exposing assume_role

By the way running the unit tests on master with python3.7 has just got substantially slower for me - after pulling the time went from <1 second to 40+ seconds. I'm not sure if a recent change in master has caused this regression:

[harshavardhana]

I'd appreciate your thoughts about how this could be used by clients though. I realize you're thinking about it like another credentials provider for the minio client (boto does this too and I'm all for supporting this use case), however for most end users it will instead be used to get credentials to give to someone else (another service, a front end client etc). Please see my (made up) user stories earlier in the thread. Boto supports this lower level api too by exposing assume_role

We should be able to expose them if they need direct access @hardbyte but it should also be a credential provider such that it can be chained together with other providers, in the current form.

By the way running the unit tests on master with python3.7 has just got substantially slower for me - after pulling the time went from <1 second to 40+ seconds. I'm not sure if a recent change in master has caused this regression:

Do not know, we should check all the pep8 fixes we did in master @hardbyte @balamurugana @kannappanr

[hardbyte]

Just debugging the credential "refreshing" and I notice the existing code uses local time stamps instead of utc which seems like a bad idea. I'll push an implemention the same as the credentials/aws_iam.py response parser for now but in both cases the api response is an iso8601 string which is in UTC.

datetime.now() (used in Expiry.is_expired) is definitely in the users local time. Python docs now recommend using datetime.now(timezone.utc)

[harshavardhana]

Just debugging the credential "refreshing" and I notice the existing code uses local time stamps instead of utc which seems like a bad idea. I'll push an implemention the same as the credentials/aws_iam.py response parser for now but in both cases the api response is an iso8601 string which is in UTC.

datetime.now() (used in Expiry.is_expired) is definitely in the users local time. Python docs now recommend using datetime.now(timezone.utc)

Yes it should use utc() @hardbyte we shouldn't rely on anything i.e local TZ

[hardbyte]

Ok this has now been tested against AWS sts as well as Minio - @harshavardhana the changes required to have a valid signature are here. I think that means the signing PR is good to merge - sorry for all the rebasing and force pushing I wanted to avoid to many conflicts with the pep8 work that is landing in master.

For this PR I still plan to refactor the expiry stuff to use UTC aware datetime objects. It won't be today though I'm afraid.

[Qantas]

FYI. I pulled the PR and ran the examples/assume_role.py on a local minio instance with a self-sign cert. Got the following error "SignatureDoesNotMatchThe request signature we calculated doesn't match the signature you provided. Check your key and signing method. "

[hardbyte]

FYI. I pulled the PR and ran the examples/assume_role.py on a local minio instance with a self-sign cert. Got the following error "SignatureDoesNotMatchThe request signature we calculated doesn't match the signature you provided. Check your key and signing method. "

Thanks for trying it out @Qantas - I just tried using the self signed certificate in tests/certs and it seemed to work fine for me. Did the error occur on the AssumeRoleProvider call or the first get()?

There is definitely an issue with the credentials expiry though - I think it will have different behavior in different time zones 🤢

[hardbyte]

@harshavardhana @donatello @Praveenrajmani I think this is now ready for another review.

I've addressed all the issues you've pointed out earlier; tested it against both AWS and Minio; fixed the timezones used by Expiry to always be timezone aware (and in UTC); and added quite a bit of notes in the example.

[harshavardhana]

@harshavardhana @donatello @Praveenrajmani I think this is now ready for another review.

I've addressed all the issues you've pointed out earlier; tested it against both AWS and Minio; fixed the timezones used by Expiry to always be timezone aware (and in UTC); and added quite a bit of notes in the example.

Very nice @hardbyte 🍰

[harshavardhana]

Of course please resolve the conflicts @hardbyte

[harshavardhana]

LGTM tested 🍰

[donatello]

LGTM

[Qantas]

FYI. I pulled the PR and ran the examples/assume_role.py on a local minio instance with a self-sign cert. Got the following error "SignatureDoesNotMatchThe request signature we calculated doesn't match the signature you provided. Check your key and signing method. "

Thanks for trying it out @Qantas - I just tried using the self signed certificate in tests/certs and it seemed to work fine for me. Did the error occur on the AssumeRoleProvider call or the first get()?

There is definitely an issue with the credentials expiry though - I think it will have different behavior in different time zones 🤢

I pulled the latest merged version and gave it a try. got a different error "Generating temporary credentials not allowed for this request" I am using a minio_client with a http_client to bring the cert in since it is not in your assume_role.py example code.

[harshavardhana]

FYI. I pulled the PR and ran the examples/assume_role.py on a local minio instance with a self-sign cert. Got the following error "SignatureDoesNotMatchThe request signature we calculated doesn't match the signature you provided. Check your key and signing method. "

Thanks for trying it out @Qantas - I just tried using the self signed certificate in tests/certs and it seemed to work fine for me. Did the error occur on the AssumeRoleProvider call or the first get()?
There is definitely an issue with the credentials expiry though - I think it will have different behavior in different time zones nauseated_face

I pulled the latest merged version and gave it a try. got a different error "Generating temporary credentials not allowed for this request" I am using a minio_client with a http_client to bring the cert in since it is not in your assume_role.py example code.

You cannot generate temporary credentials for root account @Qantas

[Qantas]

FYI. I pulled the PR and ran the examples/assume_role.py on a local minio instance with a self-sign cert. Got the following error "SignatureDoesNotMatchThe request signature we calculated doesn't match the signature you provided. Check your key and signing method. "

Thanks for trying it out @Qantas - I just tried using the self signed certificate in tests/certs and it seemed to work fine for me. Did the error occur on the AssumeRoleProvider call or the first get()?
There is definitely an issue with the credentials expiry though - I think it will have different behavior in different time zones nauseated_face

I pulled the latest merged version and gave it a try. got a different error "Generating temporary credentials not allowed for this request" I am using a minio_client with a http_client to bring the cert in since it is not in your assume_role.py example code.

You cannot generate temporary credentials for root account @Qantas
Thanks. I figured it out and created a new-user to get temp credentials.