Merge pull request #126 from OpenCHAMI/allend/fix-issues

Fix various issues, improve log messages, update documentation, and add regression tests

Author: davidallendj

CHANGELOG.md

@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## 0.5.2
+
+- Fixed issue with secrets file being closed too early
+- Added documentation about using `--insecure` with `scan`
+- Improved overall logging messages and consistency
+
 ## 0.5.1
 
 ### Added

README.md

@@ -8,25 +8,26 @@ The `magellan` CLI tool is a Redfish-based, board management controller (BMC) di
 <!-- TOC start (generated with https://github.com/derlin/bitdowntoc) -->
 
 - [OpenCHAMI Magellan](#openchami-magellan)
-  - [Main Features](#main-features)
-  - [Getting Started](#getting-started)
-  - [Building the Executable](#building-the-executable)
-    - [Building on Debian 12 (Bookworm)](#building-on-debian-12-bookworm)
-    - [Docker](#docker)
-    - [Arch Linux (AUR)](#arch-linux-aur)
-  - [Usage](#usage)
-    - [Checking for Redfish](#checking-for-redfish)
-    - [BMC ID Mapping](#bmc-id-mapping)
-    - [Running the Tool](#running-the-tool)
-    - [Managing Secrets](#managing-secrets)
-    - [Starting the Emulator](#starting-the-emulator)
-    - [Updating Firmware](#updating-firmware)
-    - [Managing Power](#managing-power)
-    - [Getting an Access Token (WIP)](#getting-an-access-token-wip)
-    - [Running with Docker](#running-with-docker)
-  - [How It Works](#how-it-works)
-  - [TODO](#todo)
-  - [Copyright](#copyright)
+	- [Main Features](#main-features)
+	- [Getting Started](#getting-started)
+	- [Documentation](#documentation)
+	- [Building the Executable](#building-the-executable)
+		- [Building on Debian 12 (Bookworm)](#building-on-debian-12-bookworm)
+		- [Docker](#docker)
+		- [Arch Linux (AUR)](#arch-linux-aur)
+	- [Usage](#usage)
+		- [Checking for Redfish](#checking-for-redfish)
+		- [BMC ID Mapping](#bmc-id-mapping)
+		- [Running the Tool](#running-the-tool)
+		- [PDU Inventory Collection](#pdu-inventory-collection)
+		- [Starting the Emulator](#starting-the-emulator)
+		- [Updating Firmware](#updating-firmware)
+		- [Managing Power](#managing-power)
+		- [Getting an Access Token (WIP)](#getting-an-access-token-wip)
+		- [Running with Docker](#running-with-docker)
+	- [How It Works](#how-it-works)
+	- [TODO](#todo)
+	- [Copyright](#copyright)
 
 <!-- TOC end -->
 
@@ -50,6 +51,20 @@ See the [TODO](#todo) section for a list of soon-ish goals planned.
 
 [Build](#building) and [run on bare metal](#running-the-tool) or run and test with Docker using the [latest prebuilt image](#running-with-docker). For quick testing, the repository integrates a Redfish emulator that can be run by executing the `emulator/setup.sh` script or running `make emulator`.
 
+## Documentation
+
+There is detailed documentation included with the project's repository that can be built using `scdoc` and `go doc`.
+
+To build the documentation, invoke the following commands.
+
+```bash
+# man page documentation
+make man
+
+# API reference documentation
+make docs
+```
+
 ## Building the Executable
 
 The `magellan` tool can be built to run on bare metal. Install the required Go tools, clone the repo, and then build the binary in the root directory with the following:
@@ -158,7 +173,6 @@ Where the `map_key` is the name of the attribute known to `magellan` that identi
 x<cabinet>c<chassis>s<shelf>b<blade>
 ```
 
-
 where `<cabinet>` is a cabinet number in the cluster, `<chassis>` is a chassis within the cabinet, `<shelf>` is the shelf within the chassis and `<blade>` is the blade within a shelf where the BMC is located. The above mapping file (minus the elipsis) will work with the example described in the [Starting the Emulator](#starting-the-emulator) section.
 
 If you are using `magellan` within a system deployed using RIE in the [Quickstart Deployment Recipe](https://github.com/OpenCHAMI/deployment-recipes/blob/main/quickstart/README.md) you can generate a BMC ID Map with XNAMEs that match the RIE configured XNAMEs from the RIE instances running under `docker-compose`. You can do this outside of the docker containers by running this script:
@@ -240,6 +254,9 @@ To start a network scan for BMC nodes, use the `scan` command. If the port is no
     --cache data/assets.db
 ```
 
+> [!NOTE]
+> Make sure to include the `--insecure` flag if the BMC does not require TLS verification when using HTTPS.
+
 This will scan the `172.16.0.0` subnet returning the host and port that return a response and store the results in a local cache with at the `data/assets.db` path. Additional flags can be set such as `--host` to add more hosts to scan that are not included on the subnet, `--timeout` to set how long to wait for a response from the BMC node, or `--concurrency` to set the number of requests to make concurrently with goroutines. Try using `./magellan help scan` for a complete set of options this subcommand. Alternatively, the same scan can be started using CIDR notation and with additional hosts:
 
 ```bash

cmd/collect.go

@@ -117,12 +117,10 @@ var CollectCmd = &cobra.Command{
 		params := &magellan.CollectParams{
 			Timeout:     timeout,
 			Concurrency: concurrency,
-			CaCertPath:  cacertPath,
 			OutputPath:  outputPath,
 			OutputDir:   outputDir,
+			Insecure:    insecure,
 			Format:      collectOutputFormat,
-			ForceUpdate: forceUpdate,
-			AccessToken: accessToken,
 			SecretStore: store,
 			BMCIDMap:    idMap,
 		}
@@ -153,9 +151,8 @@ func init() {
 	CollectCmd.Flags().StringVar(&protocol, "protocol", "tcp", "Set the protocol used to query")
 	CollectCmd.Flags().StringVarP(&outputPath, "output-file", "o", "", "Set the path to store collection data in a single file")
 	CollectCmd.Flags().StringVarP(&outputDir, "output-dir", "O", "", "Set the path to store collection data using HIVE partitioning")
+	CollectCmd.Flags().BoolVarP(&insecure, "insecure", "i", false, "Skip TLS certificate verification during probe")
 	CollectCmd.Flags().BoolVar(&showOutput, "show", false, "Show the output of a collect run")
-	CollectCmd.Flags().BoolVar(&forceUpdate, "force-update", false, "Set flag to force update data sent to SMD")
-	CollectCmd.Flags().StringVar(&cacertPath, "cacert", "", "Set the path to CA cert file (defaults to system CAs when blank)")
 	CollectCmd.Flags().VarP(&collectOutputFormat, "format", "F", "Set the default output data format (json|yaml; can be overridden by file extensions)")
 	CollectCmd.Flags().StringVarP(&idMap, "bmc-id-map", "m", "", "Set the BMC ID mapping from raw json data or use @<path> to specify a file path (json or yaml input)")
 
@@ -168,8 +165,8 @@ func init() {
 	checkBindFlagError(viper.BindPFlag("collect.protocol", CollectCmd.Flags().Lookup("protocol")))
 	checkBindFlagError(viper.BindPFlag("collect.output-file", CollectCmd.Flags().Lookup("output-file")))
 	checkBindFlagError(viper.BindPFlag("collect.output-dir", CollectCmd.Flags().Lookup("output-dir")))
-	checkBindFlagError(viper.BindPFlag("collect.force-update", CollectCmd.Flags().Lookup("force-update")))
-	checkBindFlagError(viper.BindPFlag("collect.cacert", CollectCmd.Flags().Lookup("cacert")))
+	// checkBindFlagError(viper.BindPFlag("collect.force-update", CollectCmd.Flags().Lookup("force-update")))
+	// checkBindFlagError(viper.BindPFlag("collect.cacert", CollectCmd.Flags().Lookup("cacert")))
 	checkBindFlagError(viper.BindPFlags(CollectCmd.Flags()))
 
 	rootCmd.AddCommand(CollectCmd)

cmd/crawl.go

@@ -3,7 +3,6 @@ package cmd
 import (
 	"encoding/json"
 	"fmt"
-	"os"
 
 	"github.com/rs/zerolog/log"
 
@@ -57,7 +56,7 @@ var CrawlCmd = &cobra.Command{
 			log.Debug().Str("uri", uri).Msgf("one or both of --username and --password NOT passed, attempting to obtain missing credentials from secret store at %s", secretsFile)
 			if store, err = secrets.OpenStore(secretsFile); err != nil {
 				log.Error().Str("uri", uri).Err(err).Msg("failed to open local secrets store")
-				os.Exit(1)
+				return
 			}
 
 			// Either none of the flags were passed or only one of them were; get
@@ -137,7 +136,7 @@ var CrawlCmd = &cobra.Command{
 		}, crawlOutputFormat)
 		if err != nil {
 			log.Error().Err(err).Msg("failed to marshal output JSON")
-			os.Exit(1)
+			return
 		}
 		if showOutput {
 			fmt.Println(string(output))

cmd/root.go

@@ -65,7 +65,7 @@ var rootCmd = &cobra.Command{
 		}
 	},
 	PostRun: func(cmd *cobra.Command, args []string) {
-		log.Debug().Msg("closing log file")
+		log.Debug().Str("path", logFile).Msg("closing log file")
 		err := logger.LogFile.Close()
 		if err != nil {
 			log.Error().Err(err).Msg("failed to close log file")
@@ -147,12 +147,7 @@ func InitializeConfig() {
 		viper.SetConfigFile(configPath)
 	}
 	if err := viper.ReadInConfig(); err != nil {
-		if _, ok := err.(viper.ConfigFileNotFoundError); ok {
-			err = fmt.Errorf("config file not found: %w", err)
-		} else {
-			err = fmt.Errorf("failed to load config file: %w", err)
-		}
-		log.Warn().Err(err).Msg("failed to load config")
+		log.Debug().Err(err).Msg("failed to load config")
 	}
 }
 

cmd/scan.go

@@ -38,7 +38,7 @@ var ScanCmd = &cobra.Command{
 	Use: "scan urls...",
 	Example: `
   // assumes host https://10.0.0.101:443
-  magellan scan 10.0.0.101
+  magellan scan 10.0.0.101 --insecure
 
   // assumes subnet using HTTPS and port 443 except for specified host
   magellan scan http://10.0.0.101:80 https://user:password@10.0.0.102:443 http://172.16.0.105:8080 --subnet 172.16.0.0/24
@@ -47,10 +47,10 @@ var ScanCmd = &cobra.Command{
   magellan scan 10.0.0.101 10.0.0.102 https://172.16.0.10:443 --port 8080 --protocol tcp
 
   // assumes subnet using default unspecified subnet-masks
-  magellan scan --subnet 10.0.0.0
+  magellan scan --subnet 10.0.0.0 -i
 
   // assumes subnet using HTTPS and port 443 with specified CIDR
-  magellan scan --subnet 10.0.0.0/16
+  magellan scan --subnet 10.0.0.0/16 -i
 
   // assumes subnet using HTTP and port 5000 similar to 192.168.0.0/16
   magellan scan --subnet 192.168.0.0 --protocol tcp --scheme https --port 5000 --subnet-mask 255.255.0.0
@@ -140,7 +140,6 @@ var ScanCmd = &cobra.Command{
 			log.Trace().Any("assets", foundAssets).Msgf("found assets from scan")
 		} else {
 			log.Warn().Msg("no responsive assets found")
-			// return instead of exit to close log file
 			return
 		}
 

cmd/secrets.go

@@ -28,7 +28,7 @@ var secretsCmd = &cobra.Command{
   magellan secrets store $bmc_host $bmc_creds
 
   // retrieve creds from secrets store
-  magellan secrets retrieve $bmc_host -f nodes.json
+  magellan secrets retrieve $bmc_host -f secrets.json
 
   // list creds from specific secrets
   magellan secrets list -f nodes.json`,
@@ -43,8 +43,8 @@ var secretsGenerateKeyCmd = &cobra.Command{
 	Run: func(cmd *cobra.Command, args []string) {
 		key, err := secrets.GenerateMasterKey()
 		if err != nil {
-			fmt.Printf("Error generating master key: %v\n", err)
-			os.Exit(1)
+			log.Error().Err(err).Msg("failed to generate master key")
+			return
 		}
 		fmt.Printf("%s\n", key)
 	},
@@ -65,8 +65,8 @@ var secretsStoreCmd = &cobra.Command{
 
 		// require either the args or input file
 		if len(args) < 1 && secretsStoreInputFile == "" {
-			log.Error().Msg("no input data or file")
-			os.Exit(1)
+			log.Error().Msg("requires input data or file")
+			return
 		} else if len(args) > 1 && secretsStoreInputFile == "" {
 			// use args[1] here because args[0] is the secretID
 			secretValue = args[1]
@@ -80,6 +80,7 @@ var secretsStoreCmd = &cobra.Command{
 				username string
 				password string
 			)
+
 			// seperate username and password provided
 			values = strings.Split(secretValue, ":")
 			if len(values) != 2 {
@@ -104,55 +105,76 @@ var secretsStoreCmd = &cobra.Command{
 		case "base64": // format: ($encoded_base64_string)
 			decoded, err := base64.StdEncoding.DecodeString(secretValue)
 			if err != nil {
-				log.Error().Err(err).Msg("error decoding base64 data")
+				log.Error().
+					Err(err).
+					Str("path", secretsFile).
+					Msg("failed to decode base64 data")
 				return
 			}
 
 			// check the decoded string if it's a valid JSON and has creds
 			if !isValidCredsJSON(string(decoded)) {
-				log.Error().Err(err).Msg("value is not a valid JSON or is missing credentials")
+				log.Error().
+					Err(err).
+					Str("path", secretsFile).
+					Msg("invalid JSON value or is missing credentials")
 				return
 			}
 
 			store, err = secrets.OpenStore(secretsFile)
 			if err != nil {
-				log.Error().Err(err).Msg("failed to open secrets store")
+				log.Error().
+					Err(err).
+					Str("path", secretsFile).
+					Msg("failed to open secrets store")
 				os.Exit(1)
 			}
 			secretValue = string(decoded)
 		case "json": // format: {"username": $username, "password": $password}
 			// read input from file if set and override
 			if secretsStoreInputFile != "" {
 				if secretValue != "" {
-					log.Error().Msg("cannot use -i/--input-file with positional argument")
+					log.Error().
+						Str("input-file", secretsStoreInputFile).
+						Msg("cannot use -i/--input-file with positional argument")
 					return
 				}
 				inputFileBytes, err = os.ReadFile(secretsStoreInputFile)
 				if err != nil {
-					log.Error().Err(err).Msg("failed to read input file")
+					log.Error().
+						Err(err).
+						Msg("failed to read input file")
 					return
 				}
 				secretValue = string(inputFileBytes)
 			}
 
 			// make sure we have valid JSON with "username" and "password" properties
 			if !isValidCredsJSON(secretValue) {
-				log.Error().Err(err).Msg("not a valid JSON or creds")
+				log.Error().
+					Err(err).
+					Msg("invalid JSON value or creds")
 				os.Exit(1)
 			}
 			store, err = secrets.OpenStore(secretsFile)
 			if err != nil {
-				fmt.Println(err)
-				log.Error().Err(err).Msg("failed to open secret store")
+				log.Error().
+					Err(err).
+					Str("path", secretsFile).
+					Msg("failed to open secret store")
 				os.Exit(1)
 			}
 		default:
-			log.Error().Msg("no input format set")
+			log.Error().Msg("invalid format (see --format flag for options)")
 			os.Exit(1)
 		}
 
 		if err := store.StoreSecretByID(secretID, secretValue); err != nil {
-			log.Error().Err(err).Msg("failed to store secret by ID")
+			log.Error().
+				Err(err).
+				Str("id", secretID).
+				Str("path", secretsFile).
+				Msg("failed to store secret by ID")
 			os.Exit(1)
 		}
 	},
@@ -224,29 +246,40 @@ var secretsListCmd = &cobra.Command{
 }
 
 var secretsRemoveCmd = &cobra.Command{
-	Use:   "remove secretIDs...",
+	Use:   "remove secret_ids...",
 	Args:  cobra.MinimumNArgs(1),
 	Short: "Remove secrets by IDs from secret store.",
 	Run: func(cmd *cobra.Command, args []string) {
 		for _, secretID := range args {
 			// open secret store from file
 			store, err := secrets.OpenStore(secretsFile)
 			if err != nil {
-				fmt.Println(err)
-				os.Exit(1)
+				log.Error().
+					Err(err).
+					Str("path", secretsFile).
+					Msg("failed to open secret store")
+				return
 			}
 
 			// remove secret from store by it's ID
 			err = store.RemoveSecretByID(secretID)
 			if err != nil {
-				fmt.Println("failed to remove secret: ", err)
-				os.Exit(1)
+				log.Error().
+					Err(err).
+					Str("id", secretID).
+					Str("path", secretsFile).
+					Msg("failed to remove secret")
+				return
 			}
 
 			// update store by saving to original file
 			err = secrets.SaveSecrets(secretsFile, store.(*secrets.LocalSecretStore).Secrets)
 			if err != nil {
-				log.Error().Err(err).Str("path", secretsFile).Msg("failed to save secrets to file")
+				log.Error().
+					Err(err).
+					Str("path", secretsFile).
+					Msg("failed to save secrets to file")
+				return
 			}
 		}
 	},