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

dkg: add --no-verify for hash verifications #1090

Closed
wants to merge 5 commits into from
Closed

dkg: add --no-verify for hash verifications #1090

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1f2656b
add no-verify for hash verifications
dB2510 Sep 7, 2022
9d9cb3d
cleanup
dB2510 Sep 7, 2022
0dd850c
cleanup
dB2510 Sep 7, 2022
821adb4
cleanup
dB2510 Sep 8, 2022
aeae27b
cleanup
dB2510 Sep 8, 2022
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
104 changes: 65 additions & 39 deletions cluster/definition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,58 @@ func (d Definition) Verify() error {
return nil
}

func (d Definition) VerifyHashesJSON(data []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

this API doesn't feel good. Needing to pass the raw bytes again doesn't feel intuitive.

I would go for something like:

var d Definition
err = json.Unmarshal(rawBytes, &d)

err = d.VerifyHashes()

For this to work, you need to unmarshal the hashes into private unexported fields so you can calculate the actual hashes and compare.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was also thinking the same to include this as private fields but was hesitant to do any changes in Definition type

Copy link
Contributor Author

@dB2510 dB2510 Sep 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

for this to work we would need to do json marshal every time when new Definition is called, this doesn't feel right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggest: keep VerifyHashesJSON(data []byte) error but as stateless function which would introduce two function parameters as VerifyHashesJSON(rawBytes, configHash, defHash []byte) error ?

var configHashJSON, definitionHashJSON []byte
switch {
case isJSONv1x1(d.Version):
hashes := struct {
ConfigHash []byte `json:"config_hash"`
DefinitionHash []byte `json:"definition_hash"`
}{}
if err := json.Unmarshal(data, &hashes); err != nil {
return errors.Wrap(err, "unmarshal hashes")
}

configHashJSON = hashes.ConfigHash
definitionHashJSON = hashes.DefinitionHash
case isJSONv1x2(d.Version):
hashes := struct {
ConfigHash ethHex `json:"config_hash"`
DefinitionHash ethHex `json:"definition_hash"`
}{}
if err := json.Unmarshal(data, &hashes); err != nil {
return errors.Wrap(err, "unmarshal hashes")
}

configHashJSON = hashes.ConfigHash
definitionHashJSON = hashes.DefinitionHash
default:
return errors.New("unsupported version")
}

// Verify config_hash
confHash, err := d.ConfigHash()
if err != nil {
return errors.Wrap(err, "config hash")
}

if !bytes.Equal(configHashJSON, confHash[:]) {
return errors.New("invalid config hash")
}

// Verify definition_hash
defHash, err := d.HashTreeRoot()
if err != nil {
return errors.Wrap(err, "definition hash")
}

if !bytes.Equal(definitionHashJSON, defHash[:]) {
return errors.New("invalid definition hash")
}

return nil
}

