Add ECDSA key support (SECP256R1) again #108
Open
+681
−2,552
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
just want to get plumbed in and existing tests passing
sync upstream
This implements 3rd party block generation and appending to existing tokens. It also fixes existing issues with public key interning which made deserialization of tokens with 3rd party blocks incorrect in some cases. Co-authored-by: Geoffroy Couprie <contact@geoffroycouprie.com>
# Conflicts: # src/main/java/org/biscuitsec/biscuit/crypto/KeyPair.java # src/main/java/org/biscuitsec/biscuit/crypto/PublicKey.java # src/main/java/org/biscuitsec/biscuit/token/Biscuit.java # src/main/java/org/biscuitsec/biscuit/token/UnverifiedBiscuit.java # src/main/java/org/biscuitsec/biscuit/token/builder/parser/Parser.java # src/main/java/org/biscuitsec/biscuit/token/format/SerializedBiscuit.java # src/test/java/org/biscuitsec/biscuit/token/BiscuitTest.java
Implement 3rd party block creation (biscuit-auth#104)
# Conflicts: # src/main/java/org/biscuitsec/biscuit/token/UnverifiedBiscuit.java
Korbik
requested changes
Jan 15, 2025
| import java.security.SecureRandom; | ||
| import java.security.Signature; | ||
|
|
||
| class Ed25519KeyPair extends KeyPair { |
There was a problem hiding this comment.
Suggested change
| class Ed25519KeyPair extends KeyPair { | |
| final class Ed25519KeyPair extends KeyPair { |
pom.xml
Outdated
| <dependency> | ||
| <groupId>org.bouncycastle</groupId> | ||
| <artifactId>bcprov-jdk18on</artifactId> | ||
| <version>1.78.1</version> |
There was a problem hiding this comment.
Suggested change
| <version>1.78.1</version> | |
| <version>1.79</version> |
Do we bump the version of Bouncy ?
| import java.security.Security; | ||
| import java.security.Signature; | ||
|
|
||
| class SECP256R1KeyPair extends KeyPair { |
There was a problem hiding this comment.
Suggested change
| class SECP256R1KeyPair extends KeyPair { | |
| final class SECP256R1KeyPair extends KeyPair { |
| return Either.left(new Error.FormatError.Signature.InvalidSignatureSize(signature.length)); | ||
|
|
||
| if (publicKey.algorithm == Schema.PublicKey.Algorithm.Ed25519) { | ||
| if (signature.length != 64) { |
There was a problem hiding this comment.
Could be an improvement to replace it by a constant.
| return Either.left(new Error.FormatError.Signature.InvalidSignatureSize(signature.length)); | ||
| } | ||
| } else if (publicKey.algorithm == Schema.PublicKey.Algorithm.SECP256R1) { | ||
| if (signature.length < 68 || signature.length > 72) { |
There was a problem hiding this comment.
Could be an improvement to replace it by a constant.
| @@ -335,20 +337,25 @@ public UnverifiedBiscuit copy() throws Error { | |||
|
|
|||
| public Biscuit verify(PublicKey publicKey) throws Error, NoSuchAlgorithmException, SignatureException, InvalidKeyException { | |||
| SerializedBiscuit serializedBiscuit = this.serializedBiscuit; | |||
| serializedBiscuit.verify(publicKey); | |||
| var result = serializedBiscuit.verify(publicKey); | |||
There was a problem hiding this comment.
Good catch, I will check the impact to remove vavr in the future. It is not the best fit for our purpose.
|
The failing test from the specification is linked to this PR biscuit-auth/biscuit#175 |
Thanks for the review, will address those comments 🚀 Regarding the test you updated; is that |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes #101
Had some difficulties syncing latest on the other PR, because the fork got disconnected from upstream when my organisation did a bulk visibility change across all repositories. Have just done in my personal account, same branch as before but synced with upstream.
Uses BouncyCastle for the provider. Any feedback greatly appreciated. Thanks!
I also have a follow up PR for adding remote signing capabilities, for when the private key is not directly accessible. I'll open that later, once these changes (hopefully) get accepted 🤞 . I understand you have other implementations to coordinate so no rush.