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

Biscuit v5 #217

Open
wants to merge 72 commits into
base: main
Choose a base branch
from
Open

Biscuit v5 #217

wants to merge 72 commits into from

Conversation

Geal
Copy link
Contributor

@Geal Geal commented May 12, 2024

This PR holds the upcoming changes for the v3.3 format:

  • reject if
  • null
  • heterogeneous equals
  • closures
  • typeof
  • array / map
  • FFI

as well as related features on the crypto layer:

  • ecdsa
  • signature algorithm update

set the block version when using 3rd party blocks
@Geal Geal marked this pull request as draft May 12, 2024 13:56
divarvel and others added 9 commits May 12, 2024 16:04
* feat: add `reject if`

This acts like the opposite of `check if`: if there is a match, then authorization fails.

Using `reject if` raises the block version to 5

* fix: run rustfmt on datalog/mod.rs

The file contained trailing slashes that made rustfmt crash

---------

Co-authored-by: Geoffroy Couprie <contact@geoffroycouprie.com>
…220)

Context: biscuit-auth/biscuit#130

This introduces the `HeterogeneousEqual` and `HeterogeneousNotEqual` operations, which will not return an error when their operands have different types, contrary to the existing `Equal` and `NotEqual` operations.

**breaking change**: this does not change the execution of existing tokens, but changes the text representation of the language. `Equal` was `==` and is now `===`, `NotEqual` was `!=` and is now `!==`, `HeterogeneousEqual` is `==` and `HeterogeneousNotEqual` is `!=`

---------

Co-authored-by: Geoffroy Couprie <contact@geoffroycouprie.com>
Co-authored-by: Clément Delafargue <clement@delafargue.name>
Copy link

codspeed-hq bot commented May 26, 2024

CodSpeed Performance Report

Merging #217 will not alter performance

Comparing v5 (2c75eaf) with main (24f13e2)

Summary

✅ 12 untouched benchmarks

Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 75.38344% with 642 lines in your changes missing coverage. Please review.

Project coverage is 65.61%. Comparing base (4fc3c31) to head (6d55705).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
biscuit-auth/src/token/builder/block.rs 60.71% 77 Missing ⚠️
biscuit-auth/src/token/builder/term.rs 74.57% 75 Missing ⚠️
biscuit-parser/src/builder.rs 27.77% 65 Missing ⚠️
biscuit-auth/src/token/builder/rule.rs 75.23% 53 Missing ⚠️
biscuit-auth/src/crypto/ed25519.rs 54.32% 37 Missing ⚠️
biscuit-auth/src/crypto/mod.rs 82.08% 36 Missing ⚠️
biscuit-auth/src/datalog/expression.rs 84.07% 36 Missing ⚠️
biscuit-auth/src/crypto/p256.rs 55.26% 34 Missing ⚠️
biscuit-auth/src/token/builder/expression.rs 82.75% 30 Missing ⚠️
biscuit-auth/src/format/convert.rs 80.95% 28 Missing ⚠️
... and 19 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
+ Coverage   61.50%   65.61%   +4.10%     
==========================================
  Files          25       38      +13     
  Lines        5765     6995    +1230     
==========================================
+ Hits         3546     4590    +1044     
- Misses       2219     2405     +186     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Geal and others added 16 commits May 26, 2024 22:19
This introduces the closure operations to the Biscuit language, first with the `.all()` and `.any()` operations to add conditions on the elements of a set.

It is now possible to use expressions with the following format:
```
check if [1,2,3].all($p -> $p > 0);
check if [1,2,3].any($p -> $p > 2);
```

Co-authored-by: Geoffroy Couprie <contact@geoffroycouprie.com>
While the incrementing integer for version numbers is fine for the protobuf encoding, it has been a headache when communicating with other people.

A `major.minor` scheme seems better for understanding, and helps communicate the scope of changes a bit better.

There was a bug in version checks that relied on `BISCUIT_MAX_VERSION` to check the version for specific features. This does not play well with version upgrades.

So this commit fixes both issues:

- use explicit 3.x version number for datalog features
- use explicit version numbers for feature checks, instead of relying on the max version

It also improves a bit on some error messages that were a bit cryptic, by clearly specifying the version in which specific features were implemented.
Datalog parameters are managed lazily: upon building a rule, referenced parameter are collected in a map. Setting them only updates the map, and the actual AST is only updated as needed (such as in Display impls, or when converting to datalog data structures).

Expressions used to be linear, but closures introduced the possibility of recursive expressions, so the code inside closure bodies was ignored both when collecting parameters, and when applying them.

This caused an issue because some parts of the code assume that unbound parameters are handled beforehand, and panic upon encountering them.
…losures

fix: recursively collect and apply parameters in closures
In #231 the v3/4/5 naming was removed, in favor of v3.0/3.1/3.2/3.3.

A few functions were forgotten in this renaming.
feat: add `.type()` unary method
This adds support for the array and map types, supporting more structured datalog terms, that we can generate from JSON data and explore through datalog expressions. The map type allows integers strings and parameters as key. This tries to enforce that all array elements are of the same type, but this is not very strict at the moment, it does not look at lower levels of composite types.

**breaking changes**:
- in the Datalog language, sets will now be delimited
by '{' and '}' instead of '[' and ]'. Arrays are now delimited by '['
and ']'
- parameter names now need to start with a letter

---------

Co-authored-by: Clement Delafargue <clement.delafargue@outscale.com>
* more explicit test assert

* update run limits
divarvel and others added 14 commits November 19, 2024 21:47
Instead of storing strings directly in the ops, do as we do for everything else and use the symbol table.

This required duplicating `biscuit_parser::builder::Binary` and `Unary` in the `biscuit_auth::builder` module (which previously used the definitions from the `datalog` module directly). There is a lot of duplication between `biscuit_parser::builder` and `biscuit_auth::builder`, with a circular-ish dependency (biscuit_auth depends on biscuit parser, but code generated by the `ToTokens` impl blocks in biscuit parser depend on `biscuit_auth::builder`).
This ensures consistent evaluation of functions and removes the need of storing them in `RunLimits`
datalog foreign function interface prototype
This adds suport for ECDSA signatures over the secp256r1 curve, following the specification change at biscuit-auth/biscuit@ed1c53d. Key creation now takes an algorithm argument

Co-authored-by: Clément Delafargue <clement.delafargue.ext@outscale.com>
We want to progressively roll out signature v1, without breaking existing implementations. This means we can force signature v1 everywhere a recent library version would still be required:

- all v3.3+ datalog blocks
- all blocks signed with something other than ed25519
- all tokens already containing a block using signature v1

In addition to that, third-party blocks always require signature v1, which is the only breaking change we allow
This makes this sample usable with older implementations
force using signature v1 in more cases
Same as for `legacyPublicKeys`, it is now deprecated and should not be set. It is kept in the schema to allow implementations to make sure it is not set.
this exposes a new public method on `Biscuit` and `UnverifiedBiscuit` to get a block’s version, same as printing a block contents or getting its external key.

Exposing it on `UnverifiedBiscuit` will be useful for biscuit-cli
…d-party-request

remove previous_key from ThirdPartyBlockRequest
@divarvel divarvel marked this pull request as ready for review November 26, 2024 14:14
Geal and others added 15 commits November 27, 2024 11:22
This splits the builders to their own files
This adds an `AuthorizerBuilder` struct that is used to create an `Authorizer`. All of the mutable behaviour, like adding facts or executing Datalog rules is moved into the builder, while the authorizer is limited to read-only queries (still requiring self mutability to track execution time). This will solve some awkward behaviour where the authorizer had to run Datalog rules again when facts or rules were added, but it was not done consistently. The `AuthorizerBuilder` is compatible with snapshots, to store and reuse checks and policies. It has a `build` method taking a token as argument, and a `build_unauthenticated` for authorization without token.

The builder APIs are alo changing. Before, we had the following:

```rust
 let mut builder = Biscuit::builder();	
builder.add_fact(r"right("file1", "read")"#)?;
builder.add_fact(r"right("file2", "read")"#)?;
let token = builder.build()?;
```

Builders are now constructed like this:

```rust
let token = Biscuit::builder()
    .fact(r"right("file1", "read")"#)?
    .fact(r"right("file2", "read")"#)?
    .build()?;
````
according to the grammar, `{}` could either mean *empty set* or *empty map*. Due to how the parser was written, it was always an empty set, and there was no way to have an empty map literal.

This is an issue because empty maps would still be displayed as `{}`, and maps have a different api than sets, resulting in evaluation errors.

This commit introduces `{,}` as the empty set literal, while `{}` is the empty map. The goal here is to be consistent with JSON, the reason why we chose the current syntax for arrays and maps.
- Algorithm is used by KeyPair::new()
- BiscuitBuilder, BlockBuilder, AuthorizerBuilder are used a lot and have `builder` in their names, so a top-level export makes sense
This has been the default behaviour since biscuit 2. Taking the algorithm as a parameter is a breaking change which is IMO not necessary. This will make the update a little bit less painful for consumers.

Also, I don’t know the consensus on having `Default::default()` not return the same value every time, but I find it a bit misleading.
This allows taking a snapshot of the authorizer even if the datalog evaluation fails.
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 this pull request may close these issues.

3 participants