RFC on Refactor of AWS Secret Backend

[joelthompson]

I've started to do the (long-delayed) refactor I mentioned in #2817 (comment) and I'd like to open up an RFC on my proposed changes before writing too much code.

The existing behavior is as follows (as best I understand the code):

• Role creation/update takes in either a policy or arn parameter, but not both.
  • Zero validation is done on arn parameters (beyond ensuring that a policy parameter is not also supplied)
  • The only validation on the policy parameter (beyond that an arn is not also supplied) is that it is valid JSON
  • Whichever value is supplied is stuffed into the storage backend for the given role
• Role reading looks at the stored value; if it starts with arn: the returned data structure has arn as a key, and otherwise, it has policy as a key. This can lead to such weird behavior as vault write aws/roles/foo arn=bar succeeding but a subsequent vault read aws/roles/foo has a policy variable in the output.
• When reading from the aws/creds endpoint, it always creates an IAM user.
  • If the stored role data starts with arn: Vault assumes it's the ARN of a manged policy and attempts to attach it to the user.
  • Otherwise, it assumes it's a JSON-formatted IAM policy and attempts to add it to the user being created.
• When reading from the aws/sts endpoint:
  • If the stored role data starts with arn:, then it assumes it's a role ARN to assume and attempts to assume it, but only if the string also contains :role; otherwise, it errors out.
  • Otherwise, it attempts to call sts:GetFederationToken with the policy data as the Policy paramemter

What I am proposing is:

• Role creation/update takes in the following parameters:
  • credential_type: One of iam_user, assumed_role, or federation_token, to be explicit about the credential type being requested, rather than relying on some hard-to-discern behavior based on the specific data of the role.
  • policy_arns: A framework.TypeCommaStringSlice of managed policy ARNs to be attached to a created IAM user. This would only be valid when credential_type is iam_user. I believe this would solve the request in multiple arn options for aws backend over vault client (cli) #2817
  • role_arns: The ARNs of the AWS role allowed to be assumed when calling AssumeRole. This would only be valid when credential_type is assumed_role.
  • policy_document: The JSON encoding of an AWS policy document. This would be valid for all policy types.
    • When credential_type is assumed_role then the behavior requested in Feature Request - AWS Secret Backend: Policy support to STS #3751 would apply, with the following notes:
      • The client can supply a single requested role ARN to assume. If the requested role ARN is not in the role_arns for the role, then fail the login. Otherwise, succeed.
      • If no requested role ARN is specified, and there is only a single entry in role_arns then assume that (current behavior).
      • If no requested role ARN is specified, and there are multiple entries in role_arns then fail the login. (In the future, we might consider the ability to specify a default or to codify a default, e.g., the first entry in the list.)
    • When credential_type is iam_user then the policy would be attached to the IAM user created. This would be in addition to any managed policy ARNs attached.
    • When credential_type is federation_token then this would maintain the current behavior.
• aws/sts becomes an alias for aws/creds because the separation is already encoded in the roles.
• Retrieving credentials through either the aws/sts or aws/creds endpoint will honor the credential_type in the role regardless of which endpoint is specified, and the parameters of the role will behave as described above.

I realize my proposal is a bit verbose in terms of asking clients to be more explicit about what they're asking for, but I personally think it's better than what is there now, and I'd welcome feedback about how to improve it! :)

I don't (yet) have a backwards compatibility story completely ironed out yet. I'd like to get feedback about my proposal first, and then I can figure out how to handle backwards compatibility.

Thoughts?

EDIT: Updated in response to feedback to reflect current thinking.

[jefferai]

Hi Joel,

This is looking great! A few questions:

For role_arn, could it be role_arns and allow the caller to select a role? That would add flexibility, but I don't know if it opens up some potential security holes (I don't see why not as it seems mostly equivalent to just making two different roles and giving the user access to both).

For policy_document, I wonder if we should add a TypeJSON that would allow either a serialized string, or a JSON object that is not serialized. This could be useful anywhere the user has input in JSON but we don't want to force them to serialize it if they can attach it properly to the request.

Overall I think this looks like it will make it much easier to use and bring some much needed order to things!

[joelthompson]

Thanks Jeff! :)

For role_arn, could it be role_arns and allow the caller to select a role? That would add flexibility, but I don't know if it opens up some potential security holes (I don't see why not as it seems mostly equivalent to just making two different roles and giving the user access to both).

I agree it shouldn't open any security vulnerabilities, but I reserve the right to change my mind when I start writing the code :)

The question is, what happens when the user attempts to retrieve credentials? I think the behavior when a role_arn is specified is obvious (if it's allowed by the role, then allow it; if it's not in the allowed roles, or the credential_type is not assumed_role, then deny). But what about when it's not? If there's only one allowed ARN in the Vault role, then just select that (achieving backwards compatibility). If there are multiple allowed ARNs in the Vault role, I think it should fail the login (rather than do something like pick the first ARN in the list). Thoughts?

For policy_document, I wonder if we should add a TypeJSON that would allow either a serialized string, or a JSON object that is not serialized. This could be useful anywhere the user has input in JSON but we don't want to force them to serialize it if they can attach it properly to the request.

That's an interesting proposal. I like it and think it'll make it much easier for users. I'll try to take a crack at it.

[jefferai]

Yeah, I think if on cred fetch they don't actually provide an arn and there are multiple, then deny the fetch with an appropriate error. Or, another parameter to specify a default. (Or, codify a default via documentation.)

[joelthompson]

I started working through the backwards compatibility/upgrade story, and I realized there's an ambiguity in the role definition as implemented today. If the role data has a policy document, it can be either attached to an IAM user or used as the Policy parameter when calling GetFederationToken. I don't believe there's any way to disambiguate this a prior, though I'd love to have somebody correct me on this (though I'd certainly love to have somebody correct my thoughts here).

I think the options are:

1. Change credential_type to credential_types and accept multiple credential types; this can be specified by clients upon requesting creds, and it will have a default value based on the path requested (i.e., /aws/creds or /aws/sts). I believe this preserves the most backwards compatibility but is also the most complex for users to think about. The merging of the /aws/creds and /aws/sts endpoints would mean that a policy which allowed retrieving iam_user credentials would now allow retrieving federation_token credentials and vice versa. I believe there's one small difference between the two and one big difference. The small difference is the TTLs -- AWS inherently caps the TTL of federation_token creds but not iam_user creds. The big difference is that federation_token creds are the logical intersection of the permissions described by policy data and the permissions granted to the IAM user Vault uses to call sts:GetFederationToken while iam_user creds have exactly the permissions described by the policy data.
2. A variant of the above, but only allow clients to specify a single credential type when creating a role or updating an existing role. Multiple allowed credential types would only show up when upgrading a previous role. Would also require a breaking change in the API on the management of roles, requiring users to specify a credential type.
3. A variant of the above, but when creds are requested through the /aws/sts endpoint, enforce that they are either an assumed_role or federation_token type
4. Break backwards compatibility even further and arbitrarily decide that any ambiguity is resolved in favor of either iam_user or federation_token.

My inclination is to pick option 2, and if option 3 is easy, then also implement that. (Of course, there's also option 5: abandon this refactor because it's harder than I originally anticipated and might not be worth it given some of these tradeoffs. I'm not advocating it but do want to explicitly mention it.)

[jefferai]

Hi Joel,

I with you on 2 and ideally 3 as well. With 3, do you mean, enforce in the role that it's one of those types, or in the request data?

People use AWS quite a lot and I am concerned about backwards compat, but at the same time I think that making it more explicit and clear and non-ambiguous is more important in the long run. It's never a good time to rip off the band-aid...

[joelthompson]

With 3, do you mean, enforce in the role that it's one of those types, or in the request data?

I'm a little confused by your question. My thought is that Vault should refuse to hand out iam_user credentials for requests made though the aws/sts endpoint, and that enforcement would be done by the request handler for the aws/sts endpoint. Does that answer your question?

I am concerned about backwards compat.... It's never a good time to rip off the band-aid...

Totally with you on this. I have some ideas and am writing code. I think I can make this work, but I need a couple more days before the code is ready for public consumption, and I think it'll be more productive to have that discussion once I've gotten some real code out there.

[flyinbutrs]

Unless I'm misunderstanding the proposal, I don't like the idea of multiple role_arns specified for a single aws/roles/${role} entry. IMO, you should create two separate entries in aws/roles for two different role_arns. This would ensure that any user granted access to a role in policy is allowed to assume only that role_arn. I could simple not use the role_arns overloading, but to me this would be easier to misconfigure.

Otherwise, this all jives with how I'd like to see Vault's AWS secrets provider function. Also, I agree with @jefferai that backwards compatibility is unfortunately a requirement.

[jefferai]

@flyinbutrs Actually I was suggesting that we break it :-)

[joelthompson]

I don't have a strong opinion about role_arns vs role_arn -- I've written the code for the former, but it's not merged, and it should be fairly straightforward to change. That being said, many things have been changing from singular to plural and this is in line with it. Further, this more naturally mimics the AWS semantics, e.g., you could write an IAM policy of:

{
    "Version": "2012-10-17",
    "Statement": [{
        "Effect": "Allow",
        "Action": "sts:AssumeRole",
        "Resource": ["arn:aws:iam::123456789012:role/Role1", "arn:aws:iam::123456789012:role/Role2"],
    }]
}

so I think it makes sense to have Vault semantics more closely mirror those of AWS. Similarly with policy ARNs, if you specify more than one policy, you get the permissions of all of the policies combined, so if you specify multiple role ARNs, it also feels natural to be able to select which of the roles you want to get access to.

Otherwise, I think everything should be backwards compatible EXCEPT the reading of the role data. The main concern I have around that is, as I have it now, you write in a credential_type string to the role data, but you read out a credential_types (plural) JSON list from the role data, and that can be confusing and might cause issues with Terraform's Vault provider. The other option is accepting a credential_types list parameter but only allowing users to specify a single item (also seems unfortunate) or accepting multiple credential types (potentially confusing interactions with different parameters across credential types).