Skip to content

Conversation

@preuss-adam
Copy link
Contributor

Changes are best reviewed commit-by-commit.

  • Remove cached revocation identifiers from UnverifiedBiscuit.
  • Augment UnverifiedBiscuit with external key accessors.
  • Fix algorithm type of proof.

This simplifies the code and doesn't do unnecessary recomputation of the revoked ids collection
every time a new block is added. Likely, a relying party would only access the collection of revoked
ids once to check revocation anyway. This is in line with the rust implemention that also assembles
the collection of revoked identifiers on the fly.
These methods are akin to those of the rust implementation.
@KannarFr
Copy link
Contributor

KannarFr commented Jun 7, 2025

Why removing revocationsIds from unverified?

@preuss-adam
Copy link
Contributor Author

preuss-adam commented Jun 7, 2025

Why removing revocationsIds from unverified?

This simplifies the code and doesn't do unnecessary recomputation of the revoked ids collection
every time a new block is added. Likely, a relying party would only access the collection of revoked
ids once to check revocation anyway. This is in line with the rust implemention that also assembles
the collection of revoked identifiers on the fly.


public Option<PublicKey> blockExternalKey(int index) {
if (index == 0) {
return authority.getExternalKey();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this will always turn None, there is no external signature on the authority block

? new Proof.FinalSignature(data.getProof().getFinalSignature().toByteArray())
: new Proof.NextSecret(
KeyPair.generate(
authority.getKey().getAlgorithm(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch, thanks

@Geal
Copy link
Contributor

Geal commented Jun 9, 2025

extracting the revocation ids on the fly is fine, yes.

@preuss-adam could you fill the Eclipse Contributor agreement please? (this is not a CLA, it is just there to guarantee that you are authorized to make this contribution)

@preuss-adam
Copy link
Contributor Author

extracting the revocation ids on the fly is fine, yes.

@preuss-adam could you fill the Eclipse Contributor agreement please? (this is not a CLA, it is just there to guarantee that you are authorized to make this contribution)

@Geal Apologies for the delay. I've signed the agreement now.

@preuss-adam preuss-adam requested a review from Geal July 7, 2025 17:56
@preuss-adam
Copy link
Contributor Author

@Korbik @Geal anything further or this one or are we good to merge? thx

@Korbik
Copy link
Contributor

Korbik commented Jul 10, 2025

@Korbik @Geal anything further or this one or are we good to merge? thx

I was expecting a change as a return None for the comment of @Geal but I think it is okay and it makes sense keeping like this for understanding.
https://github.com/eclipse-biscuit/biscuit-java/pull/124/files#r2136416676

@Korbik Korbik merged commit b3631cc into eclipse-biscuit:main Jul 10, 2025
2 checks passed
@preuss-adam
Copy link
Contributor Author

@Korbik @Geal anything further or this one or are we good to merge? thx

I was expecting a change as a return None for the comment of @Geal but I think it is okay and it makes sense keeping like this for understanding. https://github.com/eclipse-biscuit/biscuit-java/pull/124/files#r2136416676

Gotcha. I could change it if you like. I left it like this because the rust version behaves the same way.

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.

4 participants