common icon indicating copy to clipboard operation
common copied to clipboard

sr25519: switch from wasm to micro-sr25519

Open paulmillr opened this issue 10 months ago • 15 comments

sr cryptography in 467 lines of js code: https://github.com/paulmillr/micro-sr25519

  • All tests pass
  • There are 2 linting issues which I have 0 ideas how to fix:
packages/util-crypto/src/sr25519/deriveHard.ts
  8:9  error  Avoid referencing unbound methods which may cause unintentional scoping of `this`.
If your function does not access `this`, you can annotate it with `this: void`, or consider using an arrow function instead  @typescript-eslint/unbound-method

packages/util-crypto/src/sr25519/deriveSoft.ts
  8:63  error  Avoid referencing unbound methods which may cause unintentional scoping of `this`.
If your function does not access `this`, you can annotate it with `this: void`, or consider using an arrow function instead  @typescript-eslint/unbound-method

As a side note, this repo's dev infrastructure is slightly idiotic and urgently needs to be improved if the goal is to gain any new outside contributors. It took me 1 hour what should have taken 4 minutes. The test runner takes 3 minutes (too slow, not parallel) - would be great to have an option to crash on a first failed test. README and CONTRIBUTING.md have some bogus rules while not mentioning what's polkadot-dev-run-test and how it works. Linting is non-descriptive: for example, I don't understand what the rule above means (even though i've coded a few js libraries).

This is not really something I "want". This is something that you folks struggle with daily. Imagine how much time you're losing!

paulmillr avatar Jan 13 '25 21:01 paulmillr

For the CI errors, it just needs a yarn install pushed up.

As a side note, this repo's dev infrastructure is slightly idiotic and urgently needs to be improved if the goal is to gain any new outside contributors.

