force using signature v1 in more cases #246
Conversation
biscuit-auth/src/format/mod.rs
Outdated
| @@ -388,10 +424,15 @@ impl SerializedBiscuit { | |||
| ) -> Result<Self, error::Token> { | |||
| let keypair = self.proof.keypair()?; | |||
|
|
|||
| // The version block is not directly available, so we don’t take it into account here | |||
| // `append_serialized` is only used for third-party blocks anyway, so maybe we should make `external_signature` mandatory and not bother | |||
There was a problem hiding this comment.
@Geal what do you think about modifying append_serialized to require an external key?
There was a problem hiding this comment.
the external key is available in ExternalSignature
There was a problem hiding this comment.
I’m talking about making the argument mandatory (it’s an option for now), but the function is never called with None in the library itself, and i’m not sure why it is exposed publicly
CodSpeed Performance ReportMerging #246 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v5 #246 +/- ##
==========================================
+ Coverage 65.14% 65.16% +0.01%
==========================================
Files 27 27
Lines 6972 6981 +9
==========================================
+ Hits 4542 4549 +7
- Misses 2430 2432 +2 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
divarvel
force-pushed
the
force-sig-v1
branch
from
59fe9d3 to
e544df5
Compare
biscuit-auth/src/format/mod.rs
Outdated
| let authority_signature_version = if authority.version >= DATALOG_3_3 { | ||
| DATALOG_3_3_SIGNATURE_VERSION | ||
| } else { | ||
| match (root_keypair, next_keypair) { | ||
| (KeyPair::Ed25519(_), KeyPair::Ed25519(_)) => 0, | ||
| _ => NON_ED25519_SIGNATURE_VERSION, | ||
| } | ||
| }; |
There was a problem hiding this comment.
the version decision should be moved to another function that can be reused for each block
There was a problem hiding this comment.
the decision is a bit different based on each case (creating a biscuit does not have previous crypto::Block available, append_serialized does not have a Block available either), so the common function will be a bit less readable
There was a problem hiding this comment.
i have done it in a separate commit. the function itself is a bit harder to read and we’re forced to use explicit iterators, but it’s indeed better to have the logic in a single place. as a bonus i was able to write tests instead of just relying on samples
biscuit-auth/src/format/mod.rs
Outdated
| @@ -388,10 +424,15 @@ impl SerializedBiscuit { | |||
| ) -> Result<Self, error::Token> { | |||
| let keypair = self.proof.keypair()?; | |||
|
|
|||
| // The version block is not directly available, so we don’t take it into account here | |||
| // `append_serialized` is only used for third-party blocks anyway, so maybe we should make `external_signature` mandatory and not bother | |||
There was a problem hiding this comment.
the external key is available in ExternalSignature
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
divarvel
force-pushed
the
force-sig-v1
branch
from
f58c393 to
bf80230
Compare
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:
In addition to that, third-party blocks always require signature v1, which is the only breaking change we allow
Enabling these changes on samples made me realize the integer wraparound sample used datalog v3.3 features (lenient not equals), so I updated it to make it use the strict version.