Send encrypted secret on metadata #2853

andrevmatos · 2021-07-08T01:08:00Z

Fixes #2730
Based on top of #2766
Implements raiden-network/spec#344

Short description
This PR uses immutableMetadata (base PR) to send a new property in metadata: metadata.secret which is an hex-string containing the the secret object encrypted with target's public key using ECIES. The decrypted object has the format { secret: HexString<32>; amount: UInt<32>; payment_identifier: UInt<8> }. The target can then decrypt it with its private key (only when available, e.g. Wallet), validate the received amount is larger than or equal the expected amount and paymentId matches.
There's no capability for this. It's added/kept only if next hop/partner supports immutableMetadata, and targets which don't want, can't use it or validation fail will just fallback to requesting the secret as usual, so this is fully backwards compatible.

Definition of Done

Steps to manually test the change have been documented
Acceptance criteria are met
Change has been manually tested by the reviewer (dApp)

Steps to manually test the change (dApp)

Transfers between LCs skip SecretRequest/Reveal message exchange between initiator and target

codecov · 2021-07-08T01:28:02Z

Codecov Report

Merging #2853 (68abd34) into master (2f3fe20) will increase coverage by 0.10%.
The diff coverage is 97.12%.

@@            Coverage Diff             @@
##           master    #2853      +/-   ##
==========================================
+ Coverage   93.59%   93.69%   +0.10%     
==========================================
  Files         110      110              
  Lines        6071     6110      +39     
  Branches     1100     1107       +7     
==========================================
+ Hits         5682     5725      +43     
+ Misses        334      329       -5     
- Partials       55       56       +1

Flag	Coverage Δ
dapp	`80.64% <ø> (ø)`
dapp.unit	`80.64% <ø> (ø)`
sdk	`95.86% <97.12%> (+0.10%)`	⬆️
sdk.e2e	`74.22% <95.68%> (-0.14%)`	⬇️
sdk.integration	`79.26% <87.05%> (+0.69%)`	⬆️
sdk.unit	`49.56% <30.93%> (-1.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
raiden-ts/src/config.ts	`94.59% <ø> (ø)`
raiden-ts/src/transport/epics/webrtc.ts	`94.35% <0.00%> (+0.37%)`	⬆️
raiden-ts/src/transfers/epics/locked.ts	`96.21% <95.83%> (-0.14%)`	⬇️
raiden-ts/src/helpers.ts	`88.13% <100.00%> (-0.35%)`	⬇️
raiden-ts/src/messages/utils.ts	`99.27% <100.00%> (+0.06%)`	⬆️
raiden-ts/src/raiden.ts	`94.82% <100.00%> (+0.39%)`	⬆️
raiden-ts/src/services/utils.ts	`97.77% <100.00%> (ø)`
raiden-ts/src/transfers/actions.ts	`100.00% <100.00%> (ø)`
raiden-ts/src/transfers/epics/init.ts	`80.24% <100.00%> (+5.24%)`	⬆️
raiden-ts/src/transfers/mediate/epics.ts	`96.07% <100.00%> (-0.08%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f3fe20...68abd34. Read the comment docs.

andrevmatos · 2021-07-13T19:32:52Z

Rebased and squashed the last commit about config.encryptSecret on by default.

palango · 2021-07-21T11:43:03Z

raiden-ts/src/messages/utils.ts

-    return metadata;
-  else if (metadata) log?.warn('Invalid address metadata', { address, metadata });
+): [metadata: AddressMetadata, pubkey: PublicKey] | undefined {
+  if (!metadata) return;


Before we were logging a warning here, is this intended?

I don't think anything was logged if !metadata. The log call was only issued if metadata existed, so in the spirit of the happy case to the left, I check the undefined case early and just skip in that case.

palango · 2021-07-21T11:52:35Z

raiden-ts/src/raiden.ts

      )
      .toPromise();
