Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [SIW-742] Add cose-js sign and common utils #12

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hevelius
Copy link
Contributor

@hevelius hevelius commented Dec 6, 2023

Short description

This PR adds cose-js utils to get encryption on cbor. This PR adds only base cose-js utils methods. In another PR we refactor the doSign method to perform an encryption using io-react-native-crypto to access SE/TEE.

Note: The cose-js files are base on this repository https://github.com/erdtman/cose-js

List of Changes

  • Add cose directory with sign and common utils
  • Add node-rsa as dependency
  • Add elliptic as dependency
  • Update example app to test COSE sign on a mocked plain text
  • Update temp function in ProximityManager to perform a COSE sign

How Has This Been Tested?

Screenshots (if appropriate):

@hevelius hevelius marked this pull request as ready for review December 11, 2023 09:10
@hevelius hevelius requested a review from a team as a code owner December 11, 2023 09:10
@hevelius hevelius requested review from balanza and grausof December 11, 2023 09:10
Copy link

@balanza balanza left a 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
Copy link

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?

Comment on lines +332 to +351
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);
}
};
Copy link

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?

@hevelius
Copy link
Contributor Author

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.

@balanza the cose-js code is based on another repository and this is its original code. However it is not yet clear whether we can use this cose-js implementation for reasons related to the ISO-18013 standard. For velocity reasons, the transition to typescript is not envisaged. Refactoring in TypeScript will be the next step. I added a Jira task about it.

@balanza
Copy link

balanza commented Dec 12, 2023

I don't understand the core problem. is there a technical reason we cannot use Typescript, or is just a consideration on the effort?

@hevelius
Copy link
Contributor Author

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

@balanza
Copy link

balanza commented Dec 13, 2023

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.

Copy link
Contributor

@grausof grausof left a comment

Choose a reason for hiding this comment

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

LGTM

src/proximity-manager/index.ts Show resolved Hide resolved
@grausof
Copy link
Contributor

grausof commented Dec 14, 2023

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.

@balanza @hevelius Take a look here erdtman/cose-js#45

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.

3 participants