api: Add NewWithCredentials() by harshavardhana · Pull Request #646 · minio/minio-go

harshavardhana · 2017-04-13T08:45:37Z

This PR adds a new API

NewWithCredentials()

Internally NewWithCredentials is now used with
all APIs such as New(), NewV4(), NewV2() and NewWithRegion.

Also brings a new package called credentials to manage
various credentials type, currently the credentials
package supports

Reading file from .aws/credentials, .mc/config.json
Reading env variables for AWS*, MINIO*
Fetching from IAM roles assigned to an EC2 instance.
Static credentials which is the current default behavior.

Example code using IAM.

        iam := credentials.NewIAM("")
        s3Client, err := minio.NewWithCredentials("s3.amazonaws.com", iam, true, "")
        if err != nil {
                log.Fatalln(err)
        }

        buckets, err := s3Client.ListBuckets()
        if err != nil {
                log.Fatalln(err)
        }
        for _, bucket := range buckets {
                log.Println(bucket)
        }

Fixes #643

vadmeste · 2017-04-13T22:03:46Z

+			}
+			filename = filepath.Join(homeDir, ".mc", "config.json")
+			if runtime.GOOS == "windows" {
+				filepath.Join(homeDir, "mc", "config.json")


This looks like a typo

vadmeste · 2017-04-13T22:13:09Z

+	if endpoint == "" {
+		// IAM Roles for Amazon EC2
+		// http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html
+		endpoint = "http://169.254.169.254"


Shouldn't this be https ?

Nope it is only accessible from inside EC2 instance.

vadmeste · 2017-04-13T22:19:03Z

+	if err != nil {
+		return nil, err
+	}
+	resp, err := client.Do(req)


Close response body.

vadmeste · 2017-04-13T22:21:10Z

+	}
+
+	if err := s.Err(); err != nil {
+		return nil, errors.New("Failed to read from IAM metadata service")


I think it is bettter to append s.Err() to the returned error here.

vadmeste · 2017-04-13T22:24:04Z

+	if err != nil {
+		return ec2RoleCredRespBody{}, err
+	}
+	u.Path = path.Join(iamSecurityCredsPath, credsName)


Shouldn't we encode credsName here ?

You are right.

actually its not needed.. u.String() encodes it.

vadmeste · 2017-04-13T22:24:35Z

+		return ec2RoleCredRespBody{}, err
+	}
+
+	resp, err := client.Do(req)


Close resp body.

vadmeste · 2017-04-13T22:29:51Z

+	if e.CurrentTime == nil {
+		e.CurrentTime = time.Now
+	}
+	return e.expiration.Before(e.CurrentTime())


You mean 'After' here ?

No Before is correct. Read it as expiration time is before current time.

oh yes sorry, probably it is easier to read e.CurrentTime().After(e.expiration)

But usually it is better to use on the left side what you are validating against. Otherwise it would look like currentTime() is what we are validating not expiration.

vadmeste · 2017-04-13T22:36:13Z

+}
+
+// isExpired helper method wrapping the definition of expired credentials.
+func (c *Info) isExpired() bool {


This function doesn't seem to be useful, you can merge it in IsExpired()

isExpired() is used at two places one in Get() and another is IsExpired()

isExpired() is used at two places one in Get() and another is IsExpired()

yes, right.

vadmeste · 2017-04-13T22:40:19Z

Some missing LICENCES in some files

harshavardhana · 2017-04-14T00:10:08Z

Some missing LICENCES in some files

Yeah will add license at the end.

deekoder · 2017-04-18T19:01:54Z

@harshavardhana - is this ready for merge?

harshavardhana · 2017-04-18T19:04:21Z

@harshavardhana - is this ready for merge?

Not yet waiting on Mattermost.

vadmeste · 2017-04-22T15:39:42Z

+// NewIAM returns a pointer to a new Credentials object wrapping
+// the IAM. Takes a ConfigProvider to create a EC2Metadata client.
+// The ConfigProvider is satisfied by the session.Session type.
+func NewIAM(endpoint string) *Info {


I think this API needs to accept a role name as a parameter, not only endpoint. The application needs to get credentials from the right role name before performing the needed action which is only authorized by that role.

RoleName is not needed since the IAM RoleName is attached to the EC2 instance where the GET originates. So you are bound to be under the RoleName already.

Why do you think it is necessary?

Yes, you are right. I thought we can attach multiple IAM roles to one instance but we can only set one. (which is weird by the way and looks painful for the users)

I found this here, http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html

You *cannot* attach multiple IAM roles to a single instance, but you can attach a single IAM role to multiple instances. For more information about creating and using IAM roles,

houndci-bot · 2017-05-07T23:37:42Z

 // Different types of supported signatures - default is Latest i.e SignatureV4.
 const (
-	Latest SignatureType = iota
+	// Default signature is always V4.


comment on exported const SignatureDefault should be of the form "SignatureDefault ..."

harshavardhana · 2017-05-08T01:10:35Z

Unblocking and ready for review @vadmeste

vadmeste · 2017-05-08T12:19:20Z

+		signerType = parseSignatureType(value.SignatureType)
+	}
+
+	// FIXME: Session token should be supported.


This can't be fixed in this PR ?

Spec is little vague on post policy about this. Will need to explore more.

vadmeste · 2017-05-08T12:26:28Z

+	// ask for anything specific signature style.. Check if the
+	// credentials set returned the type of signature that should
+	// be used, if not proceed to use default.
+	if signerType == SignatureDefault && value.SignatureType != "" {


I think that the signature type that comes from config, if it exists, should override the one specified in the application code. That way, we can give users more power to configure.

Actually no. We have methods to override that see NewV4 and NewV2. We need to honor that. Since they don't take dynamic credentials.

vadmeste · 2017-05-08T12:51:20Z

-			c.secretAccessKey, location, metadata.contentLength, time.Now().UTC())
+		req = s3signer.SignV2(*req, accessKeyID, secretAccessKey)
+	case signerType.isStreamingV4() && method == "PUT":
+		req = s3signer.StreamingSignV4(req, accessKeyID,


No session token here ?

Need to figure it out .. not sure right now.

vadmeste · 2017-05-08T12:55:36Z

+
+// IsExpired returns if the credentials have been retrieved.
+func (e *EnvAWS) IsExpired() bool {
+	return !e.retrieved


This probably should always return true. A user can change his environment variables inside his application (who knows), and also it is no harm if we fetch from the environment each time Retrieve() is called.

That we already do.. it is to indicate if envs can ever be expired. They cannot be. It is not correct to call it not expired.. basically envs are similar to static creds in their behavior.

vadmeste · 2017-05-08T13:15:53Z

+
+import "fmt"
+
+// A Chain will search for a provider which returns credentials


This is well thought, I wasn't able to criticize this.

vadmeste · 2017-05-09T22:32:18Z

 		return nil, nil, err
 	}

+	if sessionToken != "" {


vadmeste · 2017-05-10T20:03:00Z

+
+// NewStatic returns a pointer to a new Credentials object
+// wrapping a static credentials value provider.
+func NewStatic(id, secret, token, signerType string) *Credentials {


Isn't better to create a new exported signature data type and use it instead of string ? that way the user would be less prone to error since any typo will make this module uses anonymous.

Actually it can be simplified..

vadmeste · 2017-05-10T20:50:43Z

+
+// NewFileAWSCredentials returns a pointer to a new Credentials object
+// wrapping the Profile file provider.
+func NewFileAWSCredentials(filename string, profile string) (*Credentials, error) {


NewFileAWSCredentials and NewFileMinioClient are always returnig nil, shall we remove the returned error ? if we do, it will be like other New() primitive for static, IAM and envs.

Yes looks like older problem. Will fix

This PR adds a new API - NewWithCredentials() Internally NewWithCredentials is now used with all APIs such as New(), NewV4(), NewV2() and NewWithRegion. Also brings a new package called `credentials` to manage various credentials type, currently the credentials package supports - Reading file from `.aws/credentials`, `.mc/config.json` - Reading env variables for AWS*, MINIO* - Fetching from IAM roles assigned to an EC2 instance. - Static credentials which is the current default behavior. Example code using IAM. ```go iam := credentials.NewIAM("") s3Client, err := minio.NewWithCredentials("s3.amazonaws.com", iam, true, "") if err != nil { log.Fatalln(err) } buckets, err := s3Client.ListBuckets() if err != nil { log.Fatalln(err) } for _, bucket := range buckets { log.Println(bucket) } ``` Fixes minio#643

vadmeste

LGTM

harshavardhana force-pushed the credentials branch 3 times, most recently from 61a6d51 to 7357588 Compare April 13, 2017 08:54

harshavardhana changed the title ~~[WIP] api: Add NewWithCredentials()~~ api: Add NewWithCredentials() Apr 13, 2017

harshavardhana force-pushed the credentials branch from 7357588 to 47bede5 Compare April 13, 2017 21:44

harshavardhana requested review from krishnasrinivas and vadmeste April 13, 2017 21:44

harshavardhana force-pushed the credentials branch from 47bede5 to ca93984 Compare April 13, 2017 21:54

vadmeste reviewed Apr 13, 2017

View reviewed changes

harshavardhana force-pushed the credentials branch from ca93984 to cf59834 Compare April 13, 2017 22:09

vadmeste reviewed Apr 13, 2017

View reviewed changes

harshavardhana force-pushed the credentials branch from cf59834 to 7c477e9 Compare April 14, 2017 00:11

harshavardhana mentioned this pull request Apr 14, 2017

read ~/.mc/config.json consistently in all libraries minio/minio-py#502

Closed

deekoder added the blocked label Apr 18, 2017

harshavardhana mentioned this pull request Apr 19, 2017

[Help Wanted] Add support for private/internal S3 VPC endpoints mattermost/mattermost#5173

Closed

vadmeste reviewed Apr 22, 2017

View reviewed changes

harshavardhana force-pushed the credentials branch 2 times, most recently from 8c3242f to 7a6ce85 Compare April 24, 2017 07:55

harshavardhana force-pushed the credentials branch 2 times, most recently from 0eb4826 to 8deedd1 Compare May 7, 2017 07:16

houndci-bot reviewed May 7, 2017

View reviewed changes

harshavardhana force-pushed the credentials branch 2 times, most recently from fdadb5b to 46c196c Compare May 8, 2017 00:04

harshavardhana force-pushed the credentials branch from 46c196c to d008c7d Compare May 8, 2017 08:44

vadmeste reviewed May 8, 2017

View reviewed changes

harshavardhana force-pushed the credentials branch from d008c7d to 9c88e6d Compare May 8, 2017 15:26

vadmeste previously approved these changes May 8, 2017

View reviewed changes

harshavardhana dismissed vadmeste’s stale review via 8e9721d May 8, 2017 21:41

harshavardhana force-pushed the credentials branch 4 times, most recently from 90f7686 to f43530b Compare May 9, 2017 01:29

vadmeste reviewed May 9, 2017

View reviewed changes

Comment thread api-presigned.go

return nil, nil, err

}

if sessionToken != "" {

Copy link
Copy Markdown

Member

vadmeste May 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

harshavardhana force-pushed the credentials branch 3 times, most recently from b0ba992 to 64743cc Compare May 10, 2017 01:46

vadmeste reviewed May 10, 2017

View reviewed changes

harshavardhana force-pushed the credentials branch from 64743cc to 414b641 Compare May 10, 2017 20:28

vadmeste reviewed May 10, 2017

View reviewed changes

harshavardhana force-pushed the credentials branch from 414b641 to 56cfb04 Compare May 10, 2017 21:47

vadmeste approved these changes May 12, 2017

View reviewed changes

harshavardhana merged commit 5d7ee33 into minio:master May 12, 2017

harshavardhana deleted the credentials branch May 12, 2017 17:45

wookasz mentioned this pull request Mar 8, 2018

Support fetching AWS S3 credentials from IAM roles assigned to EC2 instance argoproj/argo-workflows#786

Closed


		import "fmt"

		// A Chain will search for a provider which returns credentials

Conversation

harshavardhana commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshavardhana Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadmeste commented Apr 13, 2017

Uh oh!

harshavardhana commented Apr 14, 2017

Uh oh!

deekoder commented Apr 18, 2017

Uh oh!

harshavardhana commented Apr 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshavardhana commented May 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

harshavardhana commented Apr 13, 2017 •

edited

Loading

harshavardhana Apr 13, 2017 •

edited

Loading