Limited permission API keys

[mjallday]

This is a work in progress for #290 and #402

API keys that can be constrained to either read or write to a limited sub-set of endpoints.

E.g.

A POST to /api_keys

{
    "api_keys": [{
        "scopes": [
            {
                "path": "/debits",
                "permission": ["read", "write"]
            }
        ]
    }]
}

will create an API key that can create customers and read all debits whose ID starts with WD.

Note you can use asterisks as wildcards. URLS are matched exactly if a asterisk is not used so /customers/CU123does not match a permission of {"/customers": "r"} but it does match {"/customers*": "r"}

This does not implement OAuth as suggested in the original issue, this implementation seemed simpler for now but could be pushed in that direction once an MVP is out and working. I think this would be very handy for passing read only credentials for 3rd party integrations or auditors etc.

Internal implementation exists at https://github.com/balanced/balanced/issues/570 and https://github.com/balanced/balanced/pull/622

[matthewfl]

would it make more since to make this resource base rather then path based, since you could potently create a debit using /cards/asdf/debits or /customers/asdf/debits

[mjallday]

something like

{
    "permissions": {
        "customers.index": "r",
        "customers.CU123123": "w",
         "cards.create": "w"
    }
}

?

i'd like to see some examples on the syntax since i think the above example is not complete.

i feel like the path way is quite straight forward since i could do { "permissions": "/customers/CU123*" } to allow all operations on a customer (debit, credit, associate a funding instrument).

[mjallday]

@matthewfl did some more thinking about path based / endpoint based permissions.

1. paths should not change beyond revisions. we removed all versioning with rev 1.1 so we should no longer have to worry about uris changing between revisions. i think this is a point in favor of path based permissions
2. endpoint based permissions seem verbose. {"permissions": {"/debits*": "rw"}} versus {"permissions": {"debits.index": "r", "debits.create": "w", "debits.show": "rw", "debits.update": "w"}}. i'm not sure what read on the update endpoint and write on the index endpoint means, do i get a 400 for trying to say such a thing?
3. unsure how I would specify a single resource. e.g. how do i mirror {"permissions": ["/customers/CU123/orders": "rw"]}? {"permissions": ["customers.CU123.orders": "rw"]} works but it seems like it may as well be the path at that point, i've only swapped / for ..
4. how do people map between endpoint names and paths? seems like an internal detail.

these aren't exhaustive objections, just some points that i came up with and haven't been able to cleanly answer.

I had a look at how other companies use scopes; github, google, heroku, linkedin. it looks like most of them either pass straight strings write:repo_hook which is OK but not as granular (how do I say write only to one repo in particular? in this case the scopes are per-repo i believe) or bastardise the scope uris https://www.googleapis.com/auth/plus.profile.emails.read..

the oauth spec is not super helpful but it does say:

The value of the scope parameter is expressed as a list of space-
delimited, case-sensitive strings. The strings are defined by the
authorization server. If the value contains multiple space-delimited
strings, their order does not matter, and each string adds an
additional access range to the requested scope.

since this isn't a straight oauth impl i'll probably ignore that but i could see the scopes changing in an oauth impl to ?scopes=r:/customers%20w:/debits%20rw:/orders.

[matthewfl]

@mjallday I feel like you might consider looking at iam, since imo that is the most advance scope based auth system.

an idea that I had recently was to use regular expressions and match against a string representing the operation. eg .* would allow everything, while GET .* would only allow get requests. Then you could have a string for what operation is happening like: POST credits /customers/asdfasdf/credits amount=1234

[mahmoudimus]

@mjallday @matthewfl arent' scopes already part of the balanced api?

[matthewfl]

@mahmoudimus I believe that the scope implementation is more limited then what @mjallday is trying to do, eg there is a role, and some permissions associated with that role

[mjallday]

regex or glob is pretty much equiv. can be either or.

do you need to differentiate between a POST a PUT, DELETE and a PATCH? your proposal would have four separate permissions whereas the existing treats them all as a write. do I need to also specify HEAD as well as GET?

