Make better use of Tink #22
Open
Conversation
This file contains hidden or 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
Switch to using the Tink methods for converting ECDSA signatures to and from DER. This replaces a custom implementation using BC.
Use Tink's helper functions for converting between byte and BigInteger representations and for generating EC key pairs. Delete custom implementations of the same logic.
When there is no security provider passed to the sign and verify methods, rely on Tink to select a good and fast implementation from the available options. Currently, Tink prefers to use Conscrypt if it's available for improved performance.
vikram-gaur
reviewed
Sep 9, 2024
| ECPoint pubPoint = ((ECPublicKey) keyPair.getPublic()).getW(); | ||
| byte[] x = arrayFromBigNum(pubPoint.getAffineX(), keySize); | ||
| byte[] y = arrayFromBigNum(pubPoint.getAffineY(), keySize); | ||
| int keySize = fieldSizeInBytes(getCurveSpec(curveType).getCurve()); |
Collaborator
There was a problem hiding this comment.
nit: Not sure if we would benefit from computing the value vs having a fixed value here.
Contributor
Author
There was a problem hiding this comment.
Let me know your preference. The current CL preferred fewer magic values.
vikram-gaur
reviewed
Sep 9, 2024
| signature = Signature.getInstance(algorithm.getJavaAlgorithmId()); | ||
| } else { | ||
| signature = Signature.getInstance(algorithm.getJavaAlgorithmId(), provider); | ||
| return new EcdsaSignJce(key, getHashType(algorithm), EcdsaEncoding.DER).sign(message); |
Collaborator
There was a problem hiding this comment.
should we instead do this if we are asked to use tink provider rather than putting tink by default?
Contributor
Author
There was a problem hiding this comment.
Tink isn't a security provider but a library that uses the available providers. In the case of ECDSA signatures, it has a preference for the conscrypt provider if it's available because it's known to have much better performance. I thought it worth trying to capture that performance benefit.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Use the utilities that the library already provides but that were being re-implemented and use for ECDSA signatures, like it is for X25519 signatures, for a more consistent interface and allowing Tink to apply the provider preferences and perform the extra safety checks that are known to be appropriate.