Decoupled crypto backends #410

sidrubs · 2024-10-03T18:54:03Z

Introduction

This is a POC at abstracting the signing and verifying algorithms behind the JwtSigner and JwtVerifier traits whilst not breaking the encode and decode public interfaces that people know and love.

This allows any crypto backend to be used, as long as it implements the JwtSigner and JwtVerifier traits. Allowing developers to provide their own backend dependent on their use case. This should also enable more flexibility for the jsonwebtoken crate to prepackage different backends (e.g. ring, aws_lc_rs, and RustCrypto).

Implementation

JwtEncoderand JwtDecoder builder style clients were added to the public API allow the developer to use the included algorithms or provide their own.

The original encode and decode public interfaces call this builder under-the-hood with a factory function choosing the correct algorithm backend.

**Note: ** This POC only implements the HMAC family of algorithms as I didn't want to spend a bunch of time on something that was not going to be accepted.

Advantages

The builder-style API provides compile time checks to the algorithm and secret's compatibility
A developer is able to provide their own backend so does not need to rely on the jsonwebtoken crate maintainers to add something for them.
Allows the crate to be flexible, thus hopefully reduce fragmentation in the ecosystem.

Disadvantages

I have tried to make this backwards compatible, but this introduces some little idiosyncrasies, especially on the validation side (making use of the original Validation struct). Everything works, it would just seem a little weird to someone looking at parts of the codebase without context.

Wrapping Up

I understand that this a a pretty big change architecturally to the jsonwebtoken crate, so fully understand if you don't want to go this route. I do feel, however, that it makes the project more flexible and maintainable going forward.

Any suggestions are welcome.

Keats

I like the look of it but would like both RustCrypto and aws-lc support to make sure it works well.
I'm not too sure about the builders but I guess we can discuss that later

Keats · 2024-10-03T20:12:52Z

src/crypto/hmac.rs