@mahmoudimus i looked at those scopes, not sure how we'd create arbitrary ones on the fly and how we'd use them to limit/deny writes. they appear to limit the scope of resources available to operate on but that's as far as they go.

wrt iam policies, here's an example

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Action": ["s3:ListAllMyBuckets"],
      "Resource": "arn:aws:s3:::*"
    },
    {
      "Effect": "Allow",
      "Action": [
        "s3:ListBucket",
        "s3:GetBucketLocation"
      ],
      "Resource": "arn:aws:s3:::bucket-name"
    },
    {
      "Effect": "Allow",
      "Action": [
        "s3:PutObject",
        "s3:GetObject",
        "s3:DeleteObject"
      ],
      "Resource": "arn:aws:s3:::bucket-name/*"
    }
  ]
}

i'm definitely not opposed to looking into these, they allow for the concept of roles which is not supported by the current implementation, each key needs to have permissions explicitly assigned. without trying to increase the scope (ha) of this pull-request it would be great to explore and identify any way we could tweak the implementation so that it's more forwards compatible.

[matthewfl]

@mjallday I just suggested regexes since I feel like any type of permissions situation could be satisfied with them. Also I think you should differentiate between a POST and a PUT/PATCH since you can't move money with PUT/PATCH requests

[mahmoudimus]

@matthewfl voiding a hold right?

[mjallday]

PUT to create would put a dent in that idea.

Can accomplish something similar with this example which will not allow creates but still suffers from PUT to creates being allowed since you would specify the guid on creation

{
    "permissions": {
        "/debits": "r", /* equiv to deny POST */ 
        "/debits*" "w" /* equiv to allow PUT */
    }
}

[mjallday]

Looking at the IAM example, we could restructure the permissions slightly to make them more future compatible with the syntax but i'd push back against actually implementing any additional functionality.

e.g.

{ 
    "scopes": [
        {
            "path": "/debits",
            "permission": ["read", "write"]
        }
    ]
}

if we decide to add deny actions, etc this format would make adding that much easier.

[matthewfl]

@mahmoudimus voiding holds doesn't move money 😉
@mjallday ggggggg right PUT to create....

[matin]

@mjallday: Can you define the schema for scopes?

[matin]

@mjallday

A few questions:

1. Could you add scenarios where an API key of different permissions can/can't perform certain functions?
2. What are the error messages returned when an authorized function is performed?
3. Can you update the permission on an API key?
4. What API key has the permission to create other API keys?
5. What about adding a description field to API Keys? It already had meta, but that doesn't seem like a clear way to indicate the purpose/use of different API keys.

[mjallday]

1. https://github.com/mjallday/balanced-api/blob/key-perms/features/api_keys.feature#L81-L85
2. Just standard error messages. e.g. "401 Unauthorized: Not permitted to perform index on marketplaces.". We could change this to say "This API key is not permitted ..." or add some additional context if you think that would clarify that only this key has this issue.
3. Yes
4. Any API key with no scopes specified (these keys have full control), any limited key with write permissions to the /api_keys path. Having the ability to write to an API key is pretty much the same as having an API key with unlimited permissions, you could quickly use that to create a new key or amend an existing key to have more power.
5. For labeling purposes? The dashboard uses the meta field to store this information and then display it. I want to explore further information into the key e.g. an expiration date, can we hold off on doing that until I explore the concept a bit further? Unless you have a clear use-case for it.

[mjallday]

@matin updated with a clearer error message that it's this particular API key that does not have permissions to perform that operation.

the generic message is "Not permitted to perform create on debits."

for the limited key it is "Not permitted to perform create on debits with this API key."

lmk if you'd like that further clarified in the error message.

[mjallday]

@balanced/spec-ialz any more feedback so I can move this forward?

[remear]

lgtm. My 2¢, I think users will want the ability to add a description to the key so they can keep better track of them unless we update the Dashboard to show the key permissions, perhaps in a key detail view.

[mjallday]

@remear does the meta field solve that issue?

[remear]

@mjallday I think so, at least for now.

[kyungmin]

I think users can optionally add name to an api key in the current dashboard. Is this going to be stored in meta for now as well?

