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

Merged
merged 78 commits into from
Jan 16, 2025
Merged

Biscuit v5 #217

merged 78 commits into from
Jan 16, 2025

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 <[email protected]>
…220)

Context: eclipse-biscuit/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 <[email protected]>
Co-authored-by: Clément Delafargue <[email protected]>
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 <[email protected]>
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 <[email protected]>
* more explicit test assert

* update run limits
divarvel and others added 6 commits November 21, 2024 15:46
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 21 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.
This plays nicer with other libs like `clap`
ed25519 is used as the default algorithm in a lot of places
@divarvel divarvel merged commit e3eb17a into main Jan 16, 2025
2 of 4 checks passed
@divarvel divarvel deleted the v5 branch January 16, 2025 14:22
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