ory / keto

The most scalable and customizable permission server on the market. Fix your slow or broken permission system with Google's proven "Zanzibar" approach. Supports ACL, RBAC, and more. Written in Go, cloud native, headless, API-first. Available as a service on Ory Network and for self-hosters.
https://www.ory.sh/?utm_source=github&utm_medium=banner&utm_campaign=keto
Apache License 2.0
4.82k stars 348 forks source link

Slash (/) in role or policy id causes 404 error for GET and DELETE #140

Closed Ian-Butler-Novacoast closed 3 years ago

Ian-Butler-Novacoast commented 4 years ago

Describe the bug

When I try to GET or DELETE a role with a / in the string for the id, a 404 error response is returned. The same issue exists for policy ids.

Reproducing the bug

Steps to reproduce the behavior:

  1. Run docker run ....
  2. Create a role with / in the id for example: curl -X PUT \ http://localhost:32034/engines/acp/ory/exact/roles \ -H 'Accept: application/json' \ -H 'Accept-Encoding: gzip, deflate' \ -H 'Cache-Control: no-cache' \ -H 'Connection: keep-alive' \ -H 'Content-Length: 53' \ -H 'Content-Type: application/json' \ -H 'Host: localhost:32034' \ -H 'cache-control: no-cache' \ -d '{ "id": "some/group", "members": ["string"] }' and confirm 200 OK response
  3. Request details for the role with the request curl -X GET \ http://localhost:32034/engines/acp/ory/exact/roles/some%2Fgroup
  4. Delete role with the request curl -X DELETE \ http://localhost:32034/engines/acp/ory/exact/roles/some%2Fgroup

Result: 404 responses for the GET and DELETE responses, however, the role is included when listing all roles

Server logs

INFO[29052] started handling request                      method=PUT remote="192.168.65.3:55658" request=/engines/acp/ory/exact/roles
INFO[29052] completed handling request                    measure#keto.latency=5997225 method=PUT remote="192.168.65.3:55658" request=/engines/acp/ory/exact/roles status=200 text_status=OK took=5.997225ms
INFO[29107] started handling request                      method=GET remote="192.168.65.3:55658" request="/engines/acp/ory/exact/roles/some%2Fgroup"
INFO[29107] completed handling request                    measure#keto.latency=209943 method=GET remote="192.168.65.3:55658" request="/engines/acp/ory/exact/roles/some%2Fgroup" status=404 text_status="Not Found" took="209.943µs"
INFO[29209] started handling request                      method=DELETE remote="192.168.65.3:56136" request="/engines/acp/ory/exact/roles/some%2Fgroup"
INFO[29209] completed handling request                    measure#keto.latency=129919 method=DELETE remote="192.168.65.3:56136" request="/engines/acp/ory/exact/roles/some%2Fgroup" status=404 text_status="Not Found" took="129.919µs"

Server configuration

Expected behavior

GET and DELETE should handle url encoded identifiers, or an id with a slash should be prevented from being used to create a role.

Environment

aeneasr commented 4 years ago

Yeah, slashes aren't supported in ID names, we should probably reject those on POST/PUT.

dparker18 commented 4 years ago

We used the slash to include a path component at the end similar to what AWS does in their arn syntax. If it's rejected that will probably cause issues with IAM compatibility #59 .

What character ranges are supported?

aeneasr commented 4 years ago

Everything except a slash! Slashes are very dangerous, when you're using an ID like foo/../../bar for example, which can trick systems into a path rewrite attack.

Regarding AWS compatibility, we'll cross that bridge when we come to it. It will be a different API anyways and maybe we'll not rely on paths there for retrieval.

dparker18 commented 4 years ago

Gotcha, I'm an app pentester (Ian too) and totally understand the security concern. But I'd argue here it's a case of requiring the app to conform to an artificial limitation. And that slashes not only are perfectly valid here but also enable a granular and flexible ID scheme. With proper input validation of course.

For instance, how should we best implement an ID scheme in a multi tenant ticketing system where each tenant has their own organizational structure with multiple arbitrary levels? novacoast:ticketsystem:tenant123:ticket/department456/project789/folderXYZ/*

If we have a clean PR to support slash that touches minimal code and maintains security would you guys consider it?

aeneasr commented 4 years ago

If we have a clean PR to support slash that touches minimal code and maintains security would you guys consider it?

Of course :) Unfortunately I think this is an issue with httprouter iirc, but maybe I'm wrong.

aeneasr commented 3 years ago

I am closing this issue as our Google Zanzibar-based refactoring is scheduled to be released soon. Ory Keto up to version 0.5 will go in hibernation mode and receive only critical security patches.