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

Signer-less Device Pairing #281

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

Signer-less Device Pairing #281

wants to merge 10 commits into from

Conversation

nepet
Copy link
Collaborator

@nepet nepet commented Oct 1, 2023

This PR adds the "Pairing Process/Protocol" to the client stack. The Pairing-Process is a way to connect a signer-less client to a greenlight node. The pairing request has to be approved by a trusted device.

I'll add a more details about the process soon.

@nepet nepet force-pushed the main-0.2 branch 2 times, most recently from 92251ee to 981db7e Compare October 9, 2023 15:55
@nepet nepet force-pushed the main-0.2 branch 2 times, most recently from 8b5a797 to 40c5809 Compare November 5, 2023 19:52
@nepet nepet force-pushed the main-0.2 branch 2 times, most recently from ad9d0b4 to 180bb26 Compare November 26, 2023 13:24
@nepet nepet force-pushed the 202309-pairing-protocol branch 2 times, most recently from e0db8e2 to 157a322 Compare December 1, 2023 11:37
@nepet nepet force-pushed the main-0.2 branch 3 times, most recently from 644185e to d010f8d Compare February 2, 2024 18:36
@nepet nepet force-pushed the main-0.2 branch 5 times, most recently from f2b305e to f504032 Compare February 9, 2024 01:50
Base automatically changed from main-0.2 to main March 6, 2024 17:01
@nepet nepet force-pushed the 202309-pairing-protocol branch 3 times, most recently from d0f25a9 to 53d054e Compare May 20, 2024 20:59
@nepet nepet requested review from cdecker and Randy808 May 20, 2024 21:26
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this an old iteration of attestation_device.rs?


pub struct Client<T> {
inner: T,
tls: TlsConfig,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any strong objections to replacing tls, rune, and key with creds?

It would make the definition of the Client smaller and allows us to skip passing the node_id to approve_pairing

session_id: &str,
node_id: &[u8],
device_name: &str,
restrs: &str,
Copy link
Collaborator

@Randy808 Randy808 May 22, 2024

Choose a reason for hiding this comment

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

Can the restrictions argument here be dropped?

If we allow the attestation device to put whatever restrictions they want, the application requesting the pairing may too little access to fulfill their use-case. Can we make it like android permissions where the base permissions are all or nothing? A gl-client consumer can show the user the restrictions put on the app with the pairing request when they receive it in get_pairing_data. Then the pairing can be completed from the restrictions from the original pairing request on the server-side.

Also, what do you think about letting the attestation device reject the pairing request? I'm thinking it might be helpful for the requesting application on the UX side if the user doesn't like the proposed restrictions

Edit: Removed the part about completing the pairing using restrictions saved by the server. As discussed, there would be no way for the server to tack-on restrictions to the message sent to the signer_requests_stream connection since it expects the client's tls keypair to cover the data. And because requiring a signature (from the requester of the new_device) for verification in the attestation device's signer is probably not a good idea.

&self,
session_id: &str,
node_id: &[u8],
device_name: &str,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make the device_name optional?

) -> () {
let mut data = vec![];
data.put(req.session_id.as_bytes());
data.put_u64(req.timestamp);
Copy link
Collaborator

@Randy808 Randy808 May 22, 2024

Choose a reason for hiding this comment

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

Should the timestamp be checked in this method to bound its staleness like what was mentioned in the comment here?:

https://github.com/Blockstream/greenlight/pull/281/files#diff-95a68fef05d112753f7fbc0859cb6df74b6df1b75994ceb8937170b228651c88R515

let mut data = vec![];
data.put(req.session_id.as_bytes());
data.put_u64(req.timestamp);
data.put(&req.node_id[..]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we pull the node_id from self here?

impl Client<Unconnected> {
pub fn new<T>(creds: T) -> Client<Unconnected>
where
T: TlsConfigProvider,
Copy link
Collaborator

@Randy808 Randy808 May 22, 2024

Choose a reason for hiding this comment

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

Should we return an error if device_creds are passed in? Or are we allowing a pairing to be scoped to a greenlight node?

@nepet nepet force-pushed the 202309-pairing-protocol branch 3 times, most recently from eefc7d3 to 5021f5b Compare June 2, 2024 22:48
nepet added 10 commits June 11, 2024 08:22
We need to create a keypair before we can actually create the cert
during the pairing process.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
This commit only affects the protos, generated files and places that
include the protos.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
This implements the service and the methods to initialize a pairing
session on the "new device"

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
Adds the PairingQR proto message and pass it to the channel once we did
the request.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
Adds a method that returns the relevant data for "old device" to approve
a pairing. This includes the requested restrictions that an old device
needs to sign off.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
Adds the approval of the old device. This does not check the signature
and does also not check the restrictions. This soley approves the
pairing request.
This needs a tls cert and a rune to be present as we need these to sign
and attestate the approval.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
This adds a method to the pairing service that allows to check the
validity of the PairingDataResponse. This is that the public key from
the session_id matches the public key from the CSR.

We may want to add a signature for the restrictions to ensure that GL
did not manipulate them. We are OK for now as a user has to aggree to
the restrictions anyway.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
This is preperatory to set up the scheduler -> signer stream.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
Signer now receives requests from the scheduler for further processing.
The signer processes a ApprovePairingRequest and responds to the
scheduler.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
This commit replaces TlsConfig dependencies introduced by the
signerless device pairing with Credentials.

Signed-off-by: Peter Neuroth <pet.v.ne@gmail.com>
uint64 timestamp = 2;

// The id of the node to pair with.
bytes node_id = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this since it looks like it's pulled from the cert on the server side?

match self.verify_rune(PendingRequest {
request: vec![],
uri: "/cln.Node/ApprovePairing".to_string(),
signature: req.sig,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the Context need to be updated to handle the signature?

pub struct Context {

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