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

Check both node and alphabet signature in netmap contract #154

Closed
fyrchik opened this issue Oct 19, 2021 · 2 comments · Fixed by #189
Closed

Check both node and alphabet signature in netmap contract #154

fyrchik opened this issue Oct 19, 2021 · 2 comments · Fixed by #189
Assignees
Labels
netmap Netmap contract related issue
Milestone

Comments

@fyrchik
Copy link
Contributor

fyrchik commented Oct 19, 2021

Wait until nspcc-dev/neo-go#2150 is resolved.

@alexvanin
Copy link
Contributor

alexvanin commented Oct 25, 2021

I found interesting thing when I was testing nspcc-dev/neo-go#2150 with reverted #155

Storage node prepares AddPeer tx with 4 witnesses. It passes test invocation and node sends notary request.
Alphabet nodes do not resign this tx. Instead they extend NodeInfo with LOCODE attributes, hence create new AddPeer tx with 3 witnesses (no storage node witness anymore). And test invoke now fails.

We must consider this, simple revert is not enough.

@cthulhu-rider cthulhu-rider self-assigned this Nov 10, 2021
cthulhu-rider pushed a commit to cthulhu-rider/neofs-contract that referenced this issue Nov 15, 2021
@alexvanin
Copy link
Contributor

alexvanin commented Nov 16, 2021

AddPeer is indeed the main issue there. It must contain runtime.CheckWitness(nodeInfoKey) otherwise it will be possible for malicious user to create notary tx with AddPeer invocation that includes someone else's public key.

However Alphabet nodes create new AddPeer invocation transaction due to changes in Storage node attributes (e.g. LOCODE parse). Therefore Alphabet nodes cannot provide original witness and runtime.CheckWitness(nodeInfoKey) will always fail.

There are several options how we can handle it:

1. Split AddPeer into two methods for notary enabled environment

First method will contain runtime.CheckWitness(nodeInfoKey) check. Alphabet nodes will listen notary notification for this method. But to approve new peer, Alphabet nodes will invoke second method that contain runtime.CheckWitness(alphabetMulitisig).

Pros: no additional changes in Storage node.
Cons: additional method in contracts.

2. Storage node should attach only it's own witness in AddPeer invocation.

Alphabet nodes must check if AddPeer invocation omits Alphabet mulitisignature witness and contains only it's own witness.

Pros: no changes in contract.
Cons: inconvenient scheme of notary invocations in Storage node.

3. Do not modify Storage node attributes

Even if we can solve LOCODE attribute issue by attaching database in Storage node releases, it won't work with dynamic attribute changes for subnetworks.

cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Dec 7, 2021
After nspcc-dev/neofs-contract#154 alphabet
nodes should call `Register` method for approval of the notifications
spawned by `AddPeer` method.

Change `NewFromMorph` constructor of netmap morph client: it now makes
client to invoke `register` method of Netmap contract instead of
`addPeer` inside `AddPeer` method if `AsAlphabet` option is provided.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Dec 7, 2021
After nspcc-dev/neofs-contract#154 alphabet
nodes should call `Register` method for approval of the notifications
spawned by `AddPeer` method.

Change `NewFromMorph` constructor of netmap morph client: it now makes
client to invoke `register` method of Netmap contract instead of
`addPeer` inside `AddPeer` method if `AsAlphabet` option is provided.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Dec 7, 2021
After nspcc-dev/neofs-contract#154 alphabet
nodes should call `Register` method for approval of the notary
notifications spawned by `AddPeer` method.

Change `AddPeerNotaryEvent` constant to `register`.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
cthulhu-rider pushed a commit to cthulhu-rider/neofs-node that referenced this issue Dec 7, 2021
After nspcc-dev/neofs-contract#154 alphabet
nodes should call `Register` method for approval of the notary
notifications spawned by `AddPeer` method.

Call `register` method for peer approval in Netmap processor.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
aprasolova pushed a commit to aprasolova/neofs-node that referenced this issue Mar 5, 2022
After nspcc-dev/neofs-contract#154 alphabet
nodes should call `Register` method for approval of the notary
notifications spawned by `AddPeer` method.

Call `register` method for peer approval in Netmap processor.

Signed-off-by: Leonard Lyubich <leonard@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
netmap Netmap contract related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants