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

Clarify producer public key #7

Closed
topcat2001 opened this issue Jun 7, 2018 · 21 comments
Closed

Clarify producer public key #7

topcat2001 opened this issue Jun 7, 2018 · 21 comments

Comments

@topcat2001
Copy link

It is unclear if the producer_public_key is the account active key, the account owner key, the block signing key or some other. Would be good to clarify.

@ellipticasec
Copy link

My own opinion, since many BPs will (and should) have multi-sig accounts this gets complicated.

Technically the key used with regproducer is the one used to mint blocks so effectively you vote to allow someone to use that key to mint.

On the other hand that key may be rotated more frequently...

@matthewdarwin
Copy link
Contributor

Or maybe allow multiple keys?

@systemzax
Copy link

I would suggest allowing multiple keys with their role description (owner, active, block signing, etc...).

@eosrio
Copy link
Collaborator

eosrio commented Jun 17, 2018

What about the following change?
From:

{
  "producer_account_name": "",
  "producer_public_key": "",
  "org": {
    "candidate_name": "",
...

To:

{
  "producer_account_name": "",
  "permissions": {
      "owner": "",
      "active": "",
      "sign": ""
   },
  "org": {
    "candidate_name": "",
...

@lukestokes
Copy link

Right now tooling seems to be validating it against the signing key. Example:

https://validate.eosnation.io/producers/eosdacserver.html

field=<producer_public_key> does not match between bp.json and regproducer

If that's the case, this should not be a high level field, but a field for individual nodes. We should then be able to switch between our primary producer and our backup producer without seeing errors like this. Each signing key should be seen as valid. If we add our producing nodes to the bp.json, then there should be no endpoint requirements at all since those should remain private and unknown for producing nodes.

"node_type": "producer",
"producer_key": "EOS7aBPDACAn1SpJDjmaZHSEUgqfNWdUaNawVZhVuEWUx5aoepJf6",

So essentially producer_key is only required for node_type = producer and all the endpoints are only required for node_type != "producer"

@lukestokes
Copy link

Follow up question:

Is producer_public_key actually used for anything? Why is it part of the spec? It's never shown to the user, so how will voting tools use this signing key? What purpose does it serve? If it serves no purpose, we should remove it.

@n8d
Copy link
Contributor

n8d commented Jul 6, 2018

I don't think it's useful. We have account, so if there is a reason someone needs a key they could get it from the chain.

@n8d
Copy link
Contributor

n8d commented Jul 6, 2018

Unless the idea is to extend it with a signature as well, as suggested in #10.

@eosrio
Copy link
Collaborator

eosrio commented Jul 6, 2018

Okay, lets so a vote (using thumbs-up/down) to remove this. Please vote thumbs down if you want to keep or extend it to allow multiple-keys.

@matthewdarwin
Copy link
Contributor

I would prefer to remove it at this time. If someone comes up with a really good use case for it in the future, then can be re-discussed. I have not yet seen anyone say what business value this provides.

@lukestokes
Copy link

@eosrio sorry, I was confused by your vote question earlier. I thought it was a thumbs down to remove. I've changed my vote to a thumbs up for removal. When we switch back and forth between producing nodes, we don't want to also have to update the signing key. It makes no sense as the signing key is defined by the value in listproducers, not by what is in a json file on a website.

@n8d
Copy link
Contributor

n8d commented Jul 24, 2018

Oh, I was confused too :)

My vote is/was to remove it.

@matthewdarwin
Copy link
Contributor

There is one use case where I know where having the key is useful. That is when someone steals a BP's identity entirely it is obvious if the key doesn't match. This already happened. (regproduce using an existing BP's URL).

@DenisCarriere
Copy link
Contributor

Removing the block signing key from the bp.json standard would also be my vote.

If someone steals your BP key, you've got bigger problems then your bp.json not syncing up.

The block signing key is already registered on the blockchain, no need to duplicate this information in the bp.json.

@matthewdarwin
Copy link
Contributor

I think you missed my point. If someone steals your bp.json and makes no changes, the block signing key will not match. (I am not saying they are stealing your key).

Anyway, as I said in the original thread, I'm fine to also remove this field entirely.

@n8d
Copy link
Contributor

n8d commented Aug 1, 2018

If I'm understanding that case correctly, producer_account_name provides the same usefulness as it wouldn't match as well. Is that right?

But yeah, I think we are all in agreement to remove it.

@DenisCarriere
Copy link
Contributor

@matthewdarwin oh yes, if someone takes over your bp.json at least you have a way to check on chain if the information is accurate & matching.

Good argument.

@lukestokes
Copy link

It's not easy to steal an identity entirely because they would have to register a different bp account name and have a different website to host their fraudulent bp.json. The account name, url, and signing key are already on the blockchain. Signing keys can change often as BPs move from primaries to backups and back again. Those changes are seen as false positives for a problem under the current schema.

I vote to ditch it.

@notchxor
Copy link

notchxor commented Aug 2, 2018

I agree that this needs to be remove. In our case we use different keys for different nodes, so everytime we change node the keys dont match.

@eosauthority
Copy link

We removed this on our bp.json and our validation failed on the portal side.
This is still required on the schema
https://github.com/eosrio/bp-info-standard/blob/master/schema.json#L35

Can we get the Schema updated?

@igorls
Copy link
Member

igorls commented Aug 7, 2018

Yes, updating soon

@igorls igorls closed this as completed Aug 7, 2018
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

No branches or pull requests

10 participants