// ConfigHash returns the config hash of the definition object.
func (d Definition) ConfigHash() ([32]byte, error) {
return configHash(d)
Expand Down Expand Up @@ -316,46 +368,24 @@ func (d *Definition) UnmarshalJSON(data []byte) error {
}

var (
def Definition
configHashJSON []byte
defHashJSON []byte
err error
def Definition
err error
)
switch {
case isJSONv1x1(version.Version):
def, configHashJSON, defHashJSON, err = unmarshalDefinitionV1x1(data)
def, err = unmarshalDefinitionV1x1(data)
if err != nil {
return err
}
case isJSONv1x2(version.Version):
def, configHashJSON, defHashJSON, err = unmarshalDefinitionV1x2(data)
def, err = unmarshalDefinitionV1x2(data)
if err != nil {
return err
}
default:
return errors.New("unsupported version")
}

// Verify config_hash
configHash, err := def.ConfigHash()
if err != nil {
return errors.Wrap(err, "config hash")
}

if !bytes.Equal(configHashJSON, configHash[:]) {
return errors.New("invalid config hash")
}

// Verify definition_hash
defHash, err := def.HashTreeRoot()
if err != nil {
return errors.Wrap(err, "definition hash")
}

if !bytes.Equal(defHashJSON, defHash[:]) {
return errors.New("invalid definition hash")
}

*d = def

return nil
Expand Down Expand Up @@ -407,18 +437,18 @@ func marshalDefinitionV1x2(def Definition, configHash, defHash [32]byte) ([]byte
return resp, nil
}

func unmarshalDefinitionV1x1(data []byte) (def Definition, configHashJSON, defHashJSON []byte, err error) {
func unmarshalDefinitionV1x1(data []byte) (Definition, error) {
var defJSON definitionJSONv1x1
if err := json.Unmarshal(data, &defJSON); err != nil {
return Definition{}, nil, nil, errors.Wrap(err, "unmarshal definition v1_1")
return Definition{}, errors.Wrap(err, "unmarshal definition v1_1")
}

operators, err := operatorsFromV1x1(defJSON.Operators)
if err != nil {
return Definition{}, nil, nil, err
return Definition{}, err
}

def = Definition{
return Definition{
Name: defJSON.Name,
UUID: defJSON.UUID,
Version: defJSON.Version,
Expand All @@ -430,18 +460,16 @@ func unmarshalDefinitionV1x1(data []byte) (def Definition, configHashJSON, defHa
DKGAlgorithm: defJSON.DKGAlgorithm,
ForkVersion: defJSON.ForkVersion,
Operators: operators,
}

return def, defJSON.ConfigHash, defJSON.DefinitionHash, nil
}, nil
}

func unmarshalDefinitionV1x2(data []byte) (def Definition, configHashJSON, defHashJSON []byte, err error) {
func unmarshalDefinitionV1x2(data []byte) (Definition, error) {
var defJSON definitionJSONv1x2
if err := json.Unmarshal(data, &defJSON); err != nil {
return Definition{}, nil, nil, errors.Wrap(err, "unmarshal definition v1v2")
return Definition{}, errors.Wrap(err, "unmarshal definition v1v2")
}

def = Definition{
return Definition{
Name: defJSON.Name,
UUID: defJSON.UUID,
Version: defJSON.Version,
Expand All @@ -453,9 +481,7 @@ func unmarshalDefinitionV1x2(data []byte) (def Definition, configHashJSON, defHa
DKGAlgorithm: defJSON.DKGAlgorithm,
ForkVersion: defJSON.ForkVersion,
Operators: operatorsFromV1x2(defJSON.Operators),
}

return def, defJSON.ConfigHash, defJSON.DefinitionHash, nil
}, nil
}

// definitionJSONv1x1 is the json formatter of Definition for versions v1.0.0 and v1.1.1.
Expand Down
8 changes: 8 additions & 0 deletions dkg/disk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"github.com/coinbase/kryptology/pkg/signatures/bls/bls_sig"

"github.com/obolnetwork/charon/app/errors"
"github.com/obolnetwork/charon/app/log"
"github.com/obolnetwork/charon/app/z"
"github.com/obolnetwork/charon/cluster"
"github.com/obolnetwork/charon/core"
Expand Down Expand Up @@ -59,6 +60,13 @@ func loadDefinition(ctx context.Context, conf Config) (cluster.Definition, error
return cluster.Definition{}, errors.Wrap(err, "unmarshal definition")
}

// Verify config hash and definition hash from json string and resultant cluster.Definition.
if err := res.VerifyHashesJSON(buf); err != nil && !conf.NoVerify {
return cluster.Definition{}, errors.Wrap(err, "cluster definition hash verification failed. Run with --no-verify to bypass verification at own risk")
} else if err != nil && conf.NoVerify {
log.Warn(ctx, "Ignoring failed cluster definition hash verification due to --no-verify flag", err)
}

return res, nil
}

Expand Down
67 changes: 47 additions & 20 deletions dkg/disk_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,43 +91,70 @@ func TestLoadDefinition(t *testing.T) {

// Invalid definition
invalidDef := cluster.Definition{}
invalidFile := "invalid-cluster-definition.json"
err = os.WriteFile(invalidFile, []byte{1, 2, 3}, 0o666)
invalidFile1 := "invalid-cluster-definition.json"
err = os.WriteFile(invalidFile1, []byte{1, 2, 3}, 0o666)
require.NoError(t, err)

// Invalid definition without definition_hash and config_hash
invalidFile2 := "invalid-cluster-definition2.json"
defJSON := []byte(`{"name":"test cluster","operators":[{"address":"0x9A4C8145c7457b0BDC84Ba46729c3c9e15b56106","enr":"enr:-Iu4QFTSWOu_OplK-CYUv29EqIoMGQGtuHjTxLohMxOEMSxYFqraJdtWfMiwzS9wiGH-gB32IrYdyXSH-i5nJbLTm4yAgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQJ9hgLS0tOo-w3eLfoHVnENCJuN5QXDgtCX_cHQo5FpDoN0Y3CC52eDdWRwgudo","config_signature":"0x84b5e48201deebc59ac09c2aaec57870cad357d5a0b65a1954ee301b2760597d479a8ee2b6037963554b5323fb7944a389b9e86948ad89da08d6fffcc4ba5c5a01","enr_signature":"0x1530ab4bf5267f88c76850f9750e328698e0206f70a29bf2a6136cec3ffc365e620bddcd46ecfe667718dd6770b38c7c4c7bdf1e6c8ecaf0bc098d3959d9ae0c00"},{"address":"0x79AB788F445d5A689C34AD6e54e247865DE41E99","enr":"enr:-Iu4QDKAQ7dsqHud5m1T2FsjYcahgYRrzMiCZjjx9sRTOjnWH67n8ZEepVZ4WHp-XNn0c0CtFIB-KSBHeiKe8oDLztiAgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQPFe0POIvtf3fkTXUjkj7yLDJ2APptRF6CK8_9C8g0NvYN0Y3CC52mDdWRwgudq","config_signature":"0xc8a306ec102a83dce35be9a552d0b30a09e74b1c1b2316596245c0e8f2ffa5d063051373993ecb694d82e2d65a398ecd5ef06599661903470385998d19a702f001","enr_signature":"0x61c44ee84dfc3991fe9de0d25f2bc3cad7feb0d3341208c894f455a233bcb1d2647b4fd29c376ceb2e5d2990fb2af7b6eae10bfe5b55963404ea2571222b350c01"},{"address":"0xfDdd1CF7733Fd8a638020e963792f9Fcc0182Bf4","enr":"enr:-Iu4QNgEtRy6wbpdPCXrj52_rF4Ur7OQf6mOg1xfRpmzPgRYW-QSA-oUslOTmIPL8etUIg95quQoRJg9FIILIa6990uAgmlkgnY0gmlwhH8AAAGJc2VjcDI1NmsxoQLM5474DZHwbqwSbQFLrAO8PNh2AdZOXTYGy1ZItyDJaoN0Y3CC52uDdWRwguds","config_signature":"0xddb059b67e0603c0073536225e3a5ae5a7eeae178ec4c31dda521ed9b0209dc90b7e3295ee5ad4f35fbe0b8d62adf4aa4cfbddd3106f7563d234e0b134d9e93701","enr_signature":"0x3c556e60c4b44a4ea015f860759fa01e2e8664d105455b71e55845f451d7ec5e048ee56999c9f5333024bfcc087471fcb7d8e8a2685cb59d63266b4df092104501"}],"uuid":"04513690-AA41-CE01-6281-7901E9FB6D87","version":"v1.2.0","timestamp":"2022-09-07T18:46:30+05:30","num_validators":1,"threshold":2,"fee_recipient_address":"0xd805a5fcea20d3d27d3eee59d5dd5749e3271617","withdrawal_address":"0x75e896f172869cf3ade31c97f681cc1a4015ceed","dkg_algorithm":"default","fork_version":"0x00000000"}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

not a fan of this wall of text. Making changes to this is impossible. Rather:

  • unmarshal valid definition above from disk into map[string]any
  • delete the config and definition hashes
  • marshal to disk

err = os.WriteFile(invalidFile2, defJSON, 0o666)
require.NoError(t, err)
var invalidDef2 cluster.Definition
require.NoError(t, invalidDef2.UnmarshalJSON(defJSON))

defer func() {
require.NoError(t, os.Remove(validFile))
require.NoError(t, os.Remove(invalidFile))
require.NoError(t, os.Remove(invalidFile1))
require.NoError(t, os.Remove(invalidFile2))
}()

tests := []struct {
name string
defFile string
want cluster.Definition
wantErr bool
name string
defFile string
want cluster.Definition
noVerify bool
wantErr bool
}{
{
name: "Load valid definition",
defFile: validFile,
want: validDef,
wantErr: false,
name: "Load valid definition",
defFile: validFile,
want: validDef,
noVerify: false,
wantErr: false,
},
{
name: "Definition file doesn't exist",
defFile: "",
want: invalidDef,
wantErr: true,
name: "Definition file doesn't exist",
defFile: "",
want: invalidDef,
noVerify: false,
wantErr: true,
},
{
name: "Load invalid definition",
defFile: invalidFile,
want: invalidDef,
wantErr: true,
name: "Load invalid definition",
defFile: invalidFile1,
want: invalidDef,
noVerify: false,
wantErr: true,
},
{
name: "Load invalid definition with no verify",
defFile: invalidFile2,
want: invalidDef2,
noVerify: true,
wantErr: false,
},
{
name: "Load invalid definition without no verify",
defFile: invalidFile2,
want: invalidDef2,
noVerify: false,
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got, err := loadDefinition(context.Background(), Config{DefFile: tt.defFile})
got, err := loadDefinition(context.Background(), Config{DefFile: tt.defFile, NoVerify: tt.noVerify})
if tt.wantErr {
require.Error(t, err)
return
Expand Down