Unable to parse validator voting keystore from ethdo

[MysticRyuujin]

Description

ethdo adds the name key to the keystore when it creates an account. Lighthouse throws an error trying to read this file:

Failed to init validator client: unable to decrypt all validator directories: ValidatorDirError(UnableToReadKeystore(ReadError("unknown field `name`, expected one of `crypto`, `uuid`, `path`, `pubkey`, `version` at line 1 column 469")))

Version

670672265e59

Present Behaviour

When trying to use a validator keystore generated from ethdo the lighthouse validator throws an error, complaining about the name key in the keystore.

Expected Behaviour

Lighthouse validator should simply ignore extra keys...

Steps to resolve

I deleted the name key from the JSON and everything worked. So the obvious solution is to ignore extra keys in the keystore JSON.

[paulhauner]

Lighthouse validator should simply ignore extra keys

I'm not certain about this one, when it comes to cryptography I think it can pay to be strict.

EIP-2335 has been updated to include a description field. My preferred solution here would be to wait until clients adopt the new EIP and see them use description instead of name.

@mcdee what are your thoughts about using description instead of name in ethdo? If you'd prefer to stick with name we can special-case it fairly simply.

[mcdee]

The problem with using "description" is that the EIP explicitly states it as non-unique, which makes it unsuitable for ethdo's purposes of having unique account names.

I thought that the outcome of the call that was had regarding this was that both name and description fields were optional, with name being unique if present and description not, but apparently not. IMO the best solution would be to bring the name field in to the EIP, but Carl didn't respond to this so I guess it's not going to happen.

I'm caught between enforcing uniqueness on a field that is explicitly stated to be non-unique, or using a field that isn't in the EIP. Neither are good options, but sticking with the name field gives it a chance of being included in the EIP at a later date whereas if I use the description field it contradicts what is already in the EIP. So I'm pretty much forced to continue using the name field, even unofficially.

[MysticRyuujin]

I'm kinda confused about the security concerns around not accessing keys you're not interested in or are required, as opposed to whatever is happening now, where you're either itterating over extra keys...or you're trying to parse/read them for some reason? If you wanted to be explicit...you wouldn't even look at that key?

I mean...the use of the JSON format is hardly a security feature in itself, if yaml had been chosen instead would it be a security risk to have comments?

[banteg]

eth2.0-deposit-cli which is recommended in the official eth2 launch pad also uses the description field. So the import breaks with:

lighthouse account validator import --directory eth2.0-deposit-cli/validator_keys

Unable to read keystore JSON "eth2.0-deposit-cli/validator_keys/keystore-m_12381_3600_60_0_0-1595861715.json": ReadError("unknown field description, expected one of crypto, uuid, path, pubkey, version at line 1 column 504")

[michaelsproul]

We're adding the description field as part of #1381

[michaelsproul]

The medalla branch has been updated with the description fix, let us know if you have any issues @banteg

[paulhauner]

I'm kinda confused about the security concerns around not accessing keys you're not interested in or are required

An issue can arise if the user supplies a key that they expect to work, but is silently ignored. For example, if application adds a new key/value testnet: "medalla" where, in its world, it will only use that key on the Medalla testnet. If you fed that to Lighthouse you might expect it to do the same. If Lighthouse is strict about unknown keys it will fail loudly and early, otherwise it will proceed silently.

would it be a security risk to have comments?

IMO, no. I think it's reasonable for a user to assume that comments will be silently ignored by the application.

My preference is to add an optional name field so we can maintain support with ethdo and a strict JSON schema. If we keep seeing applications adding new keys to the JSON store, I would consider making only the top level of the JSON object allow arbitrary keys, since I really don't think it's a good idea at the crypto level.

[paulhauner]

In #1535 we have relaxed from EIP-2335 to allow:

• a name field
• a missing path field