[BREAKING] Abstract biscuit-auth over crypto implementation

[saoirse-a]

This adds traits to the biscuit-auth public interface for cryptographic operations and abstracts the Biscuit and related types over those traits.

The traits added are Sign, Verify, SerializePublicKey and SerializePrivateKey. Together, these represent the necessary behavior for public and private keys, distinguishing those which can be serialised into the token and those which can't:

pub trait Sign {
    type PublicKey: Verify;

    fn sign(&self, data: &[u8]) -> Result<Signature, error::Format>;
    fn public(&self) -> Self::PublicKey;
    fn algorithm(&self) -> Algorithm;
}

pub trait Verify {
    fn verify_signature(
        &self,
        data: &[u8],
        signature: &Signature,
    ) -> Result<(), error::Format>;
    fn algorithm(&self) -> Algorithm;
}

pub trait SerializePrivateKey: Sign<PublicKey: SerializePublicKey> + Clone + Sized {
    fn new_with_rng<R: RngCore + CryptoRng>(algorithm: Algorithm, rng: &mut R) -> Self;
    fn from_bytes_and_algorithm(algorithm: Algorithm, bytes: &[u8]) -> Result<Self, error::Format>;
    fn to_bytes(&self) -> Zeroizing<Vec<u8>>;
}

pub trait SerializePublicKey: Verify + Clone + PartialEq + Sized {
    fn from_bytes_and_algorithm(algorithm: Algorithm, bytes: &[u8]) -> Result<Self, error::Format>;
    fn to_bytes(&self) -> Vec<u8>;
}

The Biscuit type is parameterised by a SerializePrivateKey, which is the type of the key contain in its proof. All of the public keys in the token are the associated serialisable public key type for that private key.

The issuer key and third party signing keys can be different types from the key contained in the token; for the issuer neither the private key nor the public key need be serialisable, for the third party key only the public key needs to be serialisable because it appears in the token.

The existing PublicKey and PrivateKey types implement these traits; no other implementations are added directly to biscuit-auth with this PR (we could either add implementations for other crypto libraries like aws-lc-rs, maybe feature gated, or just require users to write their own implementations).

To support this change, two significant refactors were performed:

1. The Keypair type was eliminated. This was effectively redundant with the PrivateKey type, removing it made the set of traits required simpler.
2. The PublicKeys table used in authorization execution now stores an inert representation of the public key instead of the PublicKey type, because it doesn't need to verify signatures, just compare keys for equality. This avoids more abstraction points over crypto types in the Datalog code.

[saoirse-a]

I think there is more work to be done to fully integrate the traits:

1. Some APIs in the builders aren't generic over crypto providers still.
2. Some of the Datalog auth code (like the Authorizer itself) is abstract over key types because it stores more data from the token than it really needs to.

Before I go through all of that with a fine tooth comb I wanted to get confirmation that this is the right direction.

[divarvel]

I like this approach as it neatly handles both KMS and pluggable crypto providers.
It also opens the door to unifying Biscuit and UnverifiedBiscuit (I think)

[saoirse-a]

Updated, I think this is ready now.

[divarvel]

This generally looks good to me. got a few questions and nit-level comments.

[wbourne0]

I don't fully understand what the point of this is, since it doesn't allow for HSMs or TPMs. The private keys are still required to be serializable. Which means we must have the private material outside of it.

I don't know the codebase that well (first time looking at it today) but I don't think that having a custom public key trait makes much sense here, as there's currently only 2 algorithms.

I haven't looked through all of the library yet, but I'm also not convinced that this needs to be a breaking change.

I think this should be started off by just add a Signer trait and allowing issuance with root keys (a simple switch to &impl Signer for the root key reference, as long as the KeyPair type impl this trait, all previous calls should be compatible).

[saoirse-a]

I don't fully understand what the point of this is, since it doesn't allow for HSMs or TPMs. The private keys are still required to be serializable. Which means we must have the private material outside of it.

