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

feat: Add Access Control Policy #2338

Merged

Conversation

shahzadlone
Copy link
Member

@shahzadlone shahzadlone commented Feb 23, 2024

Relevant issue(s)

Part of Epic #1738
Resolves #2019
Resolves #2020
Resolves #2228

Description

  • Rendered ACP Doc
  • Introduces an ACP Interface that needs to be satisfied for any type of access control module.
  • This PR implements a local embedded access control system for DefraDB by implementing the ACP interface.
  • There should be no remote calls (everything introduced here is local).
  • There should be no blocking of any kind, no async stuff was introduced.
  • Documents now have the following three states, from a requester's POV:
    • Public document (always accessible).
    • Accessible document (accessible if requested by the owner)
    • Inaccessible/private document (can't access if requested by non-owner or non-identity request)
  • Ability to add permissioned schema.
  • Added permissioned schema examples.
  • Ability to add policy.
  • Added policy examples (JSON and YAML).
  • Ability to specify simple identity addresses on requests.
  • Mocks and Documents Generated (old and new).

Demo

AddPolicy{
	Creator: "source1zzg43wdrhmmk89z3pmejwete2kkd4a3vn7w969",
	Policy: `
		description: a test policy which marks a collection in a database as a resource

		actor:
		  name: actor

		resources:
		  users:
			permissions:
			  read:
				expr: owner + reader
			  write:
				expr: owner

			relations:
			  owner:
				types:
				  - actor
			  reader:
				types:
				  - actor
			  admin:
				manages:
				  - reader
				types:
				  - actor
	`,
	ExpectedPolicyID: "53980e762616fcffbe76307995895e862f87ef3f21d509325d1dc772a770b001",
}

SchemaUpdate{
	Schema: `
		type Users @policy(id: "53980e762616fcffbe76307995895e862f87ef3f21d509325d1dc772a770b001", resource: "users") {
			name: String
			age: Int
		}
	`,
}

CreateDoc{
	Identity: "source1zzg43wdrhmmk89z3pmejwete2kkd4a3vn7w969",
	Doc: `{"name": "John", "age": 27 }`,
}

Request{
	Identity: "source1zzg43wdrhmmk89z3pmejwete2kkd4a3vn7w969",
	Request: `
		query {
			Users {
				_docID
				name
				age
			}
		}
	`,
	Results: []map[string]any{
		{
			"_docID": "bae-88b63198-7d38-5714-a9ff-21ba46374fd1",
			"name":   "John",
			"age":    int64(27),
		},
	},
}

// The Following Requests Don't Have Access

Request{
	Request: `
		query {
			Users {
				_docID
				name
				age
			}
		}
	`,
	Results: []map[string]any{},
}

Request{
	Identity: "cosmos1x25hhksxhu86r45hqwk28dd70qzux3262hdrll".
	Request: `
		query {
			Users {
				_docID
				name
				age
			}
		}
	`,
	Results: []map[string]any{},
}

Features

  • In-memory and filebased acp module.
  • Registering of a document with ACP Module on creation.
  • Add policy command HTTP & CLI.
  • Detect Policy Marshal Format on adding.
  • Add permissioned schema.
  • Reject schema on validation failure.
  • Accept schema on validation success.
  • Permissioned Fetcher
  • Specify Identity on doc create.
  • Specify Identity on doc delete.
  • Specify Identity on doc update.
  • Specify Identity on request.

Things That Are In Scope Of This PR:

  • All previous features should behave as they were.
  • There should be no performance costs/hits to any previous functionality.
  • Access Control would only be registered for a document on a collection if:
    • Have ACP Module initialized / avaialable programatically
    • The creation request had an Indentity attached.
    • The collection has a permission on it (i.e. collection is permissioned).
  • Access Controled Read/Write, after Creation and Registering:
    • If there is no ACP module, have access to all documents (as if acp is turned off).
    • If there is no Indentity can only operate on public documents (unregistered documents with acp).
    • If there is Permissioned Collection, Identity and ACPModule then operate on access controled document.
  • Cosmos Identity Addresses.
  • Adding of a policy (CLI & HTTP), Tests: ./tests/integration/acp/add_policy
  • Validation of Linked Policy Resource DPI on Schema Add:
    • Accepting of a permissioned schema on Valid DPI Resource
    • Rejecting of a permissioned schema on Invalid DPI Resource
    • Tests for both here: ./tests/integration/acp/schema/add_dpi

Things That Are Out Of Scope Of This PR:

  • Full Fledged Indentity with Authentication
  • Using ACP with any other feature is not supported.
    • P2P
    • Secondary Indexes
    • Type Joins
    • Aggregates
    • Backup & Recover
    • Views
    • Lens

De-scope to after merge.

  • Update tracking issue with road-map and priority of next tasks.
  • Add simple identity generate utility command
  • Add simple identity validation utility command
  • Fix the identity validation panic

For Reviewers:

Recommendations:

Commit Priorities:

  • Commits with label PR(-) are unrelated/irrelevant to this PR, can be ignored.
  • Commits with label PR(ACP) are most important to review as they are specific to ACP implementation.
  • Commits with label PR(IDENTITY) are also important as they are specific to Indentity implementation.
  • Commits with label PR(*-TEST) are test related commits that should be looked at.
  • Commits with label PR(ACP-*) are assisting ACP commits (medium priority).
  • Commits with label PR(IDENTITY-*) are assisting ACP commits (medium priority).
  • Commits with label PR(WIP) Should not exist before merge (work in progress commits).
  • Commits with label PR(DROP) Temporary commits that will be dropped before the merge.

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

  • There are add policy tests
  • There are adding of permissioned schema (with dpi) tests.
  • There are end-to-end tests with doc creation and read using identity.

Specify the platform(s) on which this was tested:

  • Manjaro WSL2

@shahzadlone shahzadlone mentioned this pull request Feb 23, 2024
5 tasks
@shahzadlone
Copy link
Member Author

shahzadlone commented Feb 23, 2024

@AndrewSisley

Context: #2337 (comment)

Do you see the location code flow around the calling of this function to remain the same when that time comes? Or is the code flow here likely to be very temporary? And do you have a rough idea of what that new code structure might look like?

Sorry I should have been more specific, the functionality you are enquiring about is the Check call of the ACP. I haven't planned or looking at the Check being async, I think this will remain synchronous, however for the Registeringmechanics, me and John discussed recently that it should be made async (it would not block though and use a retry/cache mechanic incase a registering fails). This prompts an interesting question, what if a document is being accessed which is not public, but is in-process-to-register but the async call hasn't succeeded in regestrting with acp module. In that case the fetch would still return the document as long as the owner is the one making the request (we will store this info within the registering event queue). However all the async register stuff is out of scope for v0 (this PR).

The thought of a blocking async call per document inside the fetcher is not a happy one.

There shouldn't and will not be any blocking. In fact once bit set index checker is implemented it would improve even the current flow such that we would not have to make Check call for every document (instead can do it in a batch).

@shahzadlone shahzadlone marked this pull request as draft February 23, 2024 20:05
@shahzadlone shahzadlone self-assigned this Feb 25, 2024
@shahzadlone shahzadlone added the feature New feature or request label Feb 25, 2024
@shahzadlone shahzadlone added this to the DefraDB v0.10 milestone Feb 25, 2024
@shahzadlone shahzadlone force-pushed the lone/acp-sourcehub-module branch from 5537879 to 7830212 Compare February 25, 2024 13:28
Copy link
Contributor

@islamaliev islamaliev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far. Added some minor change requeust and suggestions.

Note done with review yet though, half through

acp/acp.go Outdated Show resolved Hide resolved
acp/acp.go Outdated Show resolved Hide resolved
acp/acp.go Outdated Show resolved Hide resolved
acp/acp.go Outdated Show resolved Hide resolved
acp/acp.go Outdated Show resolved Hide resolved
acp/dpi.go Outdated Show resolved Hide resolved
client/collection.go Outdated Show resolved Hide resolved
client/db.go Outdated Show resolved Hide resolved
client/index.go Outdated Show resolved Hide resolved
db/description/collection.go Outdated Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/acp-sourcehub-module branch 9 times, most recently from 11202f4 to 8d0979e Compare February 29, 2024 05:58
@shahzadlone
Copy link
Member Author

Looks good so far. Added some minor change requeust and suggestions.

Note done with review yet though, half through

Hey buddy @islamaliev thanks for the comments, I realized you commented before PR was ready for lower-level review. I think most of your suggestions are resolved in the latest push. The PR should now be ready to review if you want to take another look. I will mark the comments of yours as resolved that were done.

@shahzadlone shahzadlone requested a review from a team February 29, 2024 14:00
@shahzadlone shahzadlone added the area/acp Related to the acp (access control) system label Feb 29, 2024
@shahzadlone shahzadlone force-pushed the lone/acp-sourcehub-module branch from 8d0979e to db1be5b Compare February 29, 2024 15:59
@AndrewSisley
Copy link
Contributor

AndrewSisley commented Feb 29, 2024

Using ACP with any other feature is not supported.

  • P2P
  • Secondary Indexes
  • Type Joins
  • Aggregates
  • Views
  • Lens

@shahzadlone Can you please explain this? Particularly as to whether or not these allow bypassing of the permissions and access to otherwise restricted data. For example does P2P allow mutation of protected docs? Does including a doc in a join allow read access to unreadable data? If you include an aggregate in a query do you gain access to restricted docs in the rest of the query (example below):

Users {
  _count(_group)// doesn't matter
  name // can you now read the names of docs you wouldn't otherwise be able to query?
}

@shahzadlone
Copy link
Member Author

shahzadlone commented Feb 29, 2024

@AndrewSisley

Using ACP with any other feature is not supported.

  • P2P
  • Secondary Indexes
  • Type Joins
  • Aggregates
  • Views
  • Lens

@shahzadlone Can you please explain this? Particularly as to whether or not these allow bypassing of the permissions and access to otherwise restricted data. For example does P2P allow mutation of protected docs? Does including a doc in a join allow read access to unreadable data? If you include an aggregate in a query do you gain access to restricted docs in the rest of the query (example below):

Users {
  _count(_group)// doesn't matter
  name // can you now read the names of docs you wouldn't otherwise be able to query?
}

The only bypassing that happens is when you disable acp (i.e. make acp module unavailable). Other than that none of these should let you bypass, unless anyone of these gets something funky and fetches documents in a raw custom manner from the store without the fetcher logic.

I am just noting them as unsupported (even if they may or may not work) as there are no tests for them and I haven't confidently tested every edge case of theirs to mark them as "completed".

Note: Moreover, all features should work as they did before (when used without the acp).

LMK if that clarifies or you need any other clarification.

@AndrewSisley
Copy link
Contributor

question: In this PR identity is not verified? Actors are free to claim to be whoever they like and there are no checks as to whether they are that actor or not (e.g. password)? Meaning if person A knows person B's identity they can act as person B?

@AndrewSisley
Copy link
Contributor

question: In this PR is there a way to query as to who owns a document?

@shahzadlone
Copy link
Member Author

shahzadlone commented Feb 29, 2024

@AndrewSisley

question: In this PR identity is not verified? Actors are free to claim to be whoever they like and there are no checks as to whether they are that actor or not (e.g. password)? Meaning if person A knows person B's identity they can act as person B?

Note: The simple identity tests aren't pushed yet, and the removal of the hard coded values will both be pushed end of day today or tomorrow.

question: In this PR is there a way to query as to who owns a document?

  • No.

examples/schema/permissioned/book.graphql Show resolved Hide resolved
acp/DPI.md Outdated Show resolved Hide resolved
acp/DPI.md Outdated Show resolved Hide resolved
acp/DPI.md Outdated Show resolved Hide resolved
acp/DPI.md Outdated Show resolved Hide resolved
acp/TERMINOLOGY.md Outdated Show resolved Hide resolved
acp/DPI.md Outdated Show resolved Hide resolved
@shahzadlone shahzadlone force-pushed the lone/acp-sourcehub-module branch from db1be5b to ca197a4 Compare February 29, 2024 18:48
Note: These tests are responsible for ensuring validation occurs
properly when a schema is linked to a resource and policyID, ensure that
we only accepting the schema if the policyID is linked to a valid DPI
or atleast the resource that is linked is DPI compliant, otherwise the
schema MUST be rejected.
(1) Update ACPModule Method and then,
(2) Add AddPolicy Method on the Store Interface

These are both important as we add both the CLI and HTTP clients in the
next few commits. This commit is mostly a prep for adding the clients.
@shahzadlone shahzadlone force-pushed the lone/acp-sourcehub-module branch from 1f6a275 to 83a1ae4 Compare April 3, 2024 05:11
It can seem as if the identity package is not doing much right now (and most of
it can be achieved through `Option`), but my authentication identity
implementation will very nicely build on top of this.

I also prefer the `NoIdentity` variable as it quickly shows me all
references where and empty identity is specified.
Implement ACP for getting document ids as well.
Only when the collection specified has a policy, or if the default
selection of all collections is implied then if any collection has a
policy we show a user friendly error.
@shahzadlone shahzadlone force-pushed the lone/acp-sourcehub-module branch from 83a1ae4 to 2a5e7f5 Compare April 3, 2024 05:17
@shahzadlone
Copy link
Member Author

Excited for the overall coverage bump 😄
image

@shahzadlone shahzadlone merged commit a46c1fa into sourcenetwork:develop Apr 3, 2024
31 checks passed
@shahzadlone shahzadlone deleted the lone/acp-sourcehub-module branch April 3, 2024 05:40
@islamaliev
Copy link
Contributor

bug bash note: upon requesting documents with invalid identity assumes there is no identity:

defradb client collection docIDs -i blah

returns a list of public documents.
Shouldn't we inform the user that it's invalid?

@islamaliev
Copy link
Contributor

bug bash result:
tested obvious scenarios for ACP inspired by integration tests and some not so obvious ones. Also tested some out-of-scope features: relations, _avg and _count.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/acp Related to the acp (access control) system feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InMemory ACP Support ACP - Embed Zanzi ACP - Defra Policy Interface (DPI) Consumption
6 participants