cscli machines add: don't overwrite existing credential file (#2625)

* cscli machines add: don't overwrite existing credential file
* keep old behavior with --force
Now --force is used both to override the replacement of and existing machine,
and an existing credentials file. To retain the old behavior, the
existence of the file is only checked for the default configuration, not
if explicitly specified.

cmd/crowdsec-cli/capi.go

@@ -110,7 +110,7 @@ func NewCapiRegisterCmd() *cobra.Command {
 				if err != nil {
 					return fmt.Errorf("write api credentials in '%s' failed: %w", dumpFile, err)
 				}
-				log.Printf("Central API credentials dumped to '%s'", dumpFile)
+				log.Printf("Central API credentials written to '%s'", dumpFile)
 			} else {
 				fmt.Printf("%s\n", string(apiConfigDump))
 			}

cmd/crowdsec-cli/config_restore.go

@@ -183,7 +183,7 @@ func restoreConfigFromDirectory(dirPath string, oldBackup bool) error {
 			if csConfig.API.Server.OnlineClient != nil && csConfig.API.Server.OnlineClient.CredentialsFilePath != "" {
 				apiConfigDumpFile = csConfig.API.Server.OnlineClient.CredentialsFilePath
 			}
-			err = os.WriteFile(apiConfigDumpFile, apiConfigDump, 0o644)
+			err = os.WriteFile(apiConfigDumpFile, apiConfigDump, 0o600)
 			if err != nil {
 				return fmt.Errorf("write api credentials in '%s' failed: %s", apiConfigDumpFile, err)
 			}

cmd/crowdsec-cli/lapi.go

@@ -150,11 +150,11 @@ func runLapiRegister(cmd *cobra.Command, args []string) error {
 		log.Fatalf("unable to marshal api credentials: %s", err)
 	}
 	if dumpFile != "" {
-		err = os.WriteFile(dumpFile, apiConfigDump, 0644)
+		err = os.WriteFile(dumpFile, apiConfigDump, 0o600)
 		if err != nil {
 			log.Fatalf("write api credentials in '%s' failed: %s", dumpFile, err)
 		}
-		log.Printf("Local API credentials dumped to '%s'", dumpFile)
+		log.Printf("Local API credentials written to '%s'", dumpFile)
 	} else {
 		fmt.Printf("%s\n", string(apiConfigDump))
 	}

cmd/crowdsec-cli/machines.go

@@ -43,6 +43,7 @@ func generatePassword(length int) string {
 	charsetLength := len(charset)
 
 	buf := make([]byte, length)
+
 	for i := 0; i < length; i++ {
 		rInt, err := saferand.Int(saferand.Reader, big.NewInt(int64(charsetLength)))
 		if err != nil {
@@ -190,7 +191,6 @@ cscli machines add MyTestMachine --password MyPassword
 }
 
 func runMachinesAdd(cmd *cobra.Command, args []string) error {
-	var dumpFile string
 	var err error
 
 	flags := cmd.Flags()
@@ -200,7 +200,7 @@ func runMachinesAdd(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	outputFile, err := flags.GetString("file")
+	dumpFile, err := flags.GetString("file")
 	if err != nil {
 		return err
 	}
@@ -220,7 +220,7 @@ func runMachinesAdd(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	forceAdd, err := flags.GetBool("force")
+	force, err := flags.GetBool("force")
 	if err != nil {
 		return err
 	}
@@ -242,17 +242,28 @@ func runMachinesAdd(cmd *cobra.Command, args []string) error {
 	}
 
 	/*check if file already exists*/
-	if outputFile != "" {
+	if dumpFile == "" && csConfig.API.Client != nil && csConfig.API.Client.CredentialsFilePath != "" {
-		dumpFile = outputFile
+		credFile := csConfig.API.Client.CredentialsFilePath
-	} else if csConfig.API.Client != nil && csConfig.API.Client.CredentialsFilePath != "" {
+		// use the default only if the file does not exist
-		dumpFile = csConfig.API.Client.CredentialsFilePath
+		_, err := os.Stat(credFile)
+		switch {
+		case os.IsNotExist(err) || force:
+			dumpFile = csConfig.API.Client.CredentialsFilePath
+		case err != nil:
+			return fmt.Errorf("unable to stat '%s': %s", credFile, err)
+		default:
+			return fmt.Errorf(`credentials file '%s' already exists: please remove it, use "--force" or specify a different file with "-f" ("-f -" for standard output)`, credFile)
+		}
+	}
+
+	if dumpFile == "" {
+		return fmt.Errorf(`please specify a file to dump credentials to, with -f ("-f -" for standard output)`)
 	}
 
 	// create a password if it's not specified by user
 	if machinePassword == "" && !interactive {
 		if !autoAdd {
-			printHelp(cmd)
+			return fmt.Errorf("please specify a password with --password or use --auto")
-			return nil
 		}
 		machinePassword = generatePassword(passwordLength)
 	} else if machinePassword == "" && interactive {
@@ -262,7 +273,7 @@ func runMachinesAdd(cmd *cobra.Command, args []string) error {
 		survey.AskOne(qs, &machinePassword)
 	}
 	password := strfmt.Password(machinePassword)
-	_, err = dbClient.CreateMachine(&machineID, &password, "", true, forceAdd, types.PasswordAuthType)
+	_, err = dbClient.CreateMachine(&machineID, &password, "", true, force, types.PasswordAuthType)
 	if err != nil {
 		return fmt.Errorf("unable to create machine: %s", err)
 	}
@@ -291,7 +302,7 @@ func runMachinesAdd(cmd *cobra.Command, args []string) error {
 		if err != nil {
 			return fmt.Errorf("write api credentials in '%s' failed: %s", dumpFile, err)
 		}
-		log.Printf("API credentials dumped to '%s'", dumpFile)
+		log.Printf("API credentials written to '%s'", dumpFile)
 	} else {
 		fmt.Printf("%s\n", string(apiConfigDump))
 	}

test/bats/30_machines.bats

@@ -23,20 +23,24 @@ teardown() {
 
 #----------
 
-@test "can list machines as regular user" {
-    rune -0 cscli machines list
-}
-
 @test "we have exactly one machine" {
     rune -0 cscli machines list -o json
     rune -0 jq -c '[. | length, .[0].machineId[0:32], .[0].isValidated]' <(output)
     assert_output '[1,"githubciXXXXXXXXXXXXXXXXXXXXXXXX",true]'
 }
 
+@test "don't overwrite local credentials by default" {
+    rune -1 cscli machines add local -a -o json
+    rune -0 jq -r '.msg' <(stderr)
+    assert_output --partial 'already exists: please remove it, use "--force" or specify a different file with "-f"'
+    rune -0 cscli machines add local -a --force
+    assert_stderr --partial "Machine 'local' successfully added to the local API"
+}
+
 @test "add a new machine and delete it" {
     rune -0 cscli machines add -a -f /dev/null CiTestMachine -o human
     assert_stderr --partial "Machine 'CiTestMachine' successfully added to the local API"
-    assert_stderr --partial "API credentials dumped to '/dev/null'"
+    assert_stderr --partial "API credentials written to '/dev/null'"
 
     # we now have two machines
     rune -0 cscli machines list -o json
@@ -56,7 +60,7 @@ teardown() {
 @test "register, validate and then remove a machine" {
     rune -0 cscli lapi register --machine CiTestMachineRegister -f /dev/null -o human
     assert_stderr --partial "Successfully registered to Local API (LAPI)"
-    assert_stderr --partial "Local API credentials dumped to '/dev/null'"
+    assert_stderr --partial "Local API credentials written to '/dev/null'"
 
     # the machine is not validated yet
     rune -0 cscli machines list -o json

test/lib/config/config-global

@@ -61,12 +61,12 @@ make_init_data() {
     ./instance-db config-yaml
     ./instance-db setup
 
+    ./bin/preload-hub-items
+
     # when installed packages are always using sqlite, so no need to regenerate
     # local credz for sqlite
 
-    ./bin/preload-hub-items
+    [[ "${DB_BACKEND}" == "sqlite" ]] || ${CSCLI} machines add githubciXXXXXXXXXXXXXXXXXXXXXXXX --auto --force
-
-    [[ "${DB_BACKEND}" == "sqlite" ]] || ${CSCLI} machines add --auto
 
     mkdir -p "$LOCAL_INIT_DIR"
 

test/lib/config/config-local

@@ -115,11 +115,12 @@ make_init_data() {
     ./instance-db config-yaml
     ./instance-db setup
 
-    "$CSCLI" --warning machines add githubciXXXXXXXXXXXXXXXXXXXXXXXX --auto
     "$CSCLI" --warning hub update
 
     ./bin/preload-hub-items
 
+    "$CSCLI" --warning machines add githubciXXXXXXXXXXXXXXXXXXXXXXXX --auto --force
+
     mkdir -p "$LOCAL_INIT_DIR"
 
     ./instance-db dump "${LOCAL_INIT_DIR}/database"