I agree, (preaching to the choir per say). That being said the polkadot-js libs are huge, and so coupled that its hard to make headway with such little bandwidth to spare (Getting way more help soon which should really help move some things forward). That being said the dev infrastructure is all custom implementations or wrappers with no documentation which makes it even more annoying (I've been making some progress on changing that). All in all yes, I agree with your sentiment, when maintenance transferred over to me about a year ago I was left with a bunch of burning fires, a lot have been put out, but there is still much work to be done. Luckily bandwidth will be freeing up soon for me to focus on more QOL goals.


As for the content of the PR, I need to look into this a bit more in depth this week - I love the noble libs as it is, and know a bit of your work (Thank you for all the useful libs you publish) - but I still need to do my due diligence as this would be a big change.

TarikGul avatar Jan 14 '25 04:01 TarikGul

@TarikGul micro-sr25519 was funded by polkadot treasury BTW. There was a proposal.

paulmillr avatar Jan 14 '25 04:01 paulmillr

Any updates?

paulmillr avatar Jan 30 '25 16:01 paulmillr

I did look into the error, but couldn't find the source of the issue (yet!). Will continue some efforts tomorrow.

I read the treasury proposal - I'm under the assumption that there are ongoing audits and/or suppose to be audits in the future?

TarikGul avatar Jan 30 '25 16:01 TarikGul

Creator of proposal mentioned they could engage auditors; and I told them that the library should be tested somewhere before audit, because if, while testing, some changes would become necessary, that would invalidate the audit.

Since we have kinda tested the library with this pull request, albeit in a limited way, i’m ok with deferring the merge until post-audit.

paulmillr avatar Jan 30 '25 20:01 paulmillr

i’m ok with deferring the merge until post-audit.

This would be ideal. In the meantime I'll make sure I have a good understanding of this error/issue with the linter so the process for getting this in is smoother once the audit is complete. - Thanks

TarikGul avatar Jan 30 '25 21:01 TarikGul

@TarikGul @ShankarWarang @farwayer any news?

mahnunchik avatar Mar 21 '25 22:03 mahnunchik

@TarikGul @ShankarWarang @farwayer any news?

Haven't heard anything yet from my end.

TarikGul avatar Mar 22 '25 02:03 TarikGul

We are currently blocked from integrating Polkadot into the Coin Wallet until polkadot-js migrates to the lightweight micro-sr25519 library.

mahnunchik avatar Mar 24 '25 17:03 mahnunchik

We are currently blocked from integrating Polkadot into the Coin Wallet until polkadot-js migrates to the lightweight micro-sr25519 library.

An audit just needs to be completed, and once I hear anything about it - we can start to integrate the following.

TarikGul avatar Mar 26 '25 06:03 TarikGul

We are currently blocked from integrating Polkadot into the Coin Wallet until polkadot-js migrates to the lightweight micro-sr25519 library.

An audit just needs to be completed, and once I hear anything about it - we can start to integrate the following.

We have evaluated the audit offerings with 3 firms. We will finalise one and curate an OpenGov proposal for the same.

ShankarWarang avatar Mar 26 '25 06:03 ShankarWarang

@ShankarWarang any updates on this?

valentinfernandez1 avatar May 19 '25 15:05 valentinfernandez1

@ShankarWarang any updates on this?

The audit engagement by Oak Security (selected firm out of the 3) have been started yesterday and we are expecting the first report on 23rd May.

Timeline updates:

  • 29th March : Edgetributor SubDAO contributors came to the conclusion that Oak Security's offerings/scope was better than other two auditing firms. We considered the background, experience, stack familiarity of the involved researchers/auditors and the number of researchers proposed in the auditing scopes provided by the 3 candidate firms. Additional qualifications' judgement included Polkadot ecosystem familiarity, low level JS/TS experience and cryptographic curves familiarity in this exact order.
  • 8th April : The proposal for Oak Security's audit funding was drafted to OpenGov: https://polkadot.polkassembly.io/referenda/1520
  • 15th April : The proposal was proposed to the dotPAL bounty as the community advocated for the same in OpenGov.
  • 25th April : The proposal got approved by dotPAL bounty curators.
  • 28th April : First draft of the Service Agreement was shared by Oak Security for the review.
  • 1st May : Funds disbursal was done to Edgetributor SubDAO: https://polkadot.subscan.io/account/13UVJyLnbVp9RBZYFwHZ1tfzDT8J4osUGw5XgrtWo4Qtep8y?tab=transfer
  • 2nd May : Liquidation of DOT to USDC was done by the Edgetributor SubDAO and the remaining liquidation buffer was returned to the dotPAL bounty: https://hydration.subscan.io/account/16VCqpfJVJWRF2ZM76bePBgHgs4uCYwstMB4wYBXxSaTQR2H?tab=transfer, https://polkadot.subscan.io/extrinsic/25840823-2
  • 2nd May : Audit engagement start date was scheduled for 19th May as per the earliest slots available.
  • 7th May : After 4 iterations of refinements suggested by Edgetributor SubDAO to Oak Security, the Service Agreement was finalised for signing from both the parties.
  • 16th May : 50% of the audit fee was disbursed from Edgetributor SubDAO to Oak Security: https://hydration.subscan.io/extrinsic/0xaa8191122d9c672ad910d812131fd304888ad4aea79ee937b6967bf0606ff05d, https://etherscan.io/tx/0x5ee0bf471c31143a3332f36c5705fcf93abde13d14118622831289b1e5090a99
  • 19th May : The Audit Service Agreement was signed and counter-signed by Edgetributor SubDAO and Oak Security respectively.
  • 19th May : Main Security Audit (1st Stream) process was initialised involving 1 Project Manager, 1 Lead Auditor/Researcher and 3 Auditors/Researchers.

Upcoming:

  • Between 19th - 23rd May: Differential Fuzz Testing (2nd Stream) will be executed by 1 Tester.
  • 23rd May : First Audit Report for the first two streams will be published with all the issues auditors discovered. And the auditors will then be available for 3 weeks to review any fixes from @paulmillr's and/or any other contributor's end.
  • July 28th - August 1st : Additional Security Audit (3rd Stream) by 1 Senior Auditor/Researcher.
  • August 1st : Updated/final audit report from the 3rd stream will be published, including all the issues the auditor discovered. The auditor will then be available for 3 weeks to review any fixes from @paulmillr's and/or any other contributor's end.
  • Upon project completion : Disbursal of the remaining 50% of the audit fee from Edgetributor SubDAO to Oak Security and return the remaining Bridging Buffer to dotPAL bounty.

ShankarWarang avatar May 20 '25 06:05 ShankarWarang

it's done https://github.com/paulmillr/scure-sr25519/tree/main/audit

paulmillr avatar Jun 13 '25 17:06 paulmillr

cc: @valentinfernandez1

We can add this back to the queue :)

