Add Alibaba auth method #8

tyrannosaurus-becks · 2018-07-11T23:28:26Z

This PR adds the logical code itself for the Alibaba auth method.

The approach mimics that of the AWS IAM auth method.

kalafut · 2018-07-16T23:17:57Z

arn.go

+	"strings"
+)
+
+func parse(a string) (*arn, error) {


Something like parseARN(s string) would be more self-descriptive.

I can update that! Yeah that would make a lot more sense. :-)

kalafut · 2018-07-16T23:27:18Z

path_login.go

+		return nil, fmt.Errorf(`unable to parse entity's arn %s due to %s`, callerIdentity.Arn, err)
+	}
+	if parsedARN.Type != arnTypeAssumedRole {
+		// We haven't tested with other arn types and would like to understand and test these use cases before blindly


I'd probably remove this comment.

OK, I have it in two places so I'll strip it in both. I wasn't sure if it'd seem helpful or terse.

kalafut · 2018-07-16T23:34:38Z

path_login.go

+	if arn == "" {
+		return nil, errors.New("unable to retrieve arn from metadata during renewal")
+	}
+	roleName := req.Auth.InternalData["role_name"].(string)


Use the if roleName, ok := ... pattern instead so the type assertion doesn't panic if "role_name" isn't present.

Oh good catch. I actually copy pasta'd that code from here: https://github.com/hashicorp/vault/blob/master/builtin/credential/aws/path_login.go#L929. That may be a bug we should fix in Vault too.

kalafut · 2018-07-16T23:38:03Z

path_role.go

+			},
+			"arn": {
+				Type:        framework.TypeString,
+				Description: `arn of the RAM principals to bind to this role.`,


Should this be "principal"?

Actually I should probably take the word "principal" out because while there is a principal ID mentioned in the Alibaba docs, it's not an entity that's at the forefront of how people using this auth method will be thinking. Rewriting that to, "ARN of the RAM to bind to this role."

kalafut · 2018-07-17T16:05:36Z

path_login.go

+
+	parsedARN, err := parse(callerIdentity.Arn)
+	if err != nil {
+		return nil, fmt.Errorf(`unable to parse entity's arn %s due to %s`, callerIdentity.Arn, err)


Backticks intended?

Yeah, but this and the next do differ from the rest, so I'll update them to match.

kalafut · 2018-07-17T16:05:44Z

path_login.go

+		// We haven't tested with other arn types and would like to understand and test these use cases before blindly
+		// granting access so we don't create a security vulnerability. Please open a ticket describing your use case
+		// for another arn type to get that started.
+		return nil, fmt.Errorf(`only role arn types are supported at this time, but %s was provided`, callerIdentity.Arn)


Backticks intended?

kalafut · 2018-07-17T16:19:26Z

path_login.go

+	if response.StatusCode != 200 {
+		b, err := ioutil.ReadAll(response.Body)
+		if err != nil {
+			return nil, fmt.Errorf("error reading reponse body: %s", err)


typo: response

What? You don't know about the less-known child of a response, a reponse?

kalafut · 2018-07-17T16:40:06Z

path_role.go

+			},
+			"arn": {
+				Type:        framework.TypeString,
+				Description: `arn of the RAM principals to bind to this role.`,


Backticks intended?

Fixed the backtickathon.

kalafut · 2018-07-17T16:49:13Z

path_role.go

+	}
+}
+
+func pathListRoles(b *backend) *framework.Path {


Why support both role/ and roles/? Just role/ seems pretty common.

Because AWS does? /shrug :-)

I must admit I do like the approach because I think in the CLI it makes sense to type $ vault list roles; whereas in the API I think it makes sense to GET /alibaba/role and expect a list back. Gives the user choice of either.

I also must admit I think it's odd there are two *framework.Path objects for the two endpoints, when it would be possible to make a regex that matched both. However, since the AWS code did it that way I did too because I thought it might be preferred for some reason.

You might want to survey some other backends as AWS might be the exception. Personally, I like one way mainly because when I see two different paths (docs/path-help/etc.) I'm going to want to know what is different between them and which I need to use.

If you stick with two, I'd go with the roles?/ regex method vs. a separate path.

Oh man, that's a tough one, because I see it done different ways. For instance, the AWS auth method does it this way (ex. GET /role/elk, LIST /roles, LIST /role), but the AppRole auth method does it differently (ex. GET /role/elk, LIST /role), and the ActiveDirectory secrets backend does it differently (ex. GET /roles/elk, LIST /roles).

I must say that I like the last the best, but you raise a very good point and I will check in with the team about this because consistency would increase usability.

Jeff thinks the way it currently is is the current best practice. However, more of the team may or may not weigh in.

It's all a mess. Whatever we decided in standup :-)

I asked Chris about it after standup today in our 1:1 and we decided to leave it as-is for now.

The reason for leaving this alone was it matched how "roles" were defined across the other auth methods, not that we decided anything. I think the general rule should probably be in context of the thing being developed. For examples, most, if not all, auth methods use "role", let's go with that. That does not generally mean that we should use singular vs plural names for paths just that in the context of auth methods, that seemed the most right.

kalafut · 2018-07-17T20:56:01Z

path_role.go

+func (b *backend) operationRoleCreate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+
+	roleName := data.Get("role").(string)
+	if roleName == "" {


Can this ever be true? If not present the route shouldn't match.

Oh! Yes, you're absolutely right. Will strip that out. Less is more.

kalafut · 2018-07-17T20:56:15Z

path_role.go

+func (b *backend) operationRoleUpdate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+
+	roleName := data.Get("role").(string)
+	if roleName == "" {


Can this ever be true? If not present the route shouldn't match.

kalafut · 2018-07-17T20:57:03Z

path_role.go

+			// We haven't tested with other arn types and would like to understand and test these use cases before blindly
+			// granting access so we don't create a security vulnerability. Please open a ticket describing your use case
+			// for another arn type to get that started.
+			return nil, fmt.Errorf(`only role arn types are supported at this time, but %s was provided`, entry.ARN)


Are backticks intended?

kalafut · 2018-07-17T20:58:27Z

path_role.go

+
+func (b *backend) operationRoleDelete(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+	roleName := data.Get("role").(string)
+	if roleName == "" {


Can this ever be true? If not present the route shouldn't match.

kalafut · 2018-07-17T21:02:14Z

path_role.go

+		Pattern: "role/" + framework.GenericNameRegex("role"),
+		Fields: map[string]*framework.FieldSchema{
+			"role": {
+				Type:        framework.TypeString,


Probably want this to be the new framework.TypeLowerCaseString

Very good point. To make sure this was safe to do, I tried to create a couple roles in Alibaba with the same name but different casing. It turned out I couldn't, which is good because the arn is always lower case, and things could have gotten weird with matching role names to arn names. I'll lower-case it, it should be safe.

kalafut · 2018-07-17T21:02:56Z

role_manager.go

+	if roleName == "" {
+		return nil, errors.New("missing role name")
+	}
+	entry, err := s.Get(ctx, "role/"+strings.ToLower(roleName))


Can eliminate ToLower if the roles are lowercase type in path_role.go

Thanks! Yeah I ended up stripping that in a whole bunch of places.

kalafut · 2018-07-17T21:03:00Z

role_manager.go

+func (m *RoleManager) Delete(ctx context.Context, s logical.Storage, roleName string) error {
+	m.roleMutex.Lock()
+	defer m.roleMutex.Unlock()
+	return s.Delete(ctx, "role/"+strings.ToLower(roleName))


Can eliminate ToLower if the roles are lowercase type in path_role.go

kalafut · 2018-07-17T21:03:22Z

role_manager.go

+	if roleEntry == nil {
+		return errors.New("nil role entry")
+	}
+	entry, err := logical.StorageEntryJSON("role/"+strings.ToLower(roleName), roleEntry)


Can eliminate ToLower if the roles are lowercase type in path_role.go

kalafut · 2018-07-17T21:06:36Z

role_manager.go

+)
+
+type RoleManager struct {
+	roleMutex sync.RWMutex


Is this needed?

I think so.

Without it, one thread could read a role, another could update it, and then the previously read into memory role that's now outdated could be acted upon.

If we think that's harmless, we could do away with it because the mutex does cause a performance hit. And it could be argued that the mutex doesn't really solve that because while it prevents concurrent writes to the same object, it doesn't enforce the proper order of operations unless the lock is awarded based on the timestamp of when it was requested... and there's no promise of that.

And technically, I don't think a role that's read from storage, even if it's the same role, will have the same pointer as one on a different thread. So without the mutex, the -race flag still won't pick up a race.

Yeah, I see a mix of lock usage for role type operations, with probably more not using them. Thoughts on this @briankassouf ?

But assuming we did want to lock for some operations, does this approach achieve that? For example, after I call roleMgr.Read() I have a role but the mutex is unlocked and another goroutine could update the role before I complete whatever I needed to do. Where I've see locking, it is at a higher level, like at the start of pathRoleUpdate with a defer lock.unlock().

Hm, that's a good point. Yes, I did reduce the amount of code inside the mutex.

I think I'll just do away with it since it adds complexity, yet its value is negotiable since it doesn't necessarily ensure operations occur in any proper order. I mean, yes, while you could get funny outcomes from two competing threads mutating and/or reading the same role, you could also get funny outcomes from using a mutex the whole way through but having them happen in an odd order.

In edge cases where two actors are working upon the same role at the same time, neither actor would have knowledge of the other or would be surprised by an outcome effected by lacking mutexes.

So, yeah, stripping it.

kalafut

Looking good! Only minor comments from me on the implementation (haven't dug into the tests yet)

kalafut · 2018-07-17T21:15:45Z

path_role.go

+			return nil, fmt.Errorf("role name must match arn name of %s", arn.RoleName)
+		}
+		if entry.ARN.Type != arnTypeRole {
+			// We haven't tested with other arn types and would like to understand and test these use cases before blindly


I'd remove this comment.

kalafut · 2018-07-17T21:16:53Z

path_login.go

 	if err != nil {
-		return nil, fmt.Errorf(`unable to parse entity's arn %s due to %s`, callerIdentity.Arn, err)
+		return nil, fmt.Errorf("unable to parseARN entity's arn %s due to %s", callerIdentity.Arn, err)


Overzealous search and replace 🙂

Hunted these down and fixed them!

kalafut · 2018-07-17T21:17:07Z

path_role.go

 	if err != nil {
-		return nil, fmt.Errorf("unable to parse arn %s: %s", arn, err)
+		return nil, fmt.Errorf("unable to parseARN arn %s: %s", arn, err)


Overzealous search and replace 🙂

kalafut · 2018-07-17T21:17:14Z

path_role.go

 		if err != nil {
-			return nil, fmt.Errorf("unable to parse arn %s: %s", arn, err)
+			return nil, fmt.Errorf("unable to parseARN arn %s: %s", arn, err)


Overzealous search and replace 🙂

jefferai · 2018-07-24T21:58:09Z

path_login.go

+	}
+	headers, err := parseHeaders(b64Header)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing identity_request_headers: %v", err)


Please use errwrap here and in the blocks below.

jefferai · 2018-07-24T21:58:37Z

path_login.go

+
+	response, err := b.getCallerIdentityClient.Do(request)
+	if err != nil {
+		return nil, fmt.Errorf("error making request: %s", err)


Thanks! I grepped for all my uses of Errorf and went through each of them and anything that had an err involved, I used errwrap.Wrapf instead, so I got the ones you pointed out, and the rest of them too. Just pushed that up.

kalafut · 2018-07-24T22:00:03Z

path_login.go

+	if b64Url == "" {
+		return nil, errors.New("missing identity_request_url")
+	}
+	identityReqUrl, err := base64.StdEncoding.DecodeString(b64Url)


nit: golint would prefer identityReqURL

tyrannosaurus-becks · 2018-08-01T23:15:15Z

This PR is currently blocked by hashicorp/vault#4993 adding a header field type.

jefferai · 2018-08-09T18:12:52Z

arn.go

+		return nil, fmt.Errorf("unrecognized arn: contains %d colon-separated fields, expected 5", len(outerFields))
+	}
+	if outerFields[0] != "acs" {
+		return nil, errors.New("unrecognized arn: does not begin with \"acs:\"")


You can use backticks here to avoid having to escape quotes

jefferai · 2018-08-09T18:23:59Z

tools/tools.go

+	}
+
+	u := base64.StdEncoding.EncodeToString([]byte(getCallerIdentityRequest.URL.String()))
+	b, err := json.Marshal(getCallerIdentityRequest.Header)


Once the headers PR lands this should just embed the Header directly and let it be JSON encoded, then decoded by TypeHeader.

Cool! I just updated that PR to support b64 and pushed the changes. When it's merged I'll add it to this PR, and then to AWS auth.

jefferai · 2018-08-09T19:56:30Z

path_login.go

+				Type:        framework.TypeString,
+				Description: "Base64-encoded full URL against which to make the Alibaba request.",
+			},
+			"identity_request_headers": {


TypeHeader (just adding this as a reminder!)

jefferai · 2018-08-09T19:58:13Z

path_login.go

+				"role_name":     parsedARN.RoleName,
+			},
+			InternalData: map[string]interface{}{
+				"role_name": parsedARN.RoleName,


This is duplicated between here and Metadata -- only need one.

jefferai

Looks really good! Just swapping out the header stuff and changing the endpoint check.

This reverts commit 3f64f26, reversing changes made to f517568.

kalafut · 2018-08-13T21:07:24Z

path_login.go

-				Description: `Base64-encoded JSON representation of the request headers. 
-This must include the headers over which Alibaba has included a signature.`,
+				Type: framework.TypeHeader,
+				Description: `The request headers. This must include the headers over which Alibaba


Just a reminder that I'm going to do that in an immediate follow-on PR when this is merged. Going to do it all at once so I don't miss anything.

kalafut · 2018-08-13T21:08:57Z

path_login.go

@@ -60,19 +60,12 @@ func (b *backend) pathLoginUpdate(ctx context.Context, req *logical.Request, dat
 		return nil, errwrap.Wrapf("error parsing identity_request_url: {{err}}", err)
 	}

-	b64Header := data.Get("identity_request_headers").(string)
-	if b64Header == "" {
+	header := data.Get("identity_request_headers").(http.Header)


headers seems more accurate since this is a collection.

Add Alibaba auth method

add alibaba auth code

0e693d0

tyrannosaurus-becks requested review from kalafut and briankassouf July 11, 2018 23:47

kalafut reviewed Jul 16, 2018

View reviewed changes

kalafut reviewed Jul 17, 2018

View reviewed changes

feedback so far

df9cee7

kalafut reviewed Jul 17, 2018

View reviewed changes

simplify ttls, edits from testing

f317614

kalafut reviewed Jul 17, 2018

View reviewed changes

Becca Petrin added 2 commits July 18, 2018 10:25

fixes from additional feedback

158eb1a

merge master

88f6a19

jefferai reviewed Jul 24, 2018

View reviewed changes

kalafut reviewed Jul 24, 2018

View reviewed changes

Becca Petrin added 5 commits July 24, 2018 16:05

rename pkg and strip roleManager

db36f64

add bound cidrs

078c0cc

Merge branch 'master' into add-ram-auth-code

6408af7

add test for bound cidrs

a96115a

dont require the role name in vault to match alibaba

cc36dee

tyrannosaurus-becks mentioned this pull request Jul 25, 2018

Add a header type field hashicorp/vault#4993

Merged

Becca Petrin added 2 commits July 25, 2018 13:28

use errwrap

ea59053

uppercase URL

c7ebd9b

jefferai reviewed Aug 9, 2018

View reviewed changes

Becca Petrin added 5 commits August 9, 2018 14:00

dont duplicate rolename

f517568

merge master

3f64f26

Revert "merge master"

43cfd03

This reverts commit 3f64f26, reversing changes made to f517568.

update dependencies

e445f40

convert to type header

c81745c

kalafut reviewed Aug 13, 2018

View reviewed changes

briankassouf approved these changes Aug 16, 2018

View reviewed changes

strip endpoint hard-code

454bb5c

tyrannosaurus-becks merged commit 791ef1b into master Aug 16, 2018

tyrannosaurus-becks deleted the add-ram-auth-code branch August 16, 2018 18:00

tyrannosaurus-becks pushed a commit that referenced this pull request Aug 16, 2018

Merge pull request #8 from hashicorp/add-ram-auth-code

5945026

Add Alibaba auth method

tyrannosaurus-becks pushed a commit that referenced this pull request Aug 16, 2018

Merge pull request #8 from hashicorp/add-ram-auth-code

fe8c011

Add Alibaba auth method

Add Alibaba auth method #8

Add Alibaba auth method #8

Conversation

tyrannosaurus-becks commented Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrannosaurus-becks Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrannosaurus-becks Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrannosaurus-becks commented Aug 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyrannosaurus-becks commented Jul 11, 2018 •

edited

Loading

tyrannosaurus-becks Jul 17, 2018 •

edited

Loading

kalafut Jul 17, 2018 •

edited

Loading

tyrannosaurus-becks Jul 18, 2018 •

edited

Loading