Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Does the API base path require authorization? #66

Closed
andrewbonney opened this issue Sep 3, 2020 · 24 comments · Fixed by #67
Closed

Does the API base path require authorization? #66

andrewbonney opened this issue Sep 3, 2020 · 24 comments · Fixed by #67

Comments

@andrewbonney
Copy link
Contributor

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
Copy link
Contributor

prince-chrismc commented Sep 4, 2020

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
Copy link
Contributor

garethsb commented Sep 7, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

garethsb commented Sep 7, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

garethsb commented Sep 7, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

garethsb commented Sep 7, 2020

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

@garethsb
Copy link
Contributor

garethsb commented Sep 7, 2020

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
Copy link
Contributor

prince-chrismc commented Sep 7, 2020

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
  • API Base path https://api.example.com/x-nmos/<api name>/<api version>/
  • Server Base path https://api.example.com/

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 😄

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
Copy link
Contributor

garethsb commented Sep 7, 2020

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

Hmm, are we not transforming it to ^$?

@prince-chrismc
Copy link
Contributor

Depends on the regex implementation some do not require it.

@garethsb
Copy link
Contributor

garethsb commented Sep 7, 2020

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
Copy link
Contributor

garethsb commented Sep 7, 2020

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
Copy link
Contributor

"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
Copy link
Contributor

garethsb commented Sep 7, 2020

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

@andrewbonney
Copy link
Contributor Author

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
Copy link
Contributor

garethsb commented Sep 8, 2020

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

  • Access to / does not require a token (it can't as it's not in our namespace)
  • Access to /x-nmos (with or without a trailing slash) does not require a token just in case some APIs have auth enabled and some don't (such as IS-09 or the auth server itself)
  • If the relevant 'scope' is present with or without extra claims you are permitted 'read' access to any path between /x-nmos/<api name> up to /x-nmos/<api name>/<api version>/.
  • The x-nmos-* claims ONLY govern paths which exist AFTER the trailing slash in the API base path /x-nmos/<api name>/<api version>/, so it has nothing to do with the base path itself.
  • As noted above, an empty string claim such as "x-nmos-*": { "read": [ "" ] } should be disallowed in the schema.

Does that sound reasonable?

@garethsb
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants