ASVS
ASVS copied to clipboard
OWASP
13.4.2 seems too broad and not testable
Access control in GraphQL has different facets that I believe can't be generalized. I imagine it would be worth it to separate GraphQL-specific access control into query-level, object-level, and field-level access control.
Current 13.4.2:
| # | Description | L1 | L2 | L3 | CWE |
|---|---|---|---|---|---|
| 13.4.2 | Verify that GraphQL or other data layer authorization logic should be implemented at the business logic layer instead of the GraphQL layer. | ✓ | ✓ | 285 |
Maybe I misinterpreted it, but I think it's more architecture requirement to say, that do not build access control to GraphQL, but use an extra business logic layer for that.
Ok, I understand this requirement is targeting how to implement but not necessarily what to implement. For example, the IDOR control (4.2.1) is directly relevant to REST access control. Should this standard call out basic access controls for GraphQL?
Access control is access control, whatever is the technology or solution for that. My point is - we don't have and we will not make separate access control for api, separate for graphql, for "oldschool solutions" etc.
So @elarlang I think we need to think whether we want to unify this or not. It may be that we want to mention this issue specifically for GraphQL because it is a specific problem when using GraphQL. I think we need to have a think about this.
Not suggesting we add all of these requirements, but some sample access control requirements specific to GQL could be:
- Queries are used to only fetch data, any state changes use mutation operations
- Operation-level permissions are enforced
- Object access through operations are controlled and follow the principle of least privilege
I agree with the sentiment if we want to stay broad with access control and GraphQL, then that is already covered in access control. But there are some specific GQL access control factors that could be included in the ASVS in the API section.
Maybe I misinterpreted it, but I think it's more architecture requirement to say, that do not build access control to GraphQL, but use an extra business logic layer for that.
Ok, I understand this requirement is targeting how to implement but not necessarily what to implement. For example, the IDOR control (4.2.1) is directly relevant to REST access control. Should this standard call out basic access controls for GraphQL?
Isn't this true to all whether REST, GraphQL or other forms of APIs that do not build access control in the data layer authorization logic, but use an extra business logic layer for that?
I guess what I'm trying to say is if this is true, then we do not need a GraphQL-specific requirement for that unless that is really different in how GQL handles things than REST and other forms of APIs.
To me, it sounds like problem here is that with a REST API it is usually very clear where the trusted layer is where access control needs to be implemented.
With GraphQL, I think the concern is that there are a couple of layers and we are therefore being a little clearer about where that access control needs to be implemented.
Do you agree @craig-shony?
However, I don't think we need to talk about finer grained access control as that should be covered in a generic way in V4.
As a developer with experience in both REST and GraphQL, I agree that the authZ must be in the business layer in both situations. In principle the requirement is the same. But because the nature of GraphQL is more complex than REST and developers tend to underestimate this complexity, I support having an explicit point as 13.4.2
What do you think about it @craig-shony? Did @aholmis drive the points that explain this requirement?
If that's the case, I think we should keep the requirement as is. Though, I think we have to add this on L1 too as it currently covers L2 and L3, @tghosth, @elarlang?
In the current state, I get as much value as if you said "do GraphQL securely". If the goal is to "provide a basis for testing web application technical security controls and also provides developers with a list of requirements for secure development" than I think it's lacking. I'm open to being out-voted though.
I mean I think 13.2.1 is making a specific point, not just "do it securely".
Even here, "Delegate authorization logic to the business logic layer" is the summary: https://graphql.org/learn/authorization/
Thank you for clarifying, if the point of this item is to make sure you have a separate business logic layer that handles the authorization, then I would not consider that GraphQL specific.
I was reading the requirement as your business logic layer authorization should account for different access model that GraphQL enables, so I was hoping to see access control requirements appropriate to GraphQL.
Many folks allow clients to directly access GraphQL endpoints directly. In that case the access control needs to be integrated into GraphQL itself.
To force everyone to do access control "at the business logic level" for GraphQL use, shows a lack of understanding of how GraphQL is often implemented.
That's a good point "business logic level" is not really defined, I was going the assumption that having a centralized authorization layer (still within the GraphQL service) so there's one source of truth for access rules and consistent error handling would be check the box.
Having said that, I think this requirement should stay as is if we don't need to modify/remove it. Don't you agree?
Many folks allow clients to directly access GraphQL endpoints directly. In that case the access control needs to be integrated into GraphQL itself. To force everyone to do access control "at the business logic level" for GraphQL use, shows a lack of understanding of how GraphQL is often implemented.
I am afraid @jmanico that https://graphql.org/ disagrees with you :)
https://graphql.org/learn/authorization/
Exhibit A:
Exhibit B:
I agree with @csfreak92 that this requirement should probably stay as it is
Most implementations that I have seen do not have the ability to pass in a "fully-hydrated User object instead of an opaque token or API key"
Are we creating a requirement that GraphQL endpoints should not exposed to the client? I think Jim is correct in saying that this will make this requirement out-of-scope for many GraphQL implementations.
Are we creating a requirement that GraphQL endpoints should not exposed to the client?
That is what I tend to suggest. But do you think this is a reasonable requirement regarding the way folks realistically roll out GraphQL?
So my understanding was that the suggested mechanism is as follows:
flowchart LR
User((End User/
Client Appp))
GraphQL[GraphQL Layer]
Business["`Business Logic Layer (_**AuthZ applied here**_)`"]
Database[(Database)]
User -->|"Makes GraphQL queries to
pull data into the App"|GraphQL
GraphQL-->|Pulls data from|Business
Business-->|Reads data from|Database
Have I misunderstood?
I just suggest we set a new requirement that states that if you are going to expose graphQL endpoints directly to the client, then you need to implement fine grain access control within graphQL resolvers. I'm not sure if you consider this business logic layer or not, but that is where they belong IMO.
And Josh, I think it would be helpful if both of us stop using the term business logic for this discussion and be more specific about the location in the architecture we are referring to.
Also, when you're using GraphQL on the backend like a traditional database - the way you do authorization security is typically different than when exposing GraphQL to the client directly.
The fact that so many people expose graphQL directly to the client is why this discussion is so much more complicated than a traditional database architecture.
I found the following diagram on https://graphql.org on this specific page (I didn't see it when I wrote the diagram above): https://graphql.org/learn/thinking-in-graphs/#business-logic-layer
I think this is also alluding to what I have been saying that users access the interfaces at the top and they all access a common layer which enforces authorization and then that layer accesses the "persistence" layer, presumably the database.
In most cases I have seen, GraphQL is deliberately being exposed to the client as it provides a more intuitive data access interface for the client. As such, graphql.org's authorization page is explicitly saying not to check for authorization in resolvers because you might have multiple resolvers which pull the same data item and you would therefore need duplicated authorization checks.
Instead, it is saying to put the authorization into the function that pulls a specific type of data from the persistence source/database. The article itself calls this the "business logic" layer but whether we use that terminology or not, the idea is that the resolvers have to get the data from somewhere and potentially they are not pulling directly from the database.
For comparison, I looked at the documentation for Apollo Server which seems to be a popular framework for exposing a GraphQL interface.
Apollo's AuthN/AuthZ page initially seems less opinionated than graphql.org's authorization page. It explains how to do API level authorization for non-public APIs, then how to do authorization in resolvers.
However, it then continues to explain how to do authorization in data models where it says:
As always, we recommend moving the actual data fetching and transformation logic from your resolvers to data sources or model objects that each represent a concept from your application: User, Post, etc. This allows you to make your resolvers a thin routing layer, and put all of your business logic in one place.
@jmanico said the following:
I think that as we strictly evaluate ASVS requirements, it doesn't pass the test of "are you vulnerable if you don't do this". This is a way of doing AuthZ for GraphQL. It has a lot of advantages but it is not the only way.
I therefore think that maybe we need to do downgrade this to a recommendation instead of a requirement.
What do you think @jmanico @craig-shony?
I agree Josh. I can be very secure with GraphQL access control without this requirement.
I will merge this now to avoid conflicts but open to further discussion if anyone still wants.