-        self.clone().verify_slice(signature).is_ok()
+impl JwtVerifier for Hs512 {
+    fn algorithm(&self) -> Algorithm {
+        Algorithm::HS512


Looks like the kind of code that could be simplified with macros

For repeated things that that, sure it could 👍

I feel that we are only repeating it a couple times so I don't believe the complexity of a macro is worth it. I also find it easier to read normal Rust code. But maybe that is just me 🤷

Keats · 2024-10-03T20:14:39Z

src/decoding.rs

-            validate(decoded_claims.deserialize()?, validation)?;
-
-            Ok(TokenData { header, claims })
+    // The decoding step is unfortunately a little clunky but that seems to be how I need to do it to fit it into the `decode` function


how would it look like without that constraint?

Hmm, I think that comment doesn't really apply anymore (I should have removed it). There was a stage that I was less sure what was going on so I had a bit of a mess in there 🤦

sidrubs · 2024-10-03T22:45:41Z

I like the look of it but would like both RustCrypto and aws-lc support to make sure it works well. I'm not too sure about the builders but I guess we can discuss that later

Oh yeah sure thing. I'll add aws_lc_rs JwtSigner and JwtVerifier trait implementations (selected by a feature flag).

With regards to the builder, here was my thought process:

The JwtSigner and JwtVerifier traits have an algorithm method so that we know which algorithm the JwtSigner supports. I felt that most people will only ever use the a default header (with the correct algorithm), so why not automatically set it. But to be backwards compatible with the current encode function we would need to be able to set a fully custom header if needs be.

We could avoid the builder pattern by creating a function like the following and calling it from the current encode function (and initializing the signing_provider based on the header and the key). This however would not check the algorithm and secret at compile time, but that is the current behavior, so no problem.

Would this work better for you?

pub fn generic_encode<S: JwtSigner, T: Serialize>(
    signing_provider: S,
    header: &Header,
    claims: T,
    key: &EncodingKey
) -> Result<String> {
    ...
}

Keats · 2024-10-04T12:13:54Z

I think we can have both the builder and the function?

sidrubs · 2024-10-05T12:11:40Z

I think we can have both the builder and the function?

I just want to understand correctly as there are two functions I have talked about and I feel that we might be talking past each other.

There are the original encode and decode functions that are currently part of the public API in the jsonwebtoken crate. In my POC, I kept these function signatures as they were (so as not to break current the current public API), but they actually called the builder under-the-hood.
The second function came about when you mentioned that maybe the builder might not be ideal. So instead of using a builder, I suggested that we could use a function that allowed us to make use of arbitrary crypto backends. This would be called under-the-hood of the original encode and decode functions.

I both cases I was visualizing that we keep the same functionality of the original functions [1], but the internal business logic of these functions would either be provided by a builder or a function [2] style interface. So in addition to the original functions [1], we would also expose a method that an arbitrary backend could be used (this would either be a builder or another function).

Does this clear things up?

sidrubs · 2024-10-05T13:54:41Z

I have added an aws-lc-rs implementation behind a feature flag (again, only HMAC for now until we agree on a way going forward).

Currently there are rust_crypto and aws_lc_rs feature flags (more can be added easily). They will cause a descriptive compile error if enabled at the same time, but I can change this behavior to a run-time selection if needed.

rust_crypto is the default so can be tested with the following.

cargo test

aws_lc_rs needs to be enabled, so can be tested with the following.

cargo test --no-default-features --features aws_lc_rs

Keats · 2024-10-06T19:39:36Z

I both cases I was visualizing that we keep the same functionality of the original functions [1], but the internal business logic of these functions would either be provided by a builder or a function [2] style interface. So in addition to the original functions [1], we would also expose a method that an arbitrary backend could be used (this would either be a builder or another function).
Does this clear things up?

I think that's fine, as long as we can keep the keys in once_cell/lazy_static and use them without having to clone anything

Keats

I like where it's going

Keats · 2024-10-06T19:52:39Z

src/decoding.rs

+/// ```
+pub struct JwtDecoder {
+    verifying_provider: Box<dyn JwtVerifier>,
+    validation: Validation,


Should it be Option<Validator>? The default Validation is probably not useful for a lot of cases so might as well not have one

I am currently reusing a lot of the current validation logic that makes use of fields in the Validation struct. So in the initializer for the JwtDecoder it is creating a "default" Validation struct based on the algorithm of the verifying_provider. This is then used for the validation logic. If we were to use an Option<Validator> we would still need to provide default values to use somewhere in the code.

If someone wants to provide custom validation logic they can call the with_validation builder style method to replace the "default" validation that is generated in the initializer.

If one wanted to clean things up one could flatten the Validation fields and methods into the JwtDecoder, but because we want to keep the original decode function working the Validation code needs to be there, so might as well make use of it.

We should have the user pass it explicitly to the constructor then, implicit default validation is not good.

I think I should replace the builder with a function (see here). This would remove these smaller complexities.

Keats · 2024-10-06T19:57:22Z

src/encoding.rs

+    pub fn from_boxed_signer(signing_provider: Box<dyn JwtSigner>) -> Self {
+        // Determine a default header
+        let mut header = Header::new(signing_provider.algorithm());
+        header.typ = Some("JWT".to_owned());


That's the default I believe?

Yes, you are correct. I'll fix that up.

sidrubs · 2024-10-07T23:53:13Z

It seems that this POC might be used. So I'll go ahead and add the rest of the algorithms, keeping in mind some of the comments above, and get everything up to scratch to pass CI.

Unless there are any other concerns?

Keats · 2024-10-10T11:14:30Z

Go ahead, we can play with the DX once it's implemented. Maybe use macros to implement them so it's easy to make changes if needed rather than copy/paste

sidrubs · 2024-10-20T21:31:01Z

I have converted the builder to _encode and _decode functions that are called by the original public facing encode and decode functions. I think it does simplify things a bit.

I am about half done adding the RSA algorithms functionality.

I did however recently notice that the original signand verify functions in the crypto module are pub. The "new" trait based architecture does not make use of them, should I recreate their functionality using the traits based approach so that we don't break the current public facing API? Or would you envision this being a major version bump and removing those functions?

Keats · 2024-10-21T10:14:16Z

They are used by some people who only care about the crypto primitives so I would re-implement them and keep them pub

Keats · 2024-12-10T12:32:09Z

Hi @sidrubs will you have time to finish that PR?

sidrubs · 2024-12-10T17:51:43Z

Hi @sidrubs will you have time to finish that PR?

Hey @Keats we have slightly changed our focus, so am not going to finish the PR anytime soon :-/

GlenDC · 2025-01-10T22:26:23Z

@Keats

**Note: ** This POC only implements the HMAC family of algorithms as I didn't want to spend a bunch of time on something that was not going to be accepted.

Would this scope still be acceptable or do you need them all to be implemented in 1 PR?

Keats · 2025-01-11T11:26:13Z

I think it would be easier to implement all of it in one PR

GlenDC · 2025-01-11T11:28:45Z

I think it would be easier to implement all of it in one PR

Perfect. Thanks for verifying!

sidrubs added 5 commits September 30, 2024 09:43

feat(encoder): Add encoder builder

502faa4

feat(encoder): Convert to dynamic dispatch

a701547

feat(decoder): Create decoder

6a41ba7

test: Get HMAC tests passing

66d54be

docs: Neaten up docstrings

0886064

This was referenced Oct 3, 2024

Feature request: Support arbitrary crypto backends #409

Open

support ring and aws-lc-rs (default = ring) #402

Open

Keats reviewed Oct 3, 2024

View reviewed changes

feat(crypto): Implement JwtSigner and JwtVerifier for aws-lc-rs

337f9ed

Keats reviewed Oct 6, 2024

View reviewed changes

sidrubs added 3 commits October 12, 2024 12:12

feat: Remove builder style implementation

a0431d8

feat: Use original encoding and decoding key structs

4225e1f

feat(crypto): Add RSA family

78e84c1

sidrubs force-pushed the decoupled-crypto-backends branch from 0ae3b87 to 78e84c1 Compare October 16, 2024 00:42

Keats mentioned this pull request Nov 20, 2024

Add ES512 support #250

Open

GlenDC mentioned this pull request Jan 10, 2025

Add feature based on aws-lc-rs cryptographic library instead of ring #377

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decoupled crypto backends #410

Decoupled crypto backends #410

sidrubs commented Oct 3, 2024 •

edited

Loading

Keats left a comment

Keats Oct 3, 2024

sidrubs Oct 3, 2024

Keats Oct 3, 2024

sidrubs Oct 3, 2024

sidrubs commented Oct 3, 2024 •

edited

Loading

Keats commented Oct 4, 2024

sidrubs commented Oct 5, 2024

sidrubs commented Oct 5, 2024 •

edited

Loading

Keats commented Oct 6, 2024

Keats left a comment

Keats Oct 6, 2024

sidrubs Oct 7, 2024

sidrubs Oct 7, 2024

Keats Oct 8, 2024

sidrubs Oct 9, 2024

Keats Oct 6, 2024

sidrubs Oct 7, 2024

sidrubs commented Oct 7, 2024

Keats commented Oct 10, 2024

sidrubs commented Oct 20, 2024

Keats commented Oct 21, 2024

Keats commented Dec 10, 2024

sidrubs commented Dec 10, 2024

GlenDC commented Jan 10, 2025

Keats commented Jan 11, 2025

GlenDC commented Jan 11, 2025

Decoupled crypto backends #410

Are you sure you want to change the base?

Decoupled crypto backends #410

Conversation

sidrubs commented Oct 3, 2024 • edited Loading

Introduction

Implementation

Advantages

Disadvantages

Wrapping Up

Keats left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidrubs commented Oct 3, 2024 • edited Loading

Keats commented Oct 4, 2024

sidrubs commented Oct 5, 2024

sidrubs commented Oct 5, 2024 • edited Loading

Keats commented Oct 6, 2024

Keats left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidrubs commented Oct 7, 2024

Keats commented Oct 10, 2024

sidrubs commented Oct 20, 2024

Keats commented Oct 21, 2024

Keats commented Dec 10, 2024

sidrubs commented Dec 10, 2024

GlenDC commented Jan 10, 2025

Keats commented Jan 11, 2025

GlenDC commented Jan 11, 2025

sidrubs commented Oct 3, 2024 •

edited

Loading

sidrubs commented Oct 3, 2024 •

edited

Loading

sidrubs commented Oct 5, 2024 •

edited

Loading