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

Add methods to NodeAPI for signmessage and checkmessage [issue #109] #319

Merged
merged 1 commit into from
Aug 7, 2023

Conversation

lnd3v
Copy link

@lnd3v lnd3v commented Jul 14, 2023

Early (partial) commit.

This commit augments the Breez SDK with message signing and verification facilities. Changes span across the Node API interface, its implementation (Greenlight), BreezServices and the UniFFI binding, with amendments in the FFI definition.

  • Added sign_message() and check_message() methods to the NodeAPI trait and Greenlight.
  • Added async calls for these methods in BreezServices.
  • Exposed said calls via the UniFFI binding for FFI.
  • Updated the FFI definition to mirror these additions.

Todo:

[ x] formatting
[ x] tests
[ ] etc.

Copy link
Member

@roeierez roeierez left a comment

Choose a reason for hiding this comment

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

Thank you @lnd3v looks good, dropped some comments.

libs/sdk-core/src/greenlight/node_api.rs Outdated Show resolved Hide resolved
libs/sdk-core/src/breez_services.rs Outdated Show resolved Hide resolved
libs/sdk-react-native/src/index.tsx Outdated Show resolved Hide resolved
libs/sdk-react-native/src/index.tsx Outdated Show resolved Hide resolved
@lnd3v lnd3v force-pushed the msg_signing branch 2 times, most recently from 2031f76 to caea3a9 Compare July 15, 2023 05:56
libs/sdk-core/src/breez_services.rs Outdated Show resolved Hide resolved
@lnd3v lnd3v force-pushed the msg_signing branch 5 times, most recently from 859de80 to a05f9c1 Compare July 20, 2023 09:02
@lnd3v
Copy link
Author

lnd3v commented Jul 22, 2023

For the record, i had to ask about signing on the greenlight discord. The response I got:

cdecker — Today at 15:50
We are currently using the signmessage method to create attestations and authorizations for keyless clients. If we were to allow clients to call signmessage they could escalate privileges trivially.

We are going to change that soon and signmessage will be enabled. In the meantime you may find the vls code helpful in deriving the secret and signing the message.

After that it was easily to find the signing code. It works when doing a copy and paste, however when trying to use the vls code directly it fails as trait fields are private. If they were public it would be as simple as:

# cargo.toml
vls-core = "0.9.1-rc.2"
# node_api.rs
use lightning_signer::signer::derive::{KeyDerive, NativeKeyDerive};
async fn sign_message(&self, message: &str) -> Result<String> {
    let native_key_derive = NativeKeyDerive { network: bitcoin::network::constants::Network::Bitcoin };
    let (_pubkey_from_node_id, seckey) = KeyDerive::node_keys(&native_key_derive, &self.secret, &Secp256k1::new());
    println!("_pubkey_from_node_id: {} ", hex::encode(_pubkey_from_node_id.serialize()));
    Ok(sign(message.as_bytes(), &seckey).unwrap())
}

However since the NativeKeyDerive trait doesn't have a public constructor i'll go ahead and do the copy & paste approach for now, which will just have to get cleaned out once the signer's sign_message works as expected for signing lightning messages.

@lnd3v lnd3v force-pushed the msg_signing branch 2 times, most recently from 164e447 to c1038c8 Compare July 23, 2023 01:08
@lnd3v lnd3v force-pushed the msg_signing branch 3 times, most recently from bf0fe2c to 5440aca Compare July 26, 2023 14:19
Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

Some small comments about cleaning up.

libs/sdk-core/Cargo.toml Outdated Show resolved Hide resolved
libs/sdk-core/src/greenlight/node_api.rs Outdated Show resolved Hide resolved
libs/sdk-core/src/greenlight/node_api.rs Outdated Show resolved Hide resolved
libs/sdk-core/src/greenlight/node_api.rs Outdated Show resolved Hide resolved
libs/sdk-core/src/greenlight/node_api.rs Outdated Show resolved Hide resolved
libs/sdk-react-native/ios/RNBreezSDK.swift Outdated Show resolved Hide resolved
libs/sdk-react-native/ios/RNBreezSDK.swift Outdated Show resolved Hide resolved
libs/sdk-react-native/src/index.tsx Outdated Show resolved Hide resolved
libs/sdk-react-native/src/index.tsx Outdated Show resolved Hide resolved
@JssDWt JssDWt force-pushed the msg_signing branch 2 times, most recently from 9770d78 to bab3bd3 Compare August 3, 2023 20:04
@JssDWt
Copy link
Contributor

JssDWt commented Aug 3, 2023

  • Rebased
  • Upgraded greenlight dependency to include the merged sign_message PR
  • Fixed react native bindings. Android works, iOS needs to be tested still.

@JssDWt
Copy link
Contributor

JssDWt commented Aug 3, 2023

Renamed signed_message to signature

@JssDWt
Copy link
Contributor

JssDWt commented Aug 3, 2023

Added sign/check message to the flutter bridge
@erdemyerebasmaz can you check whether I did that right?

@JssDWt JssDWt force-pushed the msg_signing branch 2 times, most recently from ba64975 to f9cced6 Compare August 4, 2023 09:17
@JssDWt
Copy link
Contributor

JssDWt commented Aug 4, 2023

  • rebased
  • Updated swift mappings in sdk-react-native. iOS now also works.

Copy link
Collaborator

@dangeross dangeross left a comment

Choose a reason for hiding this comment

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

Looks good! Left 2 nitty comments

libs/sdk-react-native/ios/BreezSDKMapper.swift Outdated Show resolved Hide resolved
libs/sdk-react-native/ios/BreezSDKMapper.swift Outdated Show resolved Hide resolved
@erdemyerebasmaz
Copy link
Contributor

Added sign/check message to the flutter bridge @erdemyerebasmaz can you check whether I did that right?

@JssDWt looks good!

@JssDWt
Copy link
Contributor

JssDWt commented Aug 4, 2023

PR is ready to merge.
Is it a problem that the greenlight dependency is updated in this PR @ok300 @roeierez?

@JssDWt JssDWt requested a review from ok300 August 4, 2023 11:56
Copy link
Contributor

@ok300 ok300 left a comment

Choose a reason for hiding this comment

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

Generally LGTM, left a few comments.

I don't think it's a problem that the GL dependencies were updated, especially since the new GL version is needed for this feature.

libs/sdk-core/src/binding.rs Show resolved Hide resolved
libs/sdk-core/src/binding.rs Outdated Show resolved Hide resolved
libs/sdk-core/src/breez_services.rs Show resolved Hide resolved
@ok300
Copy link
Contributor

ok300 commented Aug 5, 2023

Fixes #109

@JssDWt JssDWt force-pushed the msg_signing branch 3 times, most recently from 18c4606 to bb1b29d Compare August 7, 2023 09:18
@JssDWt
Copy link
Contributor

JssDWt commented Aug 7, 2023

  • Updated documentation on functions and structs.
  • Rebased

]

This commit augments the Breez SDK with message signing and verification facilities. Changes span across the Node API interface, its implementation (Greenlight), BreezServices and the UniFFI binding, with amendments in the FFI definition.

- Added sign_message() and check_message() methods to the NodeAPI trait and Greenlight.
- Added async calls for these methods in BreezServices.
- Exposed said calls via the UniFFI binding for FFI.
- Updated the FFI definition to mirror these additions.
@JssDWt JssDWt merged commit 8e08341 into breez:main Aug 7, 2023
5 checks passed
@JssDWt JssDWt temporarily deployed to github-pages August 7, 2023 09:37 — with GitHub Actions Inactive
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.

7 participants