TarikGul avatar Jun 13 '25 17:06 TarikGul

@TarikGul @valentinfernandez1 @ShankarWarang Any news? We won't be adding support for Polkadot until the library switches to a lightweight alternative to wasm.

mahnunchik avatar Jul 11 '25 11:07 mahnunchik

@TarikGul @valentinfernandez1 @ShankarWarang Any news? We won't be adding support for Polkadot until the library switches to a lightweight alternative to wasm.

As the first audit is done, it's up to the maintainers to decide whether they want to use the @scure/sr25519 package now or wait for the additional security audit by the senior auditor/researcher which is scheduled for July 28th to August 1st.

ShankarWarang avatar Jul 11 '25 12:07 ShankarWarang

Hi @ShankarWarang has the August Audit been completed?? I will start the review as soon as we get confirmation on this.

Also @paulmillr please run yarn lint to fix the failing CI.

valentinfernandez1 avatar Aug 22 '25 14:08 valentinfernandez1

@valentinfernandez1 feel free to take over the PR.

paulmillr avatar Aug 22 '25 16:08 paulmillr

Hi @ShankarWarang has the August Audit been completed?? I will start the review as soon as we get confirmation on this.

Also @paulmillr please run yarn lint to fix the failing CI.

Yes, the review phase of the Additional Security Audit (3rd Stream) is now officially concluded and the final audit report is published just 2 hours back: https://github.com/oak-security/audit-reports/tree/main/Polkadot

@paulmillr, will we have a new release for the package or the current latest one is good to go? Thanks šŸ™Œ

ShankarWarang avatar Aug 23 '25 06:08 ShankarWarang

I wanted to make all my packages ESM-only+node20+, since node.js v20.19+ finally supports loading ESM modules from Common.js. And most people seem to be using babel / other transpilers anyway.

So, the question here is, what should be done here?

  1. Release ESM-only sr25519 today?
  2. Make some adjustments for sr25519 to be ESM+CJS like it was when the PR was created, release, then make another ESM-only release for the future?

Node.js v18 and earlier are no longer supported and don't ship security updates. My suggestion is to also bump the min required ver of polkadot-js to v20. That would allow us to be synced.

paulmillr avatar Aug 23 '25 13:08 paulmillr

In fact, the only high-level thing changed from (already-released) sr25519 v0.2.0 is the RNG commit, which is really low-severity.

I suggest using 0.2.0, if node v18 support is a must-have; then switching to ESM-only 0.3.0 if node v20 minimum is ok.

paulmillr avatar Aug 23 '25 13:08 paulmillr

@valentinfernandez1, @TarikGul, Please let us know if you need any assistance or further clarifications. Looking forward to the adoption of scure-sr25519. Regards!

ShankarWarang avatar Sep 03 '25 15:09 ShankarWarang

@paulmillr @ShankarWarang Hey sorry this is taking so long to be merged, as part of our September Audit we also audited this PR just to make sure everything was ok before merging it in.

The Auditors actually found a Medium severity issue that should be addressed šŸ™šŸ». What is the best way to connect with you guys so I can share the report?

valentinfernandez1 avatar Oct 14 '25 12:10 valentinfernandez1

just send me an email, you have it in my profile, no need to complicate anything

paulmillr avatar Oct 14 '25 12:10 paulmillr

and why would you not post the issue publicly if it's only related to this particular PR? The pr has not been merged so it doesn't matter if the issue is public or not

paulmillr avatar Oct 14 '25 12:10 paulmillr

The found vulnerability is not related to the PR but rather the micro-sr25519 library itself. So I don't think sharing an exploitable vulnerability is a good idea until it is addressed.

valentinfernandez1 avatar Oct 14 '25 12:10 valentinfernandez1

saw the issue and replied by email, to reiterate: not a big deal, we can proceed.

paulmillr avatar Oct 14 '25 12:10 paulmillr

this also needs to be updated to most recent 0.3.0 (or 0.2.0, if node v18 support is a must-have)

paulmillr avatar Oct 14 '25 12:10 paulmillr

please add those changes and I can add my final review

valentinfernandez1 avatar Oct 14 '25 13:10 valentinfernandez1