+    this.store.dispatch(
+      transfer.request(


Wouldn't it be nicer to have a new action for this, instead of adding this resolved parameter? Something like resolveTransfer.

I thought about it, but decided for the resolved tagged union because the whole metadata and most of the payload would overlap, and I'm not sure it's worth it to create a mostly similar copy of this action in order to split those. But I don't have hard feelings for it, it was mostly about code deduplication, but if you think it'd be better to have the copy, I can do it.

They're needed for encrypted secret handling and public key calculation.

since sending encrypted secrets use only target's pubkey, it doesn't require our private key, so it can be enabled by default. target will of course only be able to act on it if they've the private key available (e.g. ether's Wallet, as default in CLI and with Raiden key)

utils's `dispatchRequestAndGetResponse` now accepts an array of AACs, and will generic on the `request` to allow multiple types to be sent in a single instantiation.

Needed in order to encrypt secrets to given peer

'transfer' method was getting too much logic. This makes the `transfer.request` action contain some intersection to tagged unions (resolved: boolean) which chooses into 2 modes: the resolved=false is handled by the new `transferRequestResolveEpic` and will call e.g. pathFind.request (previously done in `Raiden.transfer`) and after resolving everything needed, will emit `transfer.request` again with resolved=true. This allows the API's transfer.request to be way simpler. Also, `expiration` is moved to the `locked` epic, so it's more precisely calculated when the resolved action is handled.

and decode it if we can. decoding is always attempted in the receiving side. partner support is checked against `caps.immutableMetadata`, and we always attempt to send it to any target regardless of their capabilities. If they don't support it, they'll just ignore it.

Initiator never tries to call over RTC, and instead just whitelist target's. target then is to decide whether to call initiator, and does it only if 'secret' is not in transfer.metadata, meaning we'll need to ask them for it.

Just minor typing and coding usage, no logic change.

This helps unify code which validates metadata and create a presence update from it.

weilbith

Ups. I did this review a week ago but did not finish it. Sorry for that. As Paul also reviewed it and I had a hard time to continue the review on the last/new commits, I just hand it in as is...

raiden-ts/src/config.ts

raiden-ts/src/messages/utils.ts

raiden-ts/src/raiden.ts

raiden-ts/src/transfers/epics/init.ts

raiden-ts/src/transfers/utils.ts

weilbith

✔️

andrevmatos added enhancement New feature or request sdk 🖥 optimization ⚡ Optimizations for the implementation or protocol protocol 📨 Protocol-related issues and PRs labels Jul 8, 2021

andrevmatos self-assigned this Jul 8, 2021

andrevmatos added feature and removed enhancement New feature or request labels Jul 10, 2021

andrevmatos force-pushed the feature/secret branch 6 times, most recently from 8ef40d2 to 3dfb227 Compare July 12, 2021 22:17

andrevmatos requested a review from weilbith July 12, 2021 22:56

andrevmatos marked this pull request as ready for review July 12, 2021 22:56

andrevmatos force-pushed the feature/secret branch 4 times, most recently from 7448d38 to 257bc3f Compare July 13, 2021 17:30

palango reviewed Jul 21, 2021

View reviewed changes

andrevmatos added 8 commits July 23, 2021 21:10

sdk: add eciesjs & @ethersproject/hash dependencies

f3c471d

They're needed for encrypted secret handling and public key calculation.

sdk: improve utils and add PublicKey type

952f6b6

utils's `dispatchRequestAndGetResponse` now accepts an array of AACs, and will generic on the `request` to allow multiple types to be sent in a single instantiation.

sdk: add pubkey property to matrixPresence.success action

1c0a1ca

Needed in order to encrypt secrets to given peer

sdk: improve some statements in transfer and 'Last' type

3070498

Just minor typing and coding usage, no logic change.

sdk: fix and add tests for transfer resolving and pubkey in presence

2e54838

andrevmatos force-pushed the feature/secret branch from 257bc3f to 3a023c0 Compare July 24, 2021 01:16

sdk: introduce metadataToPresence util

d6c4206

This helps unify code which validates metadata and create a presence update from it.

andrevmatos force-pushed the feature/secret branch from 3a023c0 to d6c4206 Compare July 24, 2021 22:10

andrevmatos mentioned this pull request Jul 24, 2021

Check mediators for 'Mediate' capability when initiating transfer #2868

Merged

3 tasks

weilbith reviewed Jul 26, 2021

View reviewed changes

sdk: address some styling feedback

68abd34

andrevmatos requested a review from weilbith July 26, 2021 14:35

weilbith approved these changes Jul 26, 2021

View reviewed changes

andrevmatos merged commit 7782bae into master Jul 26, 2021

andrevmatos deleted the feature/secret branch July 26, 2021 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send encrypted secret on metadata #2853

Send encrypted secret on metadata #2853

andrevmatos commented Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading

andrevmatos commented Jul 13, 2021

palango Jul 21, 2021

andrevmatos Jul 23, 2021

palango Jul 21, 2021

andrevmatos Jul 23, 2021

weilbith left a comment

weilbith left a comment

Send encrypted secret on metadata #2853

Send encrypted secret on metadata #2853

Conversation

andrevmatos commented Jul 8, 2021 • edited Loading

codecov bot commented Jul 8, 2021 • edited Loading

Codecov Report

andrevmatos commented Jul 13, 2021

palango Jul 21, 2021

Choose a reason for hiding this comment

andrevmatos Jul 23, 2021

Choose a reason for hiding this comment

palango Jul 21, 2021

Choose a reason for hiding this comment

andrevmatos Jul 23, 2021

Choose a reason for hiding this comment

weilbith left a comment

Choose a reason for hiding this comment

weilbith left a comment

Choose a reason for hiding this comment

andrevmatos commented Jul 8, 2021 •

edited

Loading

codecov bot commented Jul 8, 2021 •

edited

Loading