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: crypto nonce randomness #747

Merged
merged 9 commits into from
Aug 2, 2023
Merged

Conversation

krpeacock
Copy link
Contributor

@krpeacock krpeacock commented Aug 1, 2023

Description

Currently default nonce generation is using Math.random, a pseudorandom source of randomness. We can take advantage of better tools when they are available

Fixes SDK-1194

How Has This Been Tested?

Updated and new unit tests

Checklist:

  • My changes follow the guidelines in CONTRIBUTING.md.
  • The title of this PR complies with Conventional Commits.
  • I have edited the CHANGELOG accordingly.
  • I have made corresponding changes to the documentation.

@krpeacock krpeacock requested a review from a team as a code owner August 1, 2023 22:54
@krpeacock krpeacock changed the title Kyle/sdk 1194 webcrypto nonce feat: crypto nonce randomness Aug 1, 2023
packages/agent/src/agent/http/types.ts Outdated Show resolved Hide resolved
packages/agent/src/utils/random.ts Outdated Show resolved Hide resolved
packages/agent/src/utils/random.ts Outdated Show resolved Hide resolved
packages/agent/src/utils/random.ts Outdated Show resolved Hide resolved
packages/agent/src/utils/random.ts Outdated Show resolved Hide resolved
@krpeacock
Copy link
Contributor Author

I was aiming initially to reproduce the behavior of Math.random, but I agree that yielding a random integer is preferable. I'll simplify, using your recommendations

@@ -0,0 +1,30 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Could you write some rationale here for why we are not requiring some well-established crypto library and always using it here?

import { randomBytes } from 'crypto';

export const randomNumber = (): number => {
  return randomBytes(4).readUInt32BE(0);
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two goals we are satisfying we are taking this approach.

  1. In the interest of keeping package size small, we try to avoid introducing new dependencies, particularly in the @dfinity/agent package.

  2. The native Node.js and Browser crypto libraries are well-established in their own ecosystem. While this approach does require us to inspect the global context to determine which tool is available, it does mean that we can tap into more powerful randomness APIs than are available in any pure JavaScript implementation

See documentation below:

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

neither API is universally available (webcrypto is available in some but not all versions of Node we support), so using whichever we can detect is a compromise

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2023

size-limit report 📦

Path Size
@dfinity/agent 89.2 KB (+0.09% 🔺)
@dfinity/candid 15.48 KB (0%)
@dfinity/principal 6.77 KB (0%)
@dfinity/auth-client 94.89 KB (0%)
@dfinity/assets 91.7 KB (0%)
@dfinity/identity 91.96 KB (0%)
@dfinity/identity-secp256k1 230.53 KB (+0.05% 🔺)

Co-authored-by: Eric Swanson <64809312+ericswanson-dfinity@users.noreply.github.com>
@krpeacock krpeacock enabled auto-merge (squash) August 2, 2023 19:51
@krpeacock krpeacock merged commit cef6335 into main Aug 2, 2023
14 checks passed
@krpeacock krpeacock deleted the kyle/SDK-1194-webcrypto-nonce branch August 2, 2023 19:59
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.

None yet

2 participants