Server-side move regression "EncryptedMetadataKeyNonce is missing"

[mniewrzal]

After recent changes we have regression in server-side move implementation. User is getting "EncryptedMetadataKeyNonce is missing" error while doing move. Test case to reproduce.

func TestMove(t *testing.T) {
    testplanet.Run(t, testplanet.Config{
        SatelliteCount:   1,
        StorageNodeCount: 0,
        UplinkCount:      1,
    }, func(t *testing.T, ctx *testcontext.Context, planet *testplanet.Planet) {
        // old upling is uploading object and moving it
        // new uplink should be able to list it

        for _, version := range []string{"1.40.4", "1.49.5"} {
            t.Run(version, func(t *testing.T) {
                bucket := "bucket"
                cmd := exec.Command("go", "install", "storj.io/storj/cmd/uplink@v"+version)
                cmd.Env = os.Environ()
                cmd.Env = append(cmd.Env, "GOBIN="+ctx.Dir("binary"))
                output, err := cmd.CombinedOutput()
                t.Log(string(output))
                require.NoError(t, err)

                err = planet.Uplinks[0].CreateBucket(ctx, planet.Satellites[0], bucket)
                require.NoError(t, err)

                expectedData := testrand.Bytes(1 * memory.KiB)
                srcFile := ctx.File("src")

                err = ioutil.WriteFile(srcFile, expectedData, 0644)
                require.NoError(t, err)

                access, err := planet.Uplinks[0].Access[planet.Satellites[0].ID()].Serialize()
                require.NoError(t, err)

                project, err := planet.Uplinks[0].OpenProject(ctx, planet.Satellites[0])
                require.NoError(t, err)
                defer ctx.Check(project.Close)

                runBinary := func(args ...string) {
                    output, err = exec.Command(ctx.File("binary", "uplink"), args...).CombinedOutput()
                    t.Log(string(output))
                    require.NoError(t, err)
                }

                // upload with old uplink
                runBinary("cp", srcFile, fmt.Sprintf("sj://%s/move/old-uplink", bucket), "--access="+access)

                // move with old uplink
                runBinary("mv", fmt.Sprintf("sj://%s/move/old-uplink", bucket), fmt.Sprintf("sj://%s/move/old-uplink-moved", bucket), "--access="+access)

                testit := func(key string) {
                    cases := []uplink.ListObjectsOptions{
                        {System: false, Custom: false},
                        {System: true, Custom: false},
                        {System: false, Custom: true},
                        {System: true, Custom: true},
                    }

                    for _, tc := range cases {
                        tc.Prefix = "move/"
                        iterator := project.ListObjects(ctx, bucket, &tc)
                        require.True(t, iterator.Next())
                        require.Equal(t, key, iterator.Item().Key)
                        require.NoError(t, iterator.Err())
                    }
                }

                testit("move/old-uplink-moved")

                // move with old uplink second time
                runBinary("mv", fmt.Sprintf("sj://%s/move/old-uplink-moved", bucket), fmt.Sprintf("sj://%s/move/old-uplink-moved-second", bucket), "--access="+access)

                testit("move/old-uplink-moved-second")

            })
        }
    })
}

We should also verify if similar problem is visible for server-side copy.

Initially reported in https://github.com/storj/customer-issues/issues/50

[mniewrzal]

so to fix the problem we cannot do anything in BeginMoveObject or in uplink because we cannot fix existing uplink release and everything that will be sent to uplink from BeginMoveObject will be broken when metadata is empty (existing key and empty nonce). Removing this two checks (key and nonce) in FinishMoveObject will work but with this there is a chance that in future we can introduce another bug in uplink and we will start sending some broken metadata key and nonce. I'm not 100% sure if thats the best solution but I would:

• remove FinishMoveObject check for key and nonce
• in SQL in FinishMoveObject (UPDATE objects SET...) we can do something like if metadata is nil then don't set key and nonce
• the same UPDATE clause should return metadata and if metadata != nil we should do the same validation for key and nonce in Verify method to avoid putting broken key and nonce while doing move

[storjBuildBot]

Change https://review.dev.storj.io/c/storj/storj/+/7713 mentions this issue.

[heunland]

Looks like https://review.dev.storj.io/c/storj/storj/+/7713 has been merged. @mniewrzal could you please confirm if this will be in the next release?

[Qweder93]

@heunland Michal is in PTO, but @andriikotko confirmed that it will be there