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

Update default symbol table as last edit to v2 #94

Closed
clementd-fretlink opened this issue Mar 12, 2022 · 8 comments · Fixed by #96
Closed

Update default symbol table as last edit to v2 #94

clementd-fretlink opened this issue Mar 12, 2022 · 8 comments · Fixed by #96

Comments

@clementd-fretlink
Copy link
Contributor

clementd-fretlink commented Mar 12, 2022

tl;dr:

Set the default symbol table to:

  • read
  • write
  • resource
  • operation
  • right
  • time
  • role
  • owner
  • tenant
  • namespace
  • user
  • team
  • service
  • admin
  • email
  • group
  • member
  • ip_address
  • client
  • client_ip
  • domain
  • path
  • version
  • cluster
  • node
  • hostname
  • nonce

Context

The following default symbol table is defined in the spec:

  • authority
  • ambient
  • resource
  • operation
  • right
  • time
  • revocation_id

Unfortunately, the rust implementation (used by python and node), and the other implementations copied from it (java, go, …) use a different table (current_time instead of time). Only the haskell implementation uses the same table as defined by the spec.
In addition to the implementations, time is used pervasively for TTL checks (in documentation, and in the TTL helpers provided by the CLI tool).

So we have two choices here:

  • align the spec (and the haskell impl) to what other implementations do, update the rust CLI tool, the docs, the samples
  • fix the rust, java, go implementations to align with the spec, considering that v2 biscuits have not been deployed in the wild yet

I think we have a small window where we can fix the implementations, before v2 tokens start to be deployed. That would also let us adapt the default symbol table to v2 use, something we forgot to do when working on v2 (authority and ambient symbols are not used anymore, for instance). I'm not sure revocation_id is useful as well, since we typically want to handle revocation outside datalog.

Another thing that would be nice, is to offset the indices of interned strings in tokens, to provide future us with more flexibility wrt default symbols.

Given the current status of v2 use, I think we have a window to ship a few improvements and avoid keeping cruft around, but if that's too late, it's perfectly ok to just align the spec with what's already done.

Related PRs

@Geal
Copy link
Contributor

Geal commented Mar 15, 2022

we will also update the symbol table to remove the now unused authority and ambient, to replace them with the more common read and write

@divarvel
Copy link
Collaborator

i suggest replacing revocation_id with role as well.

Revocation ids are typically handled outside datalog, and have little value being embedded in the token anyway.

divarvel added a commit to divarvel/biscuit-java that referenced this issue Mar 16, 2022
@KannarFr
Copy link

KannarFr commented Mar 17, 2022

I suggest adding owner, tenant, namespace, WDYT?

@Geal
Copy link
Contributor

Geal commented Mar 17, 2022

user, team, service?

@divarvel
Copy link
Collaborator

divarvel commented Mar 17, 2022

Admin, email, group, member
ip_address, domain, path, version

@Geal
Copy link
Contributor

Geal commented Mar 19, 2022

I added samples with more symbols in #96 69b2981

next commit in that PR will be about reserving some indexes to grow the default symbol table in the future. Symbols added by a block will start at the 1024 index

@Geal
Copy link
Contributor

Geal commented Mar 20, 2022

48d96aa this commit implements the offsets.
I added query to the default symbols, since it is used by checks as default head name

@divarvel
Copy link
Collaborator

divarvel commented Mar 21, 2022

That's a good addition in general.

is it actually carried by the token itself? The haskell implementation (and the protobuf AST for that matter) don't require heads for checks / policies queries

@Geal Geal closed this as completed in #96 Mar 21, 2022
KannarFr pushed a commit to divarvel/biscuit-java that referenced this issue Mar 22, 2022
KannarFr pushed a commit to divarvel/biscuit-java that referenced this issue Mar 22, 2022
KannarFr pushed a commit to divarvel/biscuit-java that referenced this issue Mar 22, 2022
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.

4 participants