Skip to content

crypto/noise: Verify crypto/noise signature payload#278

Merged
lexnv merged 10 commits intomasterfrom
lexnv/fix-crypto-noise
Nov 26, 2024
Merged

crypto/noise: Verify crypto/noise signature payload#278
lexnv merged 10 commits intomasterfrom
lexnv/fix-crypto-noise

Conversation

@lexnv
Copy link
Copy Markdown
Collaborator

@lexnv lexnv commented Oct 30, 2024

This PR verifies at the litep2p level the NoiseHandshakePayload crypto/noise's signature.

lexnv added 4 commits October 30, 2024 13:12
@lexnv
error: Introduce bad signature error
c5cf0a2 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
crypto/ed25519: Extract pk as bytes
a66231f 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
crypto/noise: Verify crypto noise signature
8d761d5 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
crypto/noise/tests: Adjust testing
9a5b8d1 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added the enhancement New feature or request label Oct 30, 2024
@lexnv lexnv self-assigned this Oct 30, 2024
dmitry-markin
dmitry-markin reviewed Oct 30, 2024
View reviewed changes
src/crypto/noise/mod.rs
Comment on lines +688 to +690
if !remote_public_key.verify(
&[STATIC_KEY_DOMAIN.as_bytes(), keypair_public].concat(),
&remote_key_signature,
Copy link
Copy Markdown
Collaborator

@dmitry-markin dmitry-markin Oct 30, 2024

Choose a reason for hiding this comment

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

I am a little bit confused: if remote_public_key is a network public key of a remote peer, then what is keypair_public and why we don't check them to be equal?

Copy link
Copy Markdown
Collaborator Author

@lexnv lexnv Nov 4, 2024

Choose a reason for hiding this comment

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

Will have a look, we can include this in the next release 👍

@lexnv
Copy link
Copy Markdown
Collaborator Author

lexnv commented Nov 25, 2024

Oki doki, from the noise authentication section:

  • Authentication: A Noise protocol with static public keys verifies that the
    corresponding private keys are possessed by the participant(s), but it's up to
    the application to determine whether the remote party's static public key is
    acceptable. Methods for doing so include certificates which sign the public key
    (and which may be passed in handshake payloads), preconfigured lists of public
    keys, or "pinning" / "key-continuity" approaches where parties remember public
    keys they encounter and check whether the same party presents the same public
    key in the future.

My understanding so far, noise-libp2p uses two pair of keys:

  • static set of Diffie-Hellman (DH) keys used for encrypting the session (exchanged during the handshake phase)
  • the peerID derived key given in payload.identity_key

  1. The peerID private key signs a payload containing noise-libp2p-static-key: ++ Pk DH (random message concatenated with the public key of the session PkDH). This is given to the remove as proof that the endpoint is indeed the said peerID (authorization)

  2. The peerID public key verifies the given signature and ensures the message noise-libp2p-static-key: ++ Pk DH was not tampered with

Let me know what you think of this 🙏

@lexnv
crypto/noise: Rename to dh_remote_pubkey
2575d3e 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@dmitry-markin
Copy link
Copy Markdown
Collaborator

dmitry-markin commented Nov 26, 2024

@lexnv thank you for the explanation and the links to the docs!

@lexnv lexnv merged commit d6fae55 into master Nov 26, 2024
@lexnv lexnv deleted the lexnv/fix-crypto-noise branch November 26, 2024 12:01
@lexnv lexnv mentioned this pull request Nov 27, 2024
Merged
lexnv added a commit that referenced this pull request Nov 27, 2024
@lexnv
chore: Release version 0.8.2 (#293)
28d42c6 
## [0.8.2] - 2024-11-27

This release ensures that the provided peer identity is verified at the
crypto/noise protocol level, enhancing security and preventing potential
misuses.
The release also includes a fix that caused `TransportService` component
to panic on debug builds.

### Fixed

- req-resp: Fix panic on connection closed for substream open failure
([#291](#291))
- crypto/noise: Verify crypto/noise signature payload
([#278](#278))

### Changed

- transport_service/logs: Provide less details for trace logs
([#292](#292))


cc @paritytech/networking

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv mentioned this pull request Nov 27, 2024
Merged
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Nov 27, 2024
@lexnv @actions-user
chore: Update litep2p to v0.8.2 (#6677)
f6a3178 
This includes a critical fix for debug release versions of litep2p
(which are running in Kusama as validators).

While at it, have stopped the oncall pain of alerts around
`incoming_connections_total`. We can rethink the metric expose of
litep2p in Q1.





## [0.8.2] - 2024-11-27

This release ensures that the provided peer identity is verified at the
crypto/noise protocol level, enhancing security and preventing potential
misuses.
The release also includes a fix that caused `TransportService` component
to panic on debug builds.

### Fixed

- req-resp: Fix panic on connection closed for substream open failure
([#291](paritytech/litep2p#291))
- crypto/noise: Verify crypto/noise signature payload
([#278](paritytech/litep2p#278))

### Changed

- transport_service/logs: Provide less details for trace logs
([#292](paritytech/litep2p#292))


## Testing Done

This has been extensively tested in Kusama on all validators, that are
now running litep2p.

Deployed PR: #6638

### Litep2p Dashboards
![Screenshot 2024-11-26 at 19 19
41](https://github.com/user-attachments/assets/e00b2b2b-7e64-4d96-ab26-165e2b8d0dc9)


### Libp2p vs Litep2p CPU usage

After deploying litep2p we have reduced CPU usage from around 300-400%
to 200%, this is a significant boost in performance, freeing resources
for other subcomponents to function more optimally.


![image(1)](https://github.com/user-attachments/assets/fa793df5-4d58-4601-963d-246e56dd2a26)



cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: GitHub Action <action@github.com>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Nov 28, 2024
@lexnv @actions-user
chore: Update litep2p to v0.8.2 (#6677)
51c3e95 
This includes a critical fix for debug release versions of litep2p
(which are running in Kusama as validators).

While at it, have stopped the oncall pain of alerts around
`incoming_connections_total`. We can rethink the metric expose of
litep2p in Q1.





## [0.8.2] - 2024-11-27

This release ensures that the provided peer identity is verified at the
crypto/noise protocol level, enhancing security and preventing potential
misuses.
The release also includes a fix that caused `TransportService` component
to panic on debug builds.

### Fixed

- req-resp: Fix panic on connection closed for substream open failure
([#291](paritytech/litep2p#291))
- crypto/noise: Verify crypto/noise signature payload
([#278](paritytech/litep2p#278))

### Changed

- transport_service/logs: Provide less details for trace logs
([#292](paritytech/litep2p#292))


## Testing Done

This has been extensively tested in Kusama on all validators, that are
now running litep2p.

Deployed PR: #6638

### Litep2p Dashboards
![Screenshot 2024-11-26 at 19 19
41](https://github.com/user-attachments/assets/e00b2b2b-7e64-4d96-ab26-165e2b8d0dc9)


### Libp2p vs Litep2p CPU usage

After deploying litep2p we have reduced CPU usage from around 300-400%
to 200%, this is a significant boost in performance, freeing resources
for other subcomponents to function more optimally.


![image(1)](https://github.com/user-attachments/assets/fa793df5-4d58-4601-963d-246e56dd2a26)



cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: GitHub Action <action@github.com>
github-actions bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request Nov 28, 2024
@lexnv @actions-user @github-actions
chore: Update litep2p to v0.8.2 (#6677)
0f569a0 
This includes a critical fix for debug release versions of litep2p
(which are running in Kusama as validators).

While at it, have stopped the oncall pain of alerts around
`incoming_connections_total`. We can rethink the metric expose of
litep2p in Q1.

## [0.8.2] - 2024-11-27

This release ensures that the provided peer identity is verified at the
crypto/noise protocol level, enhancing security and preventing potential
misuses.
The release also includes a fix that caused `TransportService` component
to panic on debug builds.

### Fixed

- req-resp: Fix panic on connection closed for substream open failure
([#291](paritytech/litep2p#291))
- crypto/noise: Verify crypto/noise signature payload
([#278](paritytech/litep2p#278))

### Changed

- transport_service/logs: Provide less details for trace logs
([#292](paritytech/litep2p#292))

## Testing Done

This has been extensively tested in Kusama on all validators, that are
now running litep2p.

Deployed PR: #6638

### Litep2p Dashboards
![Screenshot 2024-11-26 at 19 19
41](https://github.com/user-attachments/assets/e00b2b2b-7e64-4d96-ab26-165e2b8d0dc9)

### Libp2p vs Litep2p CPU usage

After deploying litep2p we have reduced CPU usage from around 300-400%
to 200%, this is a significant boost in performance, freeing resources
for other subcomponents to function more optimally.

![image(1)](https://github.com/user-attachments/assets/fa793df5-4d58-4601-963d-246e56dd2a26)

cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: GitHub Action <action@github.com>
(cherry picked from commit 51c3e95)
@lexnv lexnv mentioned this pull request Dec 11, 2024
Closed
68 tasks
Krayt78 pushed a commit to Krayt78/polkadot-sdk that referenced this pull request Dec 18, 2024
@lexnv @actions-user @Krayt78
chore: Update litep2p to v0.8.2 (paritytech#6677)
43e4d82 
This includes a critical fix for debug release versions of litep2p
(which are running in Kusama as validators).

While at it, have stopped the oncall pain of alerts around
`incoming_connections_total`. We can rethink the metric expose of
litep2p in Q1.





## [0.8.2] - 2024-11-27

This release ensures that the provided peer identity is verified at the
crypto/noise protocol level, enhancing security and preventing potential
misuses.
The release also includes a fix that caused `TransportService` component
to panic on debug builds.

### Fixed

- req-resp: Fix panic on connection closed for substream open failure
([paritytech#291](paritytech/litep2p#291))
- crypto/noise: Verify crypto/noise signature payload
([paritytech#278](paritytech/litep2p#278))

### Changed

- transport_service/logs: Provide less details for trace logs
([paritytech#292](paritytech/litep2p#292))


## Testing Done

This has been extensively tested in Kusama on all validators, that are
now running litep2p.

Deployed PR: paritytech#6638

### Litep2p Dashboards
![Screenshot 2024-11-26 at 19 19
41](https://github.com/user-attachments/assets/e00b2b2b-7e64-4d96-ab26-165e2b8d0dc9)


### Libp2p vs Litep2p CPU usage

After deploying litep2p we have reduced CPU usage from around 300-400%
to 200%, this is a significant boost in performance, freeing resources
for other subcomponents to function more optimally.


![image(1)](https://github.com/user-attachments/assets/fa793df5-4d58-4601-963d-246e56dd2a26)



cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: GitHub Action <action@github.com>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
@lexnv @actions-user @dudo50
chore: Update litep2p to v0.8.2 (paritytech#6677)
bf9a4b1 
This includes a critical fix for debug release versions of litep2p
(which are running in Kusama as validators).

While at it, have stopped the oncall pain of alerts around
`incoming_connections_total`. We can rethink the metric expose of
litep2p in Q1.





## [0.8.2] - 2024-11-27

This release ensures that the provided peer identity is verified at the
crypto/noise protocol level, enhancing security and preventing potential
misuses.
The release also includes a fix that caused `TransportService` component
to panic on debug builds.

### Fixed

- req-resp: Fix panic on connection closed for substream open failure
([paritytech#291](paritytech/litep2p#291))
- crypto/noise: Verify crypto/noise signature payload
([paritytech#278](paritytech/litep2p#278))

### Changed

- transport_service/logs: Provide less details for trace logs
([paritytech#292](paritytech/litep2p#292))


## Testing Done

This has been extensively tested in Kusama on all validators, that are
now running litep2p.

Deployed PR: paritytech#6638

### Litep2p Dashboards
![Screenshot 2024-11-26 at 19 19
41](https://github.com/user-attachments/assets/e00b2b2b-7e64-4d96-ab26-165e2b8d0dc9)


### Libp2p vs Litep2p CPU usage

After deploying litep2p we have reduced CPU usage from around 300-400%
to 200%, this is a significant boost in performance, freeing resources
for other subcomponents to function more optimally.


![image(1)](https://github.com/user-attachments/assets/fa793df5-4d58-4601-963d-246e56dd2a26)



cc @paritytech/sdk-node

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: GitHub Action <action@github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmitry-markin dmitry-markin dmitry-markin approved these changes

Assignees

@lexnv lexnv

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lexnv @dmitry-markin