crypto: TRC update validation #2860

oncilla · 2019-07-17T13:08:36Z

This PR introduces TRC update validation.
The UpdateValidator checks that the new TRC contains valid contents
and the update is valid according to the regular/sensitive rules.

The caller has to verify the signatures itself. It must ensure that
all signatures mentioned in TRC.Votes are present and verifiable.

fixes #2849

This change is

lukedirtwalker

Reviewed 4 of 5 files at r1.
Reviewable status: 4 of 5 files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker and @oncilla)

go/lib/scrypto/trc/v2/keychanges.go, line 28 at r1 (raw file):

	InvalidKeyMeta = "invalid key meta"
	// InvalidKeyVersion indicates an invalid key version.
	InvalidKeyVersion = "invalid key version"

This error is always nested in InvalidKeyMeta not sure if that is intended?

go/lib/scrypto/trc/v2/keychanges.go, line 62 at r1 (raw file):

}

func (c *KeyChanges) insertAllFresh(as addr.AS, next PrimaryAS) {

I think this is not needed see comment in update.go.

go/lib/scrypto/trc/v2/keychanges.go, line 93 at r1 (raw file):

	modified := next.Algorithm != prev.Algorithm || !bytes.Equal(next.Key, prev.Key)
	// If the meta data has changed, expect a key version change.
	expectedVersion := prev.KeyVersion

possible alternative, not sure what is better:

	switch {
	case modified && next.KeyVersion != prev.KeyVersion+1:
		return modified, common.NewBasicError(InvalidKeyVersion, nil, "modified", modified,
			"expected", prev.KeyVersion+1, "actual", next.KeyVersion)
	case !modified && next.KeyVersion != prev.KeyVersion:
		return modified, common.NewBasicError(InvalidKeyVersion, nil, "modified", modified,
			"expected", prev.KeyVersion, "actual", next.KeyVersion)
	default:
		return modified, nil
	}

feel free to leave as is.

go/lib/scrypto/trc/v2/update.go, line 141 at r1 (raw file):

	for as, primary := range v.Next.PrimaryASes {
		prev, ok := v.Prev.PrimaryASes[as]
		if !ok {

This is not really needed:

		prev := v.Prev.PrimaryASes[as]
		if err := c.insertModifications(as, prev, primary); err != nil {
			return nil, err
		}

is good enough since prev will be the empty value and prev.Keys[...] will always return !ok and inserModifications checks this.

go/lib/scrypto/trc/v2/update.go, line 157 at r1 (raw file):

	for as, primary := range v.Next.PrimaryASes {
		prev, ok := v.Prev.PrimaryASes[as]
		if !ok {

can also be simplified to:

	for as, primary := range v.Next.PrimaryASes {
		prev := v.Prev.PrimaryASes[as]
		c.insertModifications(as, prev, primary)
	}

insertAll is not needed.

That's the cool thing that we work with values here 💯

go/lib/scrypto/trc/v2/update.go, line 188 at r1 (raw file):

func (v *UpdateValidator) checkVotes(info UpdateInfo) error {
	check := v.checkVotesSensitive

alternative

	var err error
	switch info.Type {
	case RegularUpdate:
		err = v.checkVotesRegular(info)
	default:
		err = v.checkVotesSensitive(info)
	}
	if err != nil {
		return err
	}

I feel this might be slightly clearer. But I'm also fine with the current version

go/lib/scrypto/trc/v2/update.go, line 269 at r1 (raw file):

}

func (c AttributeChanges) insertAll(as addr.AS, attrs Attributes, change AttributeChange) {

not needed see comment above

lukedirtwalker

Reviewed 1 of 5 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @oncilla)

go/lib/scrypto/trc/v2/update_test.go, line 30 at r1 (raw file):

// TestCommonUpdate tests shared error cases between regular and sensitive updates.
func TestCommonUpdate(t *testing.T) {

I think somewhere you should also test that the TRCs as created are indeed a valid update pair, as that is kind of the base assumption for the other tests.

go/lib/scrypto/trc/v2/update_test.go, line 39 at r1 (raw file):

				*updated = *newBaseTRC()
				updated.Version = 2
				updated.BaseVersion = 2

Just setting updated.BaseVersion = updated.Version would be a bit simpler.

go/lib/scrypto/trc/v2/update_test.go, line 51 at r1 (raw file):

		"Wrong ISD": {
			Modify: func(updated, _ *trc.TRC) {
				updated.ISD = 2

instead of 2 you could use prev.ISD + 1 it makes it a bit more flexible.

This PR introduces TRC update validation. The `UpdateValidator` checks that the new TRC contains valid contents and the update is valid according to the regular/sensitive rules. The caller has to verify the signatures itself. It must ensure that all signatures mentioned in TRC.Votes are present and verifiable.

oncilla

Reviewable status: 2 of 5 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker)

go/lib/scrypto/trc/v2/keychanges.go, line 28 at r1 (raw file):