The original goal of this work was to support other crypto implementations (not algorithms) if users want to use a different library from p256/ed25519-dalek for either performance or security assurance reasons. That's what your change in #340 doesn't fully support; you could swap the issuer key but all internal signature verifications have to route through p256/ed255190-dalek. p256 in particular is on the order of 5x slower than some other implementations (see benchmarks in #320).

But being forward compatible with HSMs was definitely also a goal of this change, which is why we went this route instead of the route in #320. You spotted that the builder APIs as expressed actually aren't compatible with that, which was a big oversight on my part. To fix that mistake, BiscuitBuilder now distinguishes between an RK parameter (the root key) and the K parameter of the biscuit, which is the proof key.

This introduced a new issue that the builder APIs don't have an obvious way to infer the K parameter if its not the same as the root key, since users are generating that key from an RNG and never directly have a value of K. I didn't want users to have to turbo fish in a type for K if they're using the standard implementations, so I gave BiscuitBuilder a phantom K parameter which like in Biscuit is defaulted to PrivateKey.

BiscuitBuilder::new constructs a BiscuitBuilder<PrivateKey> so users who want to use the default implementation don't have to specify anything. Even if they're using a custom RK like one backed by an HSM, that will be inferred from usage. For users who want to select their own K because they don't want to use the provided types, the implementation of Default allows any K, so they would write let builder: BiscuitBuilder<MyPrivateKeyType> = BiscuitBuilder::default(). This is analogous to how HashMap::new and HashMap::default work with regard to the hasher type and it seems like the best solution.

---

I've also renamed the public_keys::PublicKey to PublicKeyData to make it less confusing than having two types called PublicKey. I've made its constructors public and added algorithm to its API, since it is ultimately a part of the public API via the scope DSL. With some patches to test cases it now builds and passes.

The changelog is also more complete and I've responded to @divarvel's nits. Other than the question of trait naming, if we're happy with this direction I think this PR is ready to merge.

[saoirse-a]

One more set of related changes: I wanted to make it so you could pass any public key type (including your own custom ones) to the Datalog macros. In the process I realised that ToAnyParam and AnyParam aren't actually doing anything anymore: macros already distinguish between scopes and terms. This let me delete a good amount of code from the Datalog builders; now for terms you need to pass something that implements Into<Term> and for scopes you need to pass something that implements Into<PublicKeyData>. biscuit-quote also changed to generate calls to set_lenient methods directly.

[divarvel]

One more set of related changes: I wanted to make it so you could pass any public key type (including your own custom ones) to the Datalog macros. In the process I realised that ToAnyParam and AnyParam aren't actually doing anything anymore: macros already distinguish between scopes and terms. This let me delete a good amount of code from the Datalog builders; now for terms you need to pass something that implements Into<Term> and for scopes you need to pass something that implements Into<PublicKeyData>. biscuit-quote also changed to generate calls to set_lenient methods directly.

Thanks! I actually worked on this in #314 but it was stalled because at that time it was a breaking change.

[divarvel]

Thanks for the effort @saoirse-a, and thanks for the valuable feedback @wbourne0

I’ll merge as is, i’m still open to naming improvements before cutting a release, but i think this is fine as it is right now.

This solves two huge pain points so I’m glad to see this move forward.

[divarvel]

Could you have a look at the failing coverage test?

[wbourne0]

The original goal of this work was to support other crypto implementations (not algorithms) if users want to use a different library from p256/ed25519-dalek for either performance or security assurance reasons. That's what your change in #340 doesn't fully support; you could swap the issuer key but all internal signature verifications have to route through p256/ed255190-dalek. p256 in particular is on the order of 5x slower than some other implementations (see benchmarks in #320).

Yes, #340 is intended to be a gradual change with no breaking changes, I was intentionally keeping it minimal to avoid locking into specific architectural decisions. I only added From impls for those two types because it was the most minimal way I could think of to somewhat validate that the signatures are actually the correct format (at a type level / without potentially adding overhead).

I also don't see why this needs to be a breaking change again, especially if you're setting default parameters. Adding traits doesn't need to be a breaking change; though removing them usually is.

As for supporting other implementations of ed25519 and p256, personally I'm of the opinion that other options should be implemented via feature flags and optional dependencies (with current config under the default feature). I think in general with token formats you want to keep the user as far away from it as possible. It feels more reliable to have a first party integration than it does to provide your own.

This may be addressed but I'm also a little concerned about this implementation if a biscuit has mixed key types; I wonder if the ideal type for the validator type should enforce that validators for both be written (e.g. with associated types).

Anyhow, this is a large change to add all at once (and I'm not super familiar with the codebase in general) so I have a hard time visually validating it. I don't own this repo so do as you will, but I do think it'd be good to see some e2e examples of using this before merging it. It's probably one of the best ways to test a change like this.