From 8e3d6fcf565d90af707708c571f34eaa9cf9209f Mon Sep 17 00:00:00 2001 From: Maksim An Date: Thu, 23 Feb 2023 01:17:51 -0800 Subject: [PATCH 1/3] simplify zeroDevice to just zero first block Signed-off-by: Maksim An --- internal/guest/storage/crypt/crypt.go | 13 ++++--- internal/guest/storage/crypt/crypt_test.go | 12 +++--- internal/guest/storage/crypt/utilities.go | 43 ++++------------------ 3 files changed, 21 insertions(+), 47 deletions(-) diff --git a/internal/guest/storage/crypt/crypt.go b/internal/guest/storage/crypt/crypt.go index 064ee37d4e..7d378b03b9 100644 --- a/internal/guest/storage/crypt/crypt.go +++ b/internal/guest/storage/crypt/crypt.go @@ -23,8 +23,8 @@ var ( _getBlockDeviceSize = getBlockDeviceSize _osMkdirTemp = os.MkdirTemp _mkfsXfs = mkfsXfs - _zeroDevice = zeroDevice _osRemoveAll = os.RemoveAll + _zeroFirstBlock = zeroFirstBlock ) // cryptsetupCommand runs cryptsetup with the provided arguments @@ -172,13 +172,16 @@ func EncryptDevice(ctx context.Context, source string, dmCryptName string) (path return "", fmt.Errorf("error getting size of: %s: %w", deviceNamePath, err) } - if deviceSize == 0 { + if deviceSize < 4096 { return "", fmt.Errorf("invalid size obtained for: %s", deviceNamePath) } - // 4.1. Zero the first block. It appears that mkfs.xfs reads this before formatting. - if err = _zeroDevice(deviceNamePath, 4096, 1); err != nil { - return "", fmt.Errorf("failed zero'ing start of device %s: %w", deviceNamePath, err) + // 4.1. Zero the first block. + // In the xfs mkfs case it appears to attempt to read the first block of the device. + // This results in an integrity error. This function zeros out the start of the device, + // so we are sure that when it is read it has already been hashed so matches. + if err := _zeroFirstBlock(deviceNamePath, 4096); err != nil { + return "", fmt.Errorf("failed to zero first block: %w", err) } // 4.2. Format it as xfs diff --git a/internal/guest/storage/crypt/crypt_test.go b/internal/guest/storage/crypt/crypt_test.go index 627a2d5b7c..3d07fcb3f6 100644 --- a/internal/guest/storage/crypt/crypt_test.go +++ b/internal/guest/storage/crypt/crypt_test.go @@ -26,6 +26,7 @@ func clearCryptTestDependencies() { _getBlockDeviceSize = nil _osMkdirTemp = osMkdirTempTest _osRemoveAll = nil + _zeroFirstBlock = nil } func Test_Encrypt_Generate_Key_Error(t *testing.T) { @@ -203,10 +204,7 @@ func Test_Encrypt_Mkfs_Error(t *testing.T) { // Return a non-zero size return blockDeviceSize, nil } - _mkfsXfs = func(string) error { - return nil - } - _zeroDevice = func(string, int64, int64) error { + _zeroFirstBlock = func(_ string, _ int) error { return nil } @@ -221,8 +219,7 @@ func Test_Encrypt_Mkfs_Error(t *testing.T) { return expectedErr } - _, err := EncryptDevice(context.Background(), source, "dm-crypt-name") - if errors.Unwrap(err) != expectedErr { + if _, err := EncryptDevice(context.Background(), source, "dm-crypt-name"); errors.Unwrap(err) != expectedErr { t.Fatalf("expected err: '%v' got: '%v'", expectedErr, err) } } @@ -250,6 +247,9 @@ func Test_Encrypt_Success(t *testing.T) { // Return a non-zero size return blockDeviceSize, nil } + _zeroFirstBlock = func(_ string, _ int) error { + return nil + } _mkfsXfs = func(arg string) error { return nil } diff --git a/internal/guest/storage/crypt/utilities.go b/internal/guest/storage/crypt/utilities.go index 91926a2160..ecbb97234c 100644 --- a/internal/guest/storage/crypt/utilities.go +++ b/internal/guest/storage/crypt/utilities.go @@ -4,6 +4,7 @@ package crypt import ( + "bytes" "context" "crypto/rand" "fmt" @@ -33,46 +34,16 @@ func getBlockDeviceSize(ctx context.Context, path string) (int64, error) { return pos, nil } -// In the xfs mkfs case it appears to attempt to read the first block of the device. -// This results in an integrity error. This function zeros out the start of the device -// so we are sure that when it is read it has already been hashed so matches. -func zeroDevice(devicePath string, blockSize int64, numberOfBlocks int64) error { - fout, err := os.OpenFile(devicePath, os.O_WRONLY, 0) +func zeroFirstBlock(path string, blockSize int) error { + fout, err := os.OpenFile(path, os.O_WRONLY, 0) if err != nil { - return fmt.Errorf("failed to open device file %s: %w", devicePath, err) + return fmt.Errorf("failed to open file for zero'ing: %w", err) } defer fout.Close() - zeros := make([]byte, blockSize) - for i := range zeros { - zeros[i] = 0 - } - - // get the size so we don't overrun the end of the device - foutSize, err := fout.Seek(0, io.SeekEnd) - if err != nil { - return fmt.Errorf("zeroDevice: failed to seek to end, device file %s: %w", devicePath, err) - } - - // move back to the front. - _, err = fout.Seek(0, io.SeekStart) - if err != nil { - return fmt.Errorf("zeroDevice: failed to seek to start, device file %s: %w", devicePath, err) - } - - var offset int64 = 0 - var which int64 - for which = 0; which < numberOfBlocks; which++ { - // Exit when the end of the file is reached - if offset >= foutSize { - break - } - // Write data to destination file - written, err := fout.Write(zeros) - if err != nil { - return fmt.Errorf("failed to write destination file %s offset %d: %w", devicePath, offset, err) - } - offset += int64(written) + zeros := bytes.Repeat([]byte{0}, blockSize) + if _, err := fout.Write(zeros); err != nil { + return fmt.Errorf("failed writing zero bytes: %w", err) } return nil } From 6c0d57b5d1fdec90dc44de03b5d6e8d16f725a52 Mon Sep 17 00:00:00 2001 From: Maksim An Date: Thu, 23 Feb 2023 11:02:26 -0800 Subject: [PATCH 2/3] pr feedback: error message Signed-off-by: Maksim An --- internal/guest/storage/crypt/utilities.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/guest/storage/crypt/utilities.go b/internal/guest/storage/crypt/utilities.go index ecbb97234c..11483698c5 100644 --- a/internal/guest/storage/crypt/utilities.go +++ b/internal/guest/storage/crypt/utilities.go @@ -43,7 +43,7 @@ func zeroFirstBlock(path string, blockSize int) error { zeros := bytes.Repeat([]byte{0}, blockSize) if _, err := fout.Write(zeros); err != nil { - return fmt.Errorf("failed writing zero bytes: %w", err) + return fmt.Errorf("failed to zero-out bytes: %w", err) } return nil } From 69715cab2078da520ea0944a47e2d021ae9db88a Mon Sep 17 00:00:00 2001 From: Maksim An Date: Fri, 24 Feb 2023 11:41:41 -0800 Subject: [PATCH 3/3] pr feedback: remove redundant functions, simplify logic more Signed-off-by: Maksim An --- internal/guest/storage/crypt/crypt.go | 28 +++------ internal/guest/storage/crypt/crypt_test.go | 66 ---------------------- internal/guest/storage/crypt/utilities.go | 32 ++++------- 3 files changed, 19 insertions(+), 107 deletions(-) diff --git a/internal/guest/storage/crypt/crypt.go b/internal/guest/storage/crypt/crypt.go index 7d378b03b9..4bad3c4b95 100644 --- a/internal/guest/storage/crypt/crypt.go +++ b/internal/guest/storage/crypt/crypt.go @@ -16,15 +16,14 @@ import ( // Test dependencies var ( - _cryptsetupClose = cryptsetupClose - _cryptsetupFormat = cryptsetupFormat - _cryptsetupOpen = cryptsetupOpen - _generateKeyFile = generateKeyFile - _getBlockDeviceSize = getBlockDeviceSize - _osMkdirTemp = os.MkdirTemp - _mkfsXfs = mkfsXfs - _osRemoveAll = os.RemoveAll - _zeroFirstBlock = zeroFirstBlock + _cryptsetupClose = cryptsetupClose + _cryptsetupFormat = cryptsetupFormat + _cryptsetupOpen = cryptsetupOpen + _generateKeyFile = generateKeyFile + _osMkdirTemp = os.MkdirTemp + _mkfsXfs = mkfsXfs + _osRemoveAll = os.RemoveAll + _zeroFirstBlock = zeroFirstBlock ) // cryptsetupCommand runs cryptsetup with the provided arguments @@ -165,17 +164,6 @@ func EncryptDevice(ctx context.Context, source string, dmCryptName string) (path }() deviceNamePath := "/dev/mapper/" + dmCryptName - - // Get actual size of the scratch device - deviceSize, err := _getBlockDeviceSize(ctx, deviceNamePath) - if err != nil { - return "", fmt.Errorf("error getting size of: %s: %w", deviceNamePath, err) - } - - if deviceSize < 4096 { - return "", fmt.Errorf("invalid size obtained for: %s", deviceNamePath) - } - // 4.1. Zero the first block. // In the xfs mkfs case it appears to attempt to read the first block of the device. // This results in an integrity error. This function zeros out the start of the device, diff --git a/internal/guest/storage/crypt/crypt_test.go b/internal/guest/storage/crypt/crypt_test.go index 3d07fcb3f6..b05afb1559 100644 --- a/internal/guest/storage/crypt/crypt_test.go +++ b/internal/guest/storage/crypt/crypt_test.go @@ -5,10 +5,8 @@ package crypt import ( "context" - "fmt" "testing" - "github.com/Microsoft/hcsshim/internal/memory" "github.com/pkg/errors" ) @@ -23,7 +21,6 @@ func clearCryptTestDependencies() { _cryptsetupFormat = nil _cryptsetupOpen = nil _generateKeyFile = nil - _getBlockDeviceSize = nil _osMkdirTemp = osMkdirTempTest _osRemoveAll = nil _zeroFirstBlock = nil @@ -128,63 +125,11 @@ func Test_Encrypt_Cryptsetup_Open_Error(t *testing.T) { } } -func Test_Encrypt_Get_Device_Size_Error(t *testing.T) { - clearCryptTestDependencies() - - // Test what happens when cryptsetup fails to get the size of the - // unencrypted block device. - - _generateKeyFile = func(path string, size int64) error { - return nil - } - _osRemoveAll = func(path string) error { - return nil - } - _cryptsetupFormat = func(source string, keyFilePath string) error { - return nil - } - _cryptsetupOpen = func(source string, deviceName string, keyFilePath string) error { - return nil - } - _cryptsetupClose = func(deviceName string) error { - return nil - } - - source := "/dev/sda" - dmCryptName := "dm-crypt-target" - deviceNamePath := "/dev/mapper/" + dmCryptName - - expectedErr := errors.New("expected error message") - _getBlockDeviceSize = func(ctx context.Context, path string) (int64, error) { - return 0, expectedErr - } - - _, err := EncryptDevice(context.Background(), source, dmCryptName) - if errors.Unwrap(err) != expectedErr { - t.Fatalf("expected err: '%v' got: '%v'", expectedErr, err) - } - - // Check that it fails when the size of the block device is zero - - expectedErr = fmt.Errorf("invalid size obtained for: %s", deviceNamePath) - _getBlockDeviceSize = func(ctx context.Context, path string) (int64, error) { - return 0, nil - } - - _, err = EncryptDevice(context.Background(), source, dmCryptName) - if err.Error() != expectedErr.Error() { - t.Fatalf("expected err: '%v' got: '%v'", expectedErr, err) - } -} - func Test_Encrypt_Mkfs_Error(t *testing.T) { clearCryptTestDependencies() // Test what happens when mkfs fails to format the unencrypted device. // Verify that the arguments passed to it are the right ones. - - blockDeviceSize := int64(memory.GiB) - _generateKeyFile = func(path string, size int64) error { return nil } @@ -200,10 +145,6 @@ func Test_Encrypt_Mkfs_Error(t *testing.T) { _cryptsetupClose = func(deviceName string) error { return nil } - _getBlockDeviceSize = func(ctx context.Context, path string) (int64, error) { - // Return a non-zero size - return blockDeviceSize, nil - } _zeroFirstBlock = func(_ string, _ int) error { return nil } @@ -228,9 +169,6 @@ func Test_Encrypt_Success(t *testing.T) { clearCryptTestDependencies() // Test what happens when everything goes right. - - blockDeviceSize := int64(memory.GiB) - _generateKeyFile = func(path string, size int64) error { return nil } @@ -243,10 +181,6 @@ func Test_Encrypt_Success(t *testing.T) { _cryptsetupOpen = func(source string, deviceName string, keyFilePath string) error { return nil } - _getBlockDeviceSize = func(ctx context.Context, path string) (int64, error) { - // Return a non-zero size - return blockDeviceSize, nil - } _zeroFirstBlock = func(_ string, _ int) error { return nil } diff --git a/internal/guest/storage/crypt/utilities.go b/internal/guest/storage/crypt/utilities.go index 11483698c5..99562f75dd 100644 --- a/internal/guest/storage/crypt/utilities.go +++ b/internal/guest/storage/crypt/utilities.go @@ -5,41 +5,31 @@ package crypt import ( "bytes" - "context" "crypto/rand" "fmt" "io" "os" - - "github.com/Microsoft/hcsshim/internal/log" ) -// getBlockDeviceSize returns the size of the specified block device. -func getBlockDeviceSize(ctx context.Context, path string) (int64, error) { - file, err := os.Open(path) +func zeroFirstBlock(path string, blockSize int) error { + fout, err := os.OpenFile(path, os.O_WRONLY, 0) if err != nil { - return 0, fmt.Errorf("error opening %s: %w", path, err) + return fmt.Errorf("failed to open file for zero'ing: %w", err) } + defer fout.Close() - defer func() { - if err := file.Close(); err != nil { - log.G(ctx).WithError(err).Debug("error closing: " + path) - } - }() - - pos, err := file.Seek(0, io.SeekEnd) + size, err := fout.Seek(0, io.SeekEnd) if err != nil { - return 0, fmt.Errorf("error seeking end of %s: %w", path, err) + return fmt.Errorf("error seeking end of %s: %w", path, err) + } + if size < int64(blockSize) { + return fmt.Errorf("file size is smaller than minimum expected: %d < %d", size, blockSize) } - return pos, nil -} -func zeroFirstBlock(path string, blockSize int) error { - fout, err := os.OpenFile(path, os.O_WRONLY, 0) + _, err = fout.Seek(0, io.SeekStart) if err != nil { - return fmt.Errorf("failed to open file for zero'ing: %w", err) + return fmt.Errorf("error seeking start of %s: %w", path, err) } - defer fout.Close() zeros := bytes.Repeat([]byte{0}, blockSize) if _, err := fout.Write(zeros); err != nil {