[mjallday]

sure, i don't see what benefit having a first class field for storing this sort of data provides. unless we want to be able to add constraints to or sort by it. meta still provides the ability to filter.

i'd like to keep that out of scope and keep this pull-request focussed on a single enhancement.

[remear]

Sounds great to me. Ship it!

[kyungmin]

When will this be merged in?

[mjallday]

@kyungmin i've had a couple of customers request this feature recently. it's useful for giving accountants ro access to view data etc. this feature is ready to go imo so there's no development left unless you need additional features.

[zacksinclair]

I'm the Zack from that irc log. Just piping in to mention that we think this is extremely important and near critical to our effective use of Balanced. Would really like to see this merged!

[zacksinclair]

Following up on this again. There's a horror story of leaked AWS keys making the rounds right now - their unrestricted keys ended up leaking in one way or another, racking up significant AWS bills when they were scraped and used by unscrupulous types. These instances are user error for not using IAM, but at least AWS supports it.

The equivalent of this issue with a Balanced key could destroy a business. Consider a dev accidentally checking in an API key and it falling into inappropriate hands. Or any number of other ways a key could leak. Suddenly every credit card we have ever tokenized gets charged a maximum amount and the freshly topped up balance of our account is withdrawn to a newly tokenized bank account.

This is flat out dangerous as it is; security is dependent on numerous developers "carefully storing" keys. Obviously we all want to be careful, but mistakes and/or security holes happen. Restricting keys to certain actions would provide a significant layer of defense.

[mahmoudimus]

hey @zacksinclair we hear you :) we'll have something on this front soon.

[mjallday]

@mahmoudimus and i were talking last night. he was unhappy with the suggestion that we apply permissions directly to api keys.

I countered that we could provide roles to make this simpler to manage

{
    "roles": [{
        "name": "read-only",
        "scopes": [
            {
                "path": "/*",
                "permission": ["read"]
            }
        ]
    }]
}

{
    "api_keys": [{
        ...
        "roles": ["read-only"]
    }]
}

This however will still proliferate roles if you attempt to create a key per customer (e.g. let my customer read what data i have stored for them, let me create a merchant dashboard)

What could simplify that is if the role was not path based but rather used some other mechanism to describe the permissions. e.g.

{
    "role": [{
        "name": "show-customer", 
        "resources": ["balanced:customer:::CU123123"],
        "actions": ["customer.show", "customer.update"]
    }]
}

Where for a permission we have "customer.index" which is a combination of resource ("customer", "debit", "credit"... ) and action ("index", "update", "create", "delete", "show").

This is close to the AWS IAM permission

    {
      "Effect": "Allow",
      "Action": [
        "s3:PutObject",
        "s3:GetObject",
        "s3:DeleteObject"
      ],
      "Resource": "arn:aws:s3:::bucket-name/*"
    }

We could allow overriding the "resources" attribute on the api key itself which would allow this role to be specified once and then applied to many api keys.

@mahmoudimus if you can clarify if you think this would suit the issue that would be great. I initially explored this model but moved away from it because I did not know how to express something such as "allow this customer to update some limited subset of resources that belong to them". I think this particular implementation covers it (tho not sure if we would expose such functionality immediately).

I suspect just providing a RO role as a first step would get us 90% of the way there.

[mahmoudimus]

+1 @mjallday this is on the right track.

[kyungmin]

{
    "role": [{
        "name": "show-customer", 
        "resources": ["balanced:customer:::CU123123"],
        "actions": ["customer.show", "customer.update"]
    }]
}

How would you express the read-only mode in this format? Would it be ["*.show"]?
Do we also need to allow the users to specify the "effect" (allow/deny)?

[mjallday]

@kyungmin yes, *.show would effectively be read-only.

to begin with we'll have two pre-set roles for each marketplace, "read" and "read-write". a subsequent modification will allow creating custom roles but in the interest of speed we will not allow creation of custom roles.

[kyungmin]

#667 (comment) is about only allowing conducting charges. Would something like ["debit.update"] work for this?