-
Notifications
You must be signed in to change notification settings - Fork 0
feat: [SIW-742] Add cose-js sign and common utils
#12
Conversation
balanza
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Curious choice to go for as a plain vanilla JS file. We usually don't do it for several reasons:
- types help you describe what the code does
- types force you to reason about the possible combination of inputs a function can support
- types force you to parse input data before passing it to functions (implying input validation)
- types help you to spot corner cases on the code ("I didn't think this value could be null")
Can we go for a vanilla JS solution? Sure, as long as the need is well-motivate and the safety loss is mitigated (with plenty of unit tests and comments).
My advice is we either use Typescript (and we invest time in type-modeling) or we have tons of tests and comments.
| alg: 1, | ||
| crit: 2, | ||
| content_type: 3, | ||
| ctyp: 3, // one could question this but it makes testing easier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question:
one could question this but it makes testing easier
I question the question 🙃
What's the issue?
| const signMessage = async (message: string) => { | ||
| try { | ||
| const headers = { | ||
| p: { alg: 'ES256' }, | ||
| u: { kid: '11' }, | ||
| }; | ||
| const signer = { | ||
| key: { | ||
| d: Buffer.from( | ||
| '6c1382765aec5358f117733d281c1c7bdc39884d04a45a1e6c67c858bc206c19', | ||
| 'hex' | ||
| ), | ||
| }, | ||
| }; | ||
| const buf = await sign.create(headers, message, signer); | ||
| console.log('Signed message: ' + buf.toString('hex')); | ||
| } catch (error) { | ||
| console.log(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: What does this function does? Why it returns nothing?
@balanza the |
|
I don't understand the core problem. is there a technical reason we cannot use Typescript, or is just a consideration on the effort? |
Related to Typescript only effort |
Ok, thanks. I stick with my opinion: a JS implementation with a comparable level of "quality" requires more accuracy in testing and documenting. We can compare this extra effort with the type-gymnastic we are saving. Otherwise, we are deliberately adding some below-the-standard code, I don't think we need to do it. |
grausof
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@balanza @hevelius Take a look here erdtman/cose-js#45 |
Short description
This PR adds
cose-jsutils to get encryption on cbor. This PR adds only base cose-js utils methods. In another PR we refactor thedoSignmethod to perform an encryption usingio-react-native-cryptoto access SE/TEE.Note: The
cose-jsfiles are base on this repository https://github.com/erdtman/cose-jsList of Changes
cosedirectory with sign and common utilsnode-rsaas dependencyellipticas dependencyProximityManagerto perform a COSE signHow Has This Been Tested?
Screenshots (if appropriate):