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

go/registry: Fix node sanity checks #3660

Merged
merged 1 commit into from
Jan 29, 2021
Merged

Conversation

kostko
Copy link
Member

@kostko kostko commented Jan 28, 2021

No description provided.

@kostko kostko added c:registry Category: entity/node/runtime registry service c:bug Category: bug labels Jan 28, 2021
@kostko kostko force-pushed the kostko/fix/registry-sanity-checks branch from 47314ec to ac2a5fc Compare January 28, 2021 18:13
go/registry/api/sanity_check.go Show resolved Hide resolved
"signed_node", sigNode,
"node", n,
)
return nil, nil, fmt.Errorf("%w: registration signed by entity", ErrInvalidArgument)
Copy link
Contributor

Choose a reason for hiding this comment

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

To summarize the logic leading to this:

  • The node is absent from its entity's node list
  • The node registration is signed by its entity

Now, there are different configurations and codepaths where we want to do different things for a node in this circumstance, but they all lead to this single VerifyRegisterNodeArgs function, so there's further logic.

  1. There's a mode of registration where an entity signs its node's registration rather than putting the node's ID in its nodes list (:information_source: this requires the entity keys to be online, which is riskier, so we don't recommend it). In fact, this is the normal circumstance when using this mode. To use this, the network enables it with a flag, and the entity enables it with a flag. In that configuration, the registration is valid.
  2. But if the network and entity are not thus configured, then we have to follow the rules for the default mode, where the node must be in its entity's node list, which it's not, and it's not valid. Depending on the codepath, we want to do different things.
    1. If we asked about validity because someone is trying to register this node right now, then turn them away, because it would be useless to add an invalid node.
    2. If we're asking about some node already in our records (a 'sanity check'), then it's not valid, but it can happen because an entity might have kicked one of its nodes out, and there's no special process to clean up the node registration. We just wait for it to expire normally, while understanding that it's not valid.
    3. However, if we're asking about an entry in a genesis file, then be extra strict and say that the node shouldn't be there, so that people don't accidentally have useless records in their genesis file.

This patch adjusts VerifyRegisterNodeArgs not to return an error in situation 2.ii.

It feels awkward from an API perspective, for a function named in this way to conclude that the registration is valid. The design that leads to all this circumstantial logic being piled into a 'verify args' routine is bursting at the seams. But on the other hand, this is a timely fix for a tool that we would like to continue running.

On a side note, it looks like the error messages returned around here are meant to be on the humanly-usable side and to guide users toward correctly setting up entity-signed nodes, but I think they could be more verbose.

@kostko kostko force-pushed the kostko/fix/registry-sanity-checks branch from ac2a5fc to fb53224 Compare January 29, 2021 08:29
@kostko kostko enabled auto-merge January 29, 2021 08:38
@kostko kostko force-pushed the kostko/fix/registry-sanity-checks branch from fb53224 to 2c59390 Compare January 29, 2021 09:43
@kostko kostko force-pushed the kostko/fix/registry-sanity-checks branch from 2c59390 to d6c8231 Compare January 29, 2021 10:10
@kostko kostko merged commit a087a87 into master Jan 29, 2021
@kostko kostko deleted the kostko/fix/registry-sanity-checks branch January 29, 2021 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:bug Category: bug c:registry Category: entity/node/runtime registry service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants