From 90641dd987d6af2a964daabb0b08b795ff024401 Mon Sep 17 00:00:00 2001 From: Marco Mariani Date: Wed, 29 Nov 2023 15:42:24 +0100 Subject: [PATCH 1/2] cscli machines add: don't overwrite existing credential file --- cmd/crowdsec-cli/capi.go | 2 +- cmd/crowdsec-cli/config_restore.go | 2 +- cmd/crowdsec-cli/lapi.go | 4 ++-- cmd/crowdsec-cli/machines.go | 29 ++++++++++++++++++++--------- test/bats/30_machines.bats | 15 +++++++++------ test/lib/config/config-global | 2 +- test/lib/config/config-local | 2 +- 7 files changed, 35 insertions(+), 21 deletions(-) diff --git a/cmd/crowdsec-cli/capi.go b/cmd/crowdsec-cli/capi.go index 0261eab9cb6..e316abbc6fa 100644 --- a/cmd/crowdsec-cli/capi.go +++ b/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)) } diff --git a/cmd/crowdsec-cli/config_restore.go b/cmd/crowdsec-cli/config_restore.go index 56f62828175..ab49836a96f 100644 --- a/cmd/crowdsec-cli/config_restore.go +++ b/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) } diff --git a/cmd/crowdsec-cli/lapi.go b/cmd/crowdsec-cli/lapi.go index b2870cb200f..755c2cb6d56 100644 --- a/cmd/crowdsec-cli/lapi.go +++ b/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)) } diff --git a/cmd/crowdsec-cli/machines.go b/cmd/crowdsec-cli/machines.go index 6c97c0109b5..30f7250b390 100644 --- a/cmd/crowdsec-cli/machines.go +++ b/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 } @@ -242,17 +242,28 @@ func runMachinesAdd(cmd *cobra.Command, args []string) error { } /*check if file already exists*/ - if outputFile != "" { - dumpFile = outputFile - } else if csConfig.API.Client != nil && csConfig.API.Client.CredentialsFilePath != "" { - dumpFile = csConfig.API.Client.CredentialsFilePath + if dumpFile == "" && csConfig.API.Client != nil && csConfig.API.Client.CredentialsFilePath != "" { + credFile := csConfig.API.Client.CredentialsFilePath + // use the default only if the file does not exist + _, err := os.Stat(credFile) + switch { + case os.IsNotExist(err): + 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 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 nil + return fmt.Errorf("please specify a password with --password or use --auto") } machinePassword = generatePassword(passwordLength) } else if machinePassword == "" && interactive { @@ -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)) } diff --git a/test/bats/30_machines.bats b/test/bats/30_machines.bats index d5ddf840f4c..14cb8c9a566 100644 --- a/test/bats/30_machines.bats +++ b/test/bats/30_machines.bats @@ -23,20 +23,23 @@ 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" { + rune -1 cscli machines add -a + local_credentials=$(config_get '.api.client.credentials_path') + assert_stderr --partial "credentials file '$local_credentials' already exists, please remove it or specify a different file with -f" + refute_output +} + @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 +59,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 diff --git a/test/lib/config/config-global b/test/lib/config/config-global index 7814cefbbc5..4c6eef20917 100755 --- a/test/lib/config/config-global +++ b/test/lib/config/config-global @@ -66,7 +66,7 @@ make_init_data() { ./bin/preload-hub-items - [[ "${DB_BACKEND}" == "sqlite" ]] || ${CSCLI} machines add --auto + [[ "${DB_BACKEND}" == "sqlite" ]] || ${CSCLI} machines add --auto -f "$($CSCLI config show --key Config.API.Client.CredentialsFilePath)" mkdir -p "$LOCAL_INIT_DIR" diff --git a/test/lib/config/config-local b/test/lib/config/config-local index 0941484ffd4..21ac2477772 100755 --- a/test/lib/config/config-local +++ b/test/lib/config/config-local @@ -115,7 +115,7 @@ make_init_data() { ./instance-db config-yaml ./instance-db setup - "$CSCLI" --warning machines add githubciXXXXXXXXXXXXXXXXXXXXXXXX --auto + "$CSCLI" --warning machines add githubciXXXXXXXXXXXXXXXXXXXXXXXX --auto -f "$($CSCLI config show --key Config.API.Client.CredentialsFilePath)" "$CSCLI" --warning hub update ./bin/preload-hub-items From d8092dbaa3693b8313474a02df9fda3d5a0f76d2 Mon Sep 17 00:00:00 2001 From: Marco Mariani Date: Thu, 30 Nov 2023 09:42:26 +0100 Subject: [PATCH 2/2] 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/machines.go | 8 ++++---- test/bats/30_machines.bats | 11 ++++++----- test/lib/config/config-global | 6 +++--- test/lib/config/config-local | 3 ++- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/cmd/crowdsec-cli/machines.go b/cmd/crowdsec-cli/machines.go index 30f7250b390..b7ceed25450 100644 --- a/cmd/crowdsec-cli/machines.go +++ b/cmd/crowdsec-cli/machines.go @@ -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 } @@ -247,12 +247,12 @@ func runMachinesAdd(cmd *cobra.Command, args []string) error { // use the default only if the file does not exist _, err := os.Stat(credFile) switch { - case os.IsNotExist(err): + 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 or specify a different file with -f ("-f -" for standard output)`, credFile) + 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) } } @@ -273,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) } diff --git a/test/bats/30_machines.bats b/test/bats/30_machines.bats index 14cb8c9a566..f321b8f3f91 100644 --- a/test/bats/30_machines.bats +++ b/test/bats/30_machines.bats @@ -29,11 +29,12 @@ teardown() { assert_output '[1,"githubciXXXXXXXXXXXXXXXXXXXXXXXX",true]' } -@test "don't overwrite local credentials" { - rune -1 cscli machines add -a - local_credentials=$(config_get '.api.client.credentials_path') - assert_stderr --partial "credentials file '$local_credentials' already exists, please remove it or specify a different file with -f" - refute_output +@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" { diff --git a/test/lib/config/config-global b/test/lib/config/config-global index 4c6eef20917..22ac76df653 100755 --- a/test/lib/config/config-global +++ b/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 --auto -f "$($CSCLI config show --key Config.API.Client.CredentialsFilePath)" + [[ "${DB_BACKEND}" == "sqlite" ]] || ${CSCLI} machines add githubciXXXXXXXXXXXXXXXXXXXXXXXX --auto --force mkdir -p "$LOCAL_INIT_DIR" diff --git a/test/lib/config/config-local b/test/lib/config/config-local index 21ac2477772..e3b7bc685d4 100755 --- a/test/lib/config/config-local +++ b/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 -f "$($CSCLI config show --key Config.API.Client.CredentialsFilePath)" "$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"