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.

Sign up for GitHub

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

Jump to bottom

Support Prysm and Ethdo Keystores (Fixes #4107) #4149

Merged
merged 1 commit into from
Sep 19, 2022
Merged

Support Prysm and Ethdo Keystores (Fixes #4107) #4149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion beacon_chain/nimbus_beacon_node.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1983,7 +1983,8 @@ proc doSlashingImport(conf: BeaconNodeConf) {.raises: [SerializationError, IOErr

var spdir: SPDIR
try:
spdir = Json.loadFile(interchange, SPDIR)
spdir = Json.loadFile(interchange, SPDIR,
requireAllFields = true)
except SerializationError as err:
writeStackTrace()
stderr.write $Json & " load issue for file \"", interchange, "\"\n"
Expand Down
96 changes: 8 additions & 88 deletions beacon_chain/spec/eth2_apis/eth2_rest_serialization.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2034,100 +2034,23 @@ proc readValue*(reader: var JsonReader[RestJson], value: var ScryptParams) {.

## Keystore
proc writeValue*(writer: var JsonWriter[RestJson], value: Keystore) {.
raises: [IOError, Defect].} =
writer.beginRecord()
writer.writeField("crypto", value.crypto)
if value.description.isSome:
writer.writeField("description", value.description.get)
writer.writeField("pubkey", value.pubkey)
writer.writeField("path", string(value.path))
writer.writeField("uuid", value.uuid)
writer.writeField("version", JsonString(
Base10.toString(uint64(value.version))))
writer.endRecord()
error: "keystores must be converted to json with Json.encode(keystore). " &
Copy link
Member

@arnetheduck arnetheduck Sep 19, 2022 •
edited by zah
Loading

Choose a reason for hiding this comment

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

we use the "default" json serialization for logging - we should use a dedicated standard serializer for the keystore standard, which may or may not match our logging json serializer

Copy link
Member

Choose a reason for hiding this comment

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

ie the correct way to do this is to introduce a KeystoreJson that implements the keystore standard, whatever that may be - we cannot keep on using Json, this prevents migrating nim-json-rpc to json-ser for example

Copy link
Contributor Author

@zah zah Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

While this would be a theoretical improvement, the current state of affairs is that Json.encode(keystore) is a correct implementation of the standard, while the removed functions here were not.

Copy link
Member

Choose a reason for hiding this comment

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

well, architecturally, this PR is a regression because it moves further away from dedicated serializers - we could do the right thing here and introduce the KeystoreJson already - otherwise, it deserves documenting at least, because it's a fragile setup causing easily preventable bugs

Copy link
Member

Choose a reason for hiding this comment

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

in particular, the outright recommendation to use Json.encode is simply poor advice - it's one hack/workaround replacing another

Copy link
Contributor Author

@zah zah Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

In case that's not clear, we have been using Json.decode for keystores all along. The encoding/decoding routines were defined before the support for flavours were added to the JSON library and they are tested in the test_keystore.nim module. This PR is certainly not a regression as it removes an unnecessary duplicated definition that should not have been created at all. It also addresses a very specific issue, encountered by our end users.

I don't disagree regarding introducing KeystoreJson, but this can happen in another PR.

"There is no REST-specific encoding" .}

proc readValue*(reader: var JsonReader[RestJson], value: var Keystore) {.
raises: [SerializationError, IOError, Defect].} =
var
crypto: Option[Crypto]
description: Option[string]
pubkey: Option[ValidatorPubKey]
path: Option[KeyPath]
uuid: Option[string]
version: Option[int]

for fieldName in readObjectFields(reader):
case fieldName
of "crypto":
if crypto.isSome():
reader.raiseUnexpectedField("Multiple `crypto` fields found",
"Keystore")
crypto = some(reader.readValue(Crypto))
of "description":
let res = reader.readValue(string)
if description.isSome():
description = some(description.get() & "\n" & res)
else:
description = some(res)
of "pubkey":
if pubkey.isSome():
reader.raiseUnexpectedField("Multiple `pubkey` fields found",
"Keystore")
pubkey = some(reader.readValue(ValidatorPubKey))
of "path":
if path.isSome():
reader.raiseUnexpectedField("Multiple `path` fields found",
"Keystore")
let res = validateKeyPath(reader.readValue(string))
if res.isErr():
reader.raiseUnexpectedValue("Invalid `path` value")
path = some(res.get())
of "uuid":
if uuid.isSome():
reader.raiseUnexpectedField("Multiple `uuid` fields found",
"Keystore")
uuid = some(reader.readValue(string))
of "version":
if version.isSome():
reader.raiseUnexpectedField("Multiple `version` fields found",
"Keystore")
let res = reader.readValue(int)
if res < 0:
reader.raiseUnexpectedValue("Unexpected negative `version` value")
version = some(res)
else:
unrecognizedFieldWarning()

if crypto.isNone():
reader.raiseUnexpectedValue("Field `crypto` is missing")
if pubkey.isNone():
reader.raiseUnexpectedValue("Field `pubkey` is missing")
if path.isNone():
reader.raiseUnexpectedValue("Field `path` is missing")
if uuid.isNone():
reader.raiseUnexpectedValue("Field `uuid` is missing")
if version.isNone():
reader.raiseUnexpectedValue("Field `version` is missing")

value = Keystore(
crypto: crypto.get(),
pubkey: pubkey.get(),
path: path.get(),
uuid: uuid.get(),
description: description,
version: version.get(),
)
error: "Keystores must be loaded with `parseKeystore`. " &
"There is no REST-specific encoding".}

## KeystoresAndSlashingProtection
proc writeValue*(writer: var JsonWriter[RestJson],
value: KeystoresAndSlashingProtection) {.
raises: [IOError, Defect].} =
raises: [IOError, SerializationError, Defect].} =
writer.beginRecord()
let keystores =
block:
var res: seq[string]
for keystore in value.keystores:
let encoded = RestJson.encode(keystore)
let encoded = Json.encode(keystore)
res.add(encoded)
res
writer.writeField("keystores", keystores)
Expand Down Expand Up @@ -2171,10 +2094,7 @@ proc readValue*(reader: var JsonReader[RestJson],
for item in strKeystores:
let key =
try:
RestJson.decode(item,
Keystore,
requireAllFields = true,
allowUnknownFields = true)
parseKeystore(item)
except SerializationError as exc:
# TODO re-raise the exception by adjusting the column index, so the user
# will get an accurate syntax error within the larger message
Expand Down Expand Up @@ -2247,7 +2167,7 @@ proc readValue*(reader: var JsonReader[RestJson],
active: active.get())

proc dump*(value: KeystoresAndSlashingProtection): string {.
raises: [IOError, Defect].} =
raises: [IOError, SerializationError, Defect].} =
var stream = memoryOutput()
var writer = JsonWriter[RestJson].init(stream)
writer.writeValue(value)
Expand Down
8 changes: 6 additions & 2 deletions beacon_chain/validators/keystore_management.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,9 @@ proc loadNetKeystore*(keystorePath: string,

let keyStore =
try:
Json.loadFile(keystorePath, NetKeystore)
Json.loadFile(keystorePath, NetKeystore,
requireAllFields = true,
allowUnknownFields = true)
except IOError as err:
error "Failed to read network keystore", err = err.msg,
path = keystorePath
Expand Down Expand Up @@ -1483,7 +1485,9 @@ proc importKeystoresFromDir*(rng: var HmacDrbgContext,

let keystore =
try:
Json.loadFile(file, Keystore)
Json.loadFile(file, Keystore,
requireAllFields = true,
allowUnknownFields = true)
except SerializationError as e:
warn "Invalid keystore", err = e.formatMsg(file)
continue
Expand Down
2 changes: 1 addition & 1 deletion tests/test_keymanager_api.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,7 +641,7 @@ proc runTests(keymanager: KeymanagerToTest) {.async.} =
)

const
Vector1 = r"{""keystores"":[""{\""crypto\"":{\""kdf\"":{\""function\"":\""pbkdf2\"",\""params\"":{\""dklen\"":32,\""c\"":262144,\""prf\"":\""hmac-sha256\"",\""salt\"":\""d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3\""},\""message\"":\""\""},\""checksum\"":{\""function\"":\""sha256\"",\""params\"":{},\""message\"":\""0x88c0059314a3db1b2e86d4b0d37ac7ade7c6e56e3d3e34af298254f35c8b501e\""},\""cipher\"":{\""function\"":\""aes-128-ctr\"",\""params\"":{\""iv\"":\""264daa3f303d7259501c93d997d84fe6\""},\""message\"":\""c071f12ec97eb449422de643e737924e02eec266f3b56cde476eae4fad5c6e64\""}},\""description\"":\""Test keystore\"",\""pubkey\"":\""0xb4102a1f6c80e5c596a974ebd930c9f809c3587dc4d1d3634b77ff66db71e376dbc86c3252c6d140ce031f4ec6167798\"",\""path\"":\""m/12381/60/0/0\"",\""uuid\"":\""a3331c0c-a013-4754-a122-9988b3381fec\"",\""version\"":4}""],""passwords"":[""7465737470617373776f7264f09f9491""]}"
Vector1 = r"{""keystores"":[""{\""crypto\"":{\""kdf\"":{\""function\"":\""pbkdf2\"",\""params\"":{\""dklen\"":32,\""c\"":262144,\""prf\"":\""hmac-sha256\"",\""salt\"":\""d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3\""},\""message\"":\""\""},\""checksum\"":{\""function\"":\""sha256\"",\""params\"":{},\""message\"":\""0x88c0059314a3db1b2e86d4b0d37ac7ade7c6e56e3d3e34af298254f35c8b501e\""},\""cipher\"":{\""function\"":\""aes-128-ctr\"",\""params\"":{\""iv\"":\""264daa3f303d7259501c93d997d84fe6\""},\""message\"":\""c071f12ec97eb449422de643e737924e02eec266f3b56cde476eae4fad5c6e64\""}},\""description\"":\""Test keystore\"",\""pubkey\"":\""0xb4102a1f6c80e5c596a974ebd930c9f809c3587dc4d1d3634b77ff66db71e376dbc86c3252c6d140ce031f4ec6167798\"",\""path\"":\""m/12381/60/0/0\"",\""uuid\"":\""a3331c0c-a013-4754-a122-9988b3381fec\"",\""name\"":\""named-a3331c0c-a013-4754-a122-9988b3381fec\"",\""version\"":4}""],""passwords"":[""7465737470617373776f7264f09f9491""]}"
Vector2 = r"{""keystores"":[""{\""crypto\"":{\""kdf\"":{\""function\"":\""pbkdf2\"",\""params\"":{\""dklen\"":32,\""c\"":262144,\""prf\"":\""hmac-sha256\"",\""salt\"":\""d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3\""},\""message\"":\""\""},\""checksum\"":{\""function\"":\""sha256\"",\""params\"":{},\""message\"":\""0x88c0059314a3db1b2e86d4b0d37ac7ade7c6e56e3d3e34af298254f35c8b501e\""},\""cipher\"":{\""function\"":\""aes-128-ctr\"",\""params\"":{\""iv\"":\""264daa3f303d7259501c93d997d84fe6\""},\""message\"":\""c071f12ec97eb449422de643e737924e02eec266f3b56cde476eae4fad5c6e64\""}},\""description\"":\""Test keystore\"",\""pubkey\"":\""0xb4102a1f6c80e5c596a974ebd930c9f809c3587dc4d1d3634b77ff66db71e376dbc86c3252c6d140ce031f4ec6167798\"",\""path\"":\""m/12381/60/0/0\"",\""uuid\"":\""a3331c0c-a013-4754-a122-9988b3381fec\"",\""version\"":4}"",""{\""crypto\"":{\""kdf\"":{\""function\"":\""pbkdf2\"",\""params\"":{\""dklen\"":32,\""c\"":262144,\""prf\"":\""hmac-sha256\"",\""salt\"":\""d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3\""},\""message\"":\""\""},\""checksum\"":{\""function\"":\""sha256\"",\""params\"":{},\""message\"":\""0xadb59d10d2436c12f2fe229f27ec598739da92686485e9fed5255d3ed9bb1c1f\""},\""cipher\"":{\""function\"":\""aes-128-ctr\"",\""params\"":{\""iv\"":\""264daa3f303d7259501c93d997d84fe6\""},\""message\"":\""8d192da5a06c001eca9c954812ce165d007c889d7711b12faa7a9d6f4d5cc6ae\""}},\""description\"":\""Test keystore\"",\""pubkey\"":\""0xa00d2954717425ce047e0928e5f4ec7c0e3bbe1058db511303fd659770ddace686ee2e22ac180422e516f4c503eb2228\"",\""path\"":\""m/12381/60/0/0\"",\""uuid\"":\""905dd873-48af-416a-8c80-4283d5af84f9\"",\""version\"":4}""],""passwords"":[""7465737470617373776f7264f09f9491"",""7465737470617373776f7264f09f9491""]}"
Vector3 = r"{""keystores"":[""{\""crypto\"":{\""kdf\"":{\""function\"":\""scrypt\"",\""params\"":{\""dklen\"":32,\""n\"":262144,\""p\"":1,\""r\"":8,\""salt\"":\""d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3\""},\""message\"":\""\""},\""checksum\"":{\""function\"":\""sha256\"",\""params\"":{},\""message\"":\""0xea4d7f495ac74bbf431ef340f15ee1aea75811bd1bab8dd64b3c2dfc041d5d90\""},\""cipher\"":{\""function\"":\""aes-128-ctr\"",\""params\"":{\""iv\"":\""264daa3f303d7259501c93d997d84fe6\""},\""message\"":\""c40a44096120e406a011ec5a22d7cbb24126436c471e21b10f078c722c6d0c3f\""}},\""description\"":\""Test keystore\"",\""pubkey\"":\""0xb4102a1f6c80e5c596a974ebd930c9f809c3587dc4d1d3634b77ff66db71e376dbc86c3252c6d140ce031f4ec6167798\"",\""path\"":\""m/12381/60/0/0\"",\""uuid\"":\""ad1bf334-faaa-4257-8e28-81a45722e87b\"",\""version\"":4}""],""passwords"":[""7465737470617373776f7264f09f9491""]}"
Vector4 = r"{""keystores"":[""{\""crypto\"":{\""kdf\"":{\""function\"":\""scrypt\"",\""params\"":{\""dklen\"":32,\""n\"":262144,\""p\"":1,\""r\"":8,\""salt\"":\""d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3\""},\""message\"":\""\""},\""checksum\"":{\""function\"":\""sha256\"",\""params\"":{},\""message\"":\""0xea4d7f495ac74bbf431ef340f15ee1aea75811bd1bab8dd64b3c2dfc041d5d90\""},\""cipher\"":{\""function\"":\""aes-128-ctr\"",\""params\"":{\""iv\"":\""264daa3f303d7259501c93d997d84fe6\""},\""message\"":\""c40a44096120e406a011ec5a22d7cbb24126436c471e21b10f078c722c6d0c3f\""}},\""description\"":\""Test keystore\"",\""pubkey\"":\""0xb4102a1f6c80e5c596a974ebd930c9f809c3587dc4d1d3634b77ff66db71e376dbc86c3252c6d140ce031f4ec6167798\"",\""path\"":\""m/12381/60/0/0\"",\""uuid\"":\""ad1bf334-faaa-4257-8e28-81a45722e87b\"",\""version\"":4}"",""{\""crypto\"":{\""kdf\"":{\""function\"":\""scrypt\"",\""params\"":{\""dklen\"":32,\""n\"":262144,\""p\"":1,\""r\"":8,\""salt\"":\""d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3\""},\""message\"":\""\""},\""checksum\"":{\""function\"":\""sha256\"",\""params\"":{},\""message\"":\""0x71ed99dab563f1e9f1190b0de9d92d3266df2223036e7dc3ca9d9599478fe5a4\""},\""cipher\"":{\""function\"":\""aes-128-ctr\"",\""params\"":{\""iv\"":\""264daa3f303d7259501c93d997d84fe6\""},\""message\"":\""896298820832505128a09f51d72e4fa143b40997c3bafc40e213bf52cc6da4f5\""}},\""description\"":\""Test keystore\"",\""pubkey\"":\""0xa00d2954717425ce047e0928e5f4ec7c0e3bbe1058db511303fd659770ddace686ee2e22ac180422e516f4c503eb2228\"",\""path\"":\""m/12381/60/0/0\"",\""uuid\"":\""d91bcde8-8bf5-45c6-b04d-c10d99ae9b6b\"",\""version\"":4}""],""passwords"":[""7465737470617373776f7264f09f9491"",""7465737470617373776f7264f09f9491""]}"
Expand Down