AMWA-TV / is-10

AMWA IS-10 NMOS Authorization Specification
https://specs.amwa.tv/is-10
Apache License 2.0
3 stars 4 forks source link

Does the API base path require authorization? #66

Closed andrewbonney closed 4 years ago

andrewbonney commented 4 years ago

Paths such as /x-nmos/connection/v1.1 could require a token to access, and this is what the testing tool is currently assuming. However, the claims path structure which uses paths like single/senders/* means it's not obvious how you would provide or constrain access to this base path.

Is this path 'special' in that it provides an implicit read permission if any valid token is presented to it? Does the empty string "" signify permission to access this path? Or does this path not require authorization at all?

prince-chrismc commented 4 years ago

From https://github.com/AMWA-TV/nmos-authorization/blob/v1.0-dev/docs/4.4.%20Behaviour%20-%20Access%20Tokens.md#the-access-permissions-object

For NMOS API URL's that follow the standard pattern of: <api_proto>://<hostname>:<port>/x-nmos/<api type>/<api version>/ all path specifiers MUST start after the API version and its trailing slash

Does this conflict with our trailing slash rules? Or is more wording required to clarify the text?

Does the empty string "" signify permission to access this path

An empty string would be no permission and should be excluded from the token since it could not match anything with the regex equivalent.

garethsb commented 4 years ago

An empty string would be no permission and should be excluded from the token since it could not match anything with the regex equivalent.

An empty regex matches everything, I believed?

Since the docs currently say "all path specifiers MUST start after the API version and its trailing slash", I would say that the path closest to the root that can be expressed is therefore /x-nmos/<api type>/<api version>/.

Since the x-nmos-* private claims cannot express paths 'above' this, I would expect that they are simply not taken into account and only the registered claims are verified. I wouldn't express it as an implicit read permission, but that's the effect (since no API supports PATCH/POST/PUT for these paths).

It's slightly awkward that /x-nmos/<api type>/<api version>/ may have different permissions than without the trailing slash, but that's just the way it is, IMHO.

andrewbonney commented 4 years ago

Just to check, do you still think a token should be necessary to access paths before the trailing slash, or should these not require a token at all? If the former, would paths like /x-nmos/ still require a token?

garethsb commented 4 years ago

Sorry, yes, I meant to offer auth not being required at all 'above' /x-nmos/<api type>/<api version>/ as another option, unless that's covered somewhere in the auth spec already? Is there any downside in HTTP for clients providing an Authorization request header when it's not required?

andrewbonney commented 4 years ago

I don't believe there's a downside to the Auth header being present when it's not required, unless of course you send it to a 'rogue system' which could re-use the token until it expires.

I don't think it's a big deal to not require auth for the base path, but it does mean that resource server testing will need to be subtly specialised for each specification under test. This may be most challenging for the Registration API which doesn't offer a GET path which is guaranteed to exist beyond the base path.

garethsb commented 4 years ago

By 'base path', do you mean https://api.example.com/ or https://api.example.com/x-nmos/<api name>/<api version>/?

What's the need in the tests for a GET path that's guaranteed to exist? The pattern * matches zero characters, so https://api.example.com/x-nmos/registration/v1.3/ is covered by that pattern in the x-nmos-registration private claim. Is that enough?

(And I suppose there ought to be tests to make sure that https://api.example.com/ and up to https://api.example.com/x-nmos/<api name>/<api version> do not require auth.)

andrewbonney commented 4 years ago

By base path I meant https://api.example.com/x-nmos/<api name>/<api version>/

If we say that * covers https://api.example.com/x-nmos/registration/v1.3/, we need to be clear given the existing trailing slash policy that this also means https://api.example.com/x-nmos/registration/v1.3.

The issue that remains is that if you don't want to put "*" in the token, but you want to allow GETs to the base path along with one other path then this can't be expressed. For example, ["single/receivers/*"] would not allow access to the base path, and I don't think anything could be added to the token which could fix this. If we re-defined the claims slightly you could set the claims to ["/", "/single/receivers/*"], but this still feels slightly ambiguous given that the trailing slash policy specifies that the 'primary' path is the one without a trailing slash.

garethsb commented 4 years ago

Adding "" to the claim works, doesn't it?

garethsb commented 4 years ago

I'm not sure it needs to be in conflict with the trailing slash policy? GET /x-nmos/foo/v3.1 does not need authorization, but may redirect to /x-nmos/foo/v3.1/ which does need auth. On the other hand, if GET /x-nmos/foo/v3.1/ redirects to /x-nmos/foo/v3.1, it requires auth to do so, even though the target path doesn't.

prince-chrismc commented 4 years ago

An empty string would be no permission and should be excluded from the token since it could not match anything with the regex equivalent.

An empty regex matches everything, I believed?

Yes, says google Though we are not using regex directly, applying the transform rule would have this effect. This would mean to permit everything [ "" ] is possible. Is that a security vulnerability?

My Terminology

~What are the rules for trailing slashes? docs seems to say we leave out the trailing slash but in our path permissions it's omitted as well. Maybe is should be included in the token?~ Gareth answered while I was writing :smile:

Is there a use case were permission should be revoked for the API base path? Does that make sense? I could not image one...

Perhaps the permission should always given when the x-nmos-* is granted?

Scenarios thus far

Permission Effect
omitted always reject
"x-nmos-*": {} should not exist
"x-nmos-*": { "read": [] } should not exist
"x-nmos-*": { "read": [ "" ] } always permit (???)
"x-nmos-*": { "read": [ "*" ] } permits everything (except api base path???)
"x-nmos-*": { "read": [ "/" ] } only api base path (???)
"x-nmos-*": { "read": [ "*sender*" ] } only paths with 'sender'
garethsb commented 4 years ago

This would mean to permit everything [ "" ] is possible. Is that a security vulnerability?

Hmm, are we not transforming it to ^$?

prince-chrismc commented 4 years ago

Depends on the regex implementation some do not require it.

garethsb commented 4 years ago
Permission Effect
omitted always reject
"x-nmos-*": { "read": [ "" ] } maps to ^$, matches only the API version base path with trailing slash
"x-nmos-*": { "read": [ "*" ] } maps to ^.*$, matches everything including the API version base path with trailing slash
"x-nmos-*": { "read": [ "/" ] } don't do that (would match the API version base path with an extra trailing slash)
"x-nmos-*": { "read": [ "*sender*" ] } maps to ^.*sender.*$, so only and all paths with 'sender'
garethsb commented 4 years ago

Depends on the regex implementation some do not require it.

The empty regex matches every string, the ^$ regex matches only the empty string, so they're different...

prince-chrismc commented 4 years ago

"x-nmos-*": { "read": [ "" ] } maps to ^$, matches only the API version base path with trailing slash

Does that make sense to do? Yes that's fine if we do it, but I would never expect it in a deployment.

"x-nmos-*": { "read": [ "*sender*" ] } maps to ^.*sender.*$, so only and all paths with 'sender'

Did you mean to include the API base path? Ie, so only API base path and all paths with 'sender'

garethsb commented 4 years ago

I meant: (only) all paths with 'sender'

andrewbonney commented 4 years ago

Perhaps it's worth thinking about this another way. I can't currently see any reasons why it makes sense to restrict access to the API base path if you are granting access to some other paths in the same API. On that basis, is it really worth increasing the size of every token by adding an empty string element to each claims array (which also has the potential to get forgotten) for the sake of being explicit?

garethsb commented 4 years ago

Yes, it would seem very reasonable that everything up to /x-nmos/<api name>/<api version>/ isn't covered by the x-nmos-* private claims at all, just requiring the relevant scope to be present? The wrinkle is that the x-nmos-* patterns "" and "*" both provide a way to also match that API base path. We could prohibit (in the schema) the empty string from appearing in the array, but due to the wildcard one, should we make an explicit statement that "*" doesn't apply to the API base path?

(As an aside, was there any discussion about use cases for separately authorizing different API versions?)

andrewbonney commented 4 years ago

Yes, I think words certainly need adding if we define a special case regarding the path matching.

I don't believe there has been any discussion of separately authorising different versions.

prince-chrismc commented 4 years ago
Permission Currently Proposed
omitted always reject
"x-nmos-*": {} against schema
"x-nmos-*": { "read": [] } against schema
"x-nmos-*": { "read": [ "" ] } always permit (???) against schema
"x-nmos-*": { "read": [ "*" ] } permits everything (except api base path???) permits everything (to be easy)
"x-nmos-*": { "read": [ "/" ] } only api base path / does not match anything
"x-nmos-*": { "read": [ "*sender*" ] } only paths with 'sender' only paths with 'sender' plus implicit api base path

If the API scope is granted, resource servers SHALL permit the API base path <path> of that API along with those explicit given in the private claim of a token.

You have to have the scope to have the private claim, I don't think we need an extra statement in that case.

andrewbonney commented 4 years ago

Sounds about right, although I think we may be able to avoid the * at the start of paths. I think this boils down to:

Does that sound reasonable?

garethsb commented 4 years ago

Yes, ^ that! Very clear, Andrew.

Not sure what you mean by "I think we may be able to avoid the * at the start of paths"?

andrewbonney commented 4 years ago

Not sure what you mean by "I think we may be able to avoid the * at the start of paths"?

I was reading "*sender*" incorrectly. I realise now in the case of IS-05 that actually means you can use single or bulk.

garethsb commented 4 years ago

And * is useful on its own too, to mean everything...

I was wondering if we could define * to be equivalent to regex pattern .+ rather than .*, but I don't know if there are cases where matching zero chars as well as one-or-more chars later in the path would be useful (plus it would be a surprising definition given typical filesystem/shell definition of the * wildcard). So I think keeping it as now and including your clear statement that "The x-nmos-* claims ONLY govern paths which exist AFTER the trailing slash in the API base path..." is the best option.