Fixes for windows & enable in CI (#586)

Signed-off-by: James Carnegie <[email protected]>
Co-authored-by: Radoslav Dimitrov <[email protected]>

.github/workflows/tests.yml

@@ -32,11 +32,14 @@ jobs:
     strategy:
       fail-fast: false # Keep running if one leg fails.
       matrix:
-        os: [ubuntu-latest] # , macos-latest, windows-latest] Enable later so we don't waste github actions resources
+        os: [ubuntu-latest, windows-latest] # , ] Enable later so we don't waste github actions resources
         go-version: ${{ fromJSON(needs.get-go-versions.outputs.matrix) }}
     runs-on: ${{ matrix.os }}
     needs: get-go-versions
     steps:
+      - name: Set git to use LF
+        run: git config --global core.autocrlf false
+
       - name: Checkout code
         uses: actions/checkout@f43a0e5ff2bd294095638e18286ca9a3d1956744
 

examples/cli/tuf-client/cmd/get.go

@@ -157,7 +157,7 @@ func verifyEnv() (*localConfig, error) {
 		return nil, fmt.Errorf("no local download folder: %w", err)
 	}
 	// verify there's a local root.json available for bootstrapping trust
-	_, err = os.Stat(fmt.Sprintf("%s/%s.json", env.MetadataDir, metadata.ROOT))
+	_, err = os.Stat(filepath.Join(env.MetadataDir, fmt.Sprintf("%s.json", metadata.ROOT)))
 	if err != nil {
 		return nil, fmt.Errorf("no local download folder: %w", err)
 	}

examples/cli/tuf-client/cmd/init.go

@@ -75,7 +75,7 @@ func InitializeCmd() error {
 		if err != nil {
 			return err
 		}
-		rootPath = fmt.Sprintf("%s/%s.json", rootPath, metadata.ROOT)
+		rootPath = filepath.Join(rootPath, fmt.Sprintf("%s.json", metadata.ROOT))
 		// no need to copy root.json to the metadata folder as we already download it in the expected location
 		copyTrusted = false
 	}

metadata/metadata_test.go

@@ -21,8 +21,8 @@ import (
 	"crypto/ed25519"
 	"crypto/sha256"
 	"encoding/json"
-	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -506,7 +506,7 @@ func TestToByte(t *testing.T) {
 
 func TestFromFile(t *testing.T) {
 	root := Root(fixedExpire)
-	_, err := root.FromFile(fmt.Sprintf("%s/1.root.json", TEST_REPOSITORY_DATA))
+	_, err := root.FromFile(filepath.Join(TEST_REPOSITORY_DATA, "1.root.json"))
 	assert.NoError(t, err)
 
 	assert.Equal(t, fixedExpire, root.Signed.Expires)
@@ -554,7 +554,7 @@ func TestToFile(t *testing.T) {
 	tmpDir, err := os.MkdirTemp(tmp, "0750")
 	assert.NoError(t, err)
 
-	fileName := fmt.Sprintf("%s/1.root.json", tmpDir)
+	fileName := filepath.Join(tmpDir, "1.root.json")
 	assert.NoFileExists(t, fileName)
 	root, err := Root().FromBytes(testRootBytes)
 	assert.NoError(t, err)

metadata/trustedmetadata/trustedmetadata_test.go

@@ -19,8 +19,8 @@ package trustedmetadata
 
 import (
 	"crypto"
-	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
@@ -38,39 +38,50 @@ func setAllRolesBytes(path string) {
 	log := metadata.GetLogger()
 
 	allRoles = make(map[string][]byte)
-	root, err := os.ReadFile(fmt.Sprintf("%s/root.json", path))
+	rootPath := filepath.Join(path, "root.json")
+	root, err := os.ReadFile(rootPath)
 	if err != nil {
-		log.Error(err, "failed to root bytes")
+		log.Error(err, "failed to read root bytes")
 		os.Exit(1)
 	}
 	allRoles[metadata.ROOT] = root
-	targets, err := os.ReadFile(fmt.Sprintf("%s/targets.json", path))
+
+	targetsPath := filepath.Join(path, "targets.json")
+	targets, err := os.ReadFile(targetsPath)
 	if err != nil {
-		log.Error(err, "failed to targets bytes")
+		log.Error(err, "failed to read targets bytes")
 		os.Exit(1)
 	}
 	allRoles[metadata.TARGETS] = targets
-	snapshot, err := os.ReadFile(fmt.Sprintf("%s/snapshot.json", path))
+
+	snapshotPath := filepath.Join(path, "snapshot.json")
+	snapshot, err := os.ReadFile(snapshotPath)
 	if err != nil {
-		log.Error(err, "failed to snapshot bytes")
+		log.Error(err, "failed to read snapshot bytes")
 		os.Exit(1)
 	}
 	allRoles[metadata.SNAPSHOT] = snapshot
-	timestamp, err := os.ReadFile(fmt.Sprintf("%s/timestamp.json", path))
+
+	timestampPath := filepath.Join(path, "timestamp.json")
+	timestamp, err := os.ReadFile(timestampPath)
 	if err != nil {
-		log.Error(err, "failed to timestamp bytes")
+		log.Error(err, "failed to read timestamp bytes")
 		os.Exit(1)
 	}
 	allRoles[metadata.TIMESTAMP] = timestamp
-	role1, err := os.ReadFile(fmt.Sprintf("%s/role1.json", path))
+
+	role1Path := filepath.Join(path, "role1.json")
+	role1, err := os.ReadFile(role1Path)
 	if err != nil {
-		log.Error(err, "failed to role1 bytes")
+		log.Error(err, "failed to read role1 bytes")
 		os.Exit(1)
 	}
 	allRoles["role1"] = role1
-	role2, err := os.ReadFile(fmt.Sprintf("%s/role2.json", path))
+
+	role2Path := filepath.Join(path, "role2.json")
+	role2, err := os.ReadFile(role2Path)
 	if err != nil {
-		log.Error(err, "failed to role2 bytes")
+		log.Error(err, "failed to read role2 bytes")
 		os.Exit(1)
 	}
 	allRoles["role2"] = role2
@@ -104,7 +115,7 @@ func modifyRootMetadata(fn modifyRoot) ([]byte, error) {
 	}
 	fn(root)
 
-	signer, err := signature.LoadSignerFromPEMFile(testutils.KeystoreDir+"/root_key", crypto.SHA256, cryptoutils.SkipPassword)
+	signer, err := signature.LoadSignerFromPEMFile(filepath.Join(testutils.KeystoreDir, "root_key"), crypto.SHA256, cryptoutils.SkipPassword)
 	if err != nil {
 		log.Error(err, "failed to load signer from pem file")
 	}
@@ -127,7 +138,7 @@ func modifyTimestamptMetadata(fn modifyTimestamp) ([]byte, error) {
 	}
 	fn(timestamp)
 
-	signer, err := signature.LoadSignerFromPEMFile(testutils.KeystoreDir+"/timestamp_key", crypto.SHA256, cryptoutils.SkipPassword)
+	signer, err := signature.LoadSignerFromPEMFile(filepath.Join(testutils.KeystoreDir, "timestamp_key"), crypto.SHA256, cryptoutils.SkipPassword)
 	if err != nil {
 		log.Error(err, "failed to load signer from pem file")
 	}
@@ -150,7 +161,7 @@ func modifySnapshotMetadata(fn modifySnapshot) ([]byte, error) {
 	}
 	fn(snapshot)
 
-	signer, err := signature.LoadSignerFromPEMFile(testutils.KeystoreDir+"/snapshot_key", crypto.SHA256, cryptoutils.SkipPassword)
+	signer, err := signature.LoadSignerFromPEMFile(filepath.Join(testutils.KeystoreDir, "snapshot_key"), crypto.SHA256, cryptoutils.SkipPassword)
 	if err != nil {
 		log.Error(err, "failed to load signer from pem file")
 	}
@@ -173,7 +184,7 @@ func modifyTargetsMetadata(fn modifyTargets) ([]byte, error) {
 	}
 	fn(targets)
 
-	signer, err := signature.LoadSignerFromPEMFile(testutils.KeystoreDir+"/targets_key", crypto.SHA256, cryptoutils.SkipPassword)
+	signer, err := signature.LoadSignerFromPEMFile(filepath.Join(testutils.KeystoreDir, "targets_key"), crypto.SHA256, cryptoutils.SkipPassword)
 	if err != nil {
 		log.Error(err, "failed to load signer from pem file")
 	}

metadata/updater/updater.go

@@ -26,6 +26,8 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
+	"regexp"
+	"runtime"
 	"strconv"
 	"strings"
 	"time"
@@ -237,7 +239,7 @@ func (update *Updater) DownloadTarget(targetFile *metadata.TargetFiles, filePath
 			targetFilePath = fmt.Sprintf("%s.%s", hashes, dirName)
 		} else {
 			// <dir-prefix>/<hash>.<target-name>
-			targetFilePath = fmt.Sprintf("%s/%s.%s", dirName, hashes, baseName)
+			targetFilePath = filepath.Join(dirName, fmt.Sprintf("%s.%s", hashes, baseName))
 		}
 	}
 	fullURL := fmt.Sprintf("%s%s", targetBaseURL, targetFilePath)
@@ -575,6 +577,40 @@ func (update *Updater) preOrderDepthFirstWalk(targetFilePath string) (*metadata.
 	return nil, fmt.Errorf("target %s not found", targetFilePath)
 }
 
+// on windows, you can't rename a file across drives, so let's move instead
+func MoveFile(source, destination string) (err error) {
+	if runtime.GOOS == "windows" {
+		inputFile, err := os.Open(source)
+		if err != nil {
+			return fmt.Errorf("Couldn't open source file: %s", err)
+		}
+		defer inputFile.Close()
+		outputFile, err := os.Create(destination)
+		if err != nil {
+			inputFile.Close()
+			return fmt.Errorf("Couldn't open dest file: %s", err)
+		}
+		defer outputFile.Close()
+		c, err := io.Copy(outputFile, inputFile)
+		if err != nil {
+			return fmt.Errorf("Writing to output file failed: %s", err)
+		}
+		if c <= 0 {
+			return fmt.Errorf("Nothing copied to output file")
+		}
+		inputFile.Close()
+		// The copy was successful, so now delete the original file
+		err = os.Remove(source)
+		if err != nil {
+			return fmt.Errorf("Failed removing original file: %s", err)
+		}
+		return nil
+	} else {
+		return os.Rename(source, destination)
+	}
+
+}
+
 // persistMetadata writes metadata to disk atomically to avoid data loss
 func (update *Updater) persistMetadata(roleName string, data []byte) error {
 	log := metadata.GetLogger()
@@ -593,6 +629,7 @@ func (update *Updater) persistMetadata(roleName string, data []byte) error {
 	if err != nil {
 		return err
 	}
+	defer file.Close()
 	// write the data content to the temporary file
 	err = os.WriteFile(file.Name(), data, 0644)
 	if err != nil {
@@ -603,11 +640,21 @@ func (update *Updater) persistMetadata(roleName string, data []byte) error {
 		}
 		return err
 	}
+
+	// can't move/rename an open file on windows, so close it first
+	file.Close()
 	// if all okay, rename the temporary file to the desired one
-	err = os.Rename(file.Name(), fileName)
+	err = MoveFile(file.Name(), fileName)
+	if err != nil {
+		return err
+	}
+	read, err := os.ReadFile(fileName)
 	if err != nil {
 		return err
 	}
+	if string(read) != string(data) {
+		return fmt.Errorf("failed to persist metadata: %w", err)
+	}
 	return nil
 }
 
@@ -648,8 +695,20 @@ func (update *Updater) GetTrustedMetadataSet() trustedmetadata.TrustedMetadata {
 	return *update.trusted
 }
 
+func IsWindowsPath(path string) bool {
+	match, _ := regexp.MatchString(`^[a-zA-Z]:\\`, path)
+	return match
+}
+
 // ensureTrailingSlash ensures url ends with a slash
 func ensureTrailingSlash(url string) string {
+	if IsWindowsPath(url) {
+		slash := string(filepath.Separator)
+		if strings.HasSuffix(url, slash) {
+			return url
+		}
+		return url + slash
+	}
 	if strings.HasSuffix(url, "/") {
 		return url
 	}

metadata/updater/updater_consistent_snapshot_test.go

@@ -39,10 +39,12 @@ func TestTopLevelRolesUpdateWithConsistentSnapshotDisabled(t *testing.T) {
 	updaterConfig, err := loadUpdaterConfig()
 	assert.NoError(t, err)
 	updater := initUpdater(updaterConfig)
-
 	// cleanup fetch tracker metadata
 	simulator.Sim.FetchTracker.Metadata = []simulator.FTMetadata{}
 	err = updater.Refresh()
+	if err != nil {
+		t.Fatal(err)
+	}
 	assert.NoError(t, err)
 
 	// metadata files are fetched with the expected version (or None)
@@ -71,7 +73,9 @@ func TestTopLevelRolesUpdateWithConsistentSnapshotEnabled(t *testing.T) {
 	updaterConfig, err := loadUpdaterConfig()
 	assert.NoError(t, err)
 	updater := initUpdater(updaterConfig)
-
+	if updater == nil {
+		t.Fatal("updater is nil")
+	}
 	// cleanup fetch tracker metadata
 	simulator.Sim.FetchTracker.Metadata = []simulator.FTMetadata{}
 	err = updater.Refresh()
@@ -133,7 +137,9 @@ func TestDelegatesRolesUpdateWithConsistentSnapshotDisabled(t *testing.T) {
 	updaterConfig, err := loadUpdaterConfig()
 	assert.NoError(t, err)
 	updater := initUpdater(updaterConfig)
-
+	if updater == nil {
+		t.Fatal("updater is nil")
+	}
 	err = updater.Refresh()
 	assert.NoError(t, err)
 
@@ -149,7 +155,7 @@ func TestDelegatesRolesUpdateWithConsistentSnapshotDisabled(t *testing.T) {
 		{Name: "..", Value: -1},
 		{Name: ".", Value: -1},
 	}
-	assert.EqualValues(t, expectedsnapshotEnabled, simulator.Sim.FetchTracker.Metadata)
+	assert.ElementsMatch(t, expectedsnapshotEnabled, simulator.Sim.FetchTracker.Metadata)
 	// metadata files are always persisted without a version prefix
 	assertFilesExist(t, metadata.TOP_LEVEL_ROLE_NAMES[:])
 }
@@ -197,7 +203,9 @@ func TestDelegatesRolesUpdateWithConsistentSnapshotEnabled(t *testing.T) {
 	updaterConfig, err := loadUpdaterConfig()
 	assert.NoError(t, err)
 	updater := initUpdater(updaterConfig)
-
+	if updater == nil {
+		t.Fatal("updater is nil")
+	}
 	err = updater.Refresh()
 	assert.NoError(t, err)