Add GitHub Action to run gosec and amend already present issues

.changes/v2.17.0/516-notes.md

@@ -0,0 +1 @@
+* Added a new GitHub Action to run `gosec` on every push and pull request [GH-516]

.github/workflows/check-security.yml

@@ -0,0 +1,12 @@
+name: Run Gosec
+on: [push, pull_request]
+jobs:
+  gosec:
+    runs-on: ubuntu-latest
+    env:
+      GO111MODULE: on
+    steps:
+      - name: Checkout Source
+        uses: actions/checkout@v2
+      - name: gosec
+        run: make security

Makefile

@@ -2,15 +2,15 @@ TEST?=./...
 GOFMT_FILES?=$$(find . -name '*.go')
 maindir=$(PWD)
 
-default: fmtcheck vet static build
+default: fmtcheck vet static security build
 
 # test runs the test suite and vets the code
 test: testunit tagverify
 	@echo "==> Running Functional Tests"
 	cd govcd && go test -tags "functional" -timeout=650m -check.vv
 
 # tagverify checks that each tag can run independently
-tagverify: fmtcheck 
+tagverify: fmtcheck
 	@echo "==> Running Tags Tests"
 	@./scripts/test-tags.sh
 
@@ -62,6 +62,10 @@ vet:
 static: fmtcheck
 	@./scripts/staticcheck.sh
 
+# security runs the source code security analysis tool `gosec`
+security: fmtcheck
+	@./scripts/gosec.sh
+
 get-deps:
 	@echo "==> Fetching dependencies"
 	@go get -v $(TEST)

govcd/api.go

@@ -61,6 +61,7 @@ type Client struct {
 const AuthorizationHeader = "X-Vcloud-Authorization"
 
 // BearerTokenHeader is the header key containing a bearer token
+// #nosec G101 -- This is not a credential, it's just the header key
 const BearerTokenHeader = "X-Vmware-Vcloud-Access-Token"
 
 const ApiTokenHeader = "API-token"
@@ -882,3 +883,10 @@ func urlParseRequestURI(href string) *url.URL {
 	}
 	return apiEndpoint
 }
+
+// safeClose closes a file and logs the error, if any. This can be used instead of file.Close()
+func safeClose(file *os.File) {
+	if err := file.Close(); err != nil {
+		util.Logger.Printf("Error closing file: %s\n", err)
+	}
+}

govcd/api_vcd.go

@@ -118,6 +118,7 @@ func NewVCDClient(vcdEndpoint url.URL, insecure bool, options ...VCDClientOption
 	}
 
 	// Setting defaults
+	// #nosec G402 -- InsecureSkipVerify: insecure - This allows connecting to VCDs with self-signed certificates
 	vcdClient := &VCDClient{
 		Client: Client{
 			APIVersion: minVcdApiVersion,

govcd/api_vcd_test.go

@@ -305,7 +305,7 @@ func readCleanupList() ([]CleanupEntity, error) {
 	if os.IsNotExist(err) {
 		return nil, err
 	}
-	listText, err := os.ReadFile(persistentCleanupListFile)
+	listText, err := os.ReadFile(filepath.Clean(persistentCleanupListFile))
 	if err != nil {
 		return nil, err
 	}
@@ -332,7 +332,7 @@ func writeCleanupList(cleanupList []CleanupEntity) error {
 	if err != nil {
 		return err
 	}
-	file, err := os.Create(persistentCleanupListFile)
+	file, err := os.Create(filepath.Clean(persistentCleanupListFile))
 	if err != nil {
 		return err
 	}
@@ -434,7 +434,7 @@ func GetConfigStruct() (TestConfig, error) {
 	if os.IsNotExist(err) {
 		return TestConfig{}, fmt.Errorf("Configuration file %s not found: %s", config, err)
 	}
-	yamlFile, err := os.ReadFile(config)
+	yamlFile, err := os.ReadFile(filepath.Clean(config))
 	if err != nil {
 		return TestConfig{}, fmt.Errorf("could not read config file %s: %s", config, err)
 	}

govcd/api_vcd_test_unit.go

@@ -10,6 +10,7 @@ package govcd
 import (
 	"io"
 	"os"
+	"path/filepath"
 	"testing"
 )
 
@@ -20,11 +21,11 @@ func goldenString(t *testing.T, goldenFile string, actual string, update bool) s
 
 	goldenPath := "../test-resources/golden/" + t.Name() + "_" + goldenFile + ".golden"
 
-	f, err := os.OpenFile(goldenPath, os.O_RDWR|os.O_CREATE, 0644)
+	f, err := os.OpenFile(filepath.Clean(goldenPath), os.O_RDWR|os.O_CREATE, 0600)
 	if err != nil {
 		t.Fatalf("unable to find golden file '%s': %s", goldenPath, err)
 	}
-	defer f.Close()
+	defer safeClose(f)
 
 	if update {
 		_, err := f.WriteString(actual)

govcd/catalog.go

@@ -471,8 +471,7 @@ func fetchVappTemplate(client *Client, vappTemplateUrl *url.URL) (*types.VAppTem
 // Function will return parsed part for upload files from description xml.
 func uploadOvfDescription(client *Client, ovfFile string, ovfUploadUrl *url.URL) error {
 	util.Logger.Printf("[TRACE] Uploding ovf description with file: %s and url: %s\n", ovfFile, ovfUploadUrl)
-	// #nosec G304 - linter does not like 'filePath' to be a variable. However this is necessary for file uploads.
-	openedFile, err := os.Open(ovfFile)
+	openedFile, err := os.Open(filepath.Clean(ovfFile))
 	if err != nil {
 		return err
 	}
@@ -642,8 +641,7 @@ func getOvfPath(filesAbsPaths []string) (string, error) {
 }
 
 func getOvf(ovfFilePath string) (Envelope, error) {
-	// #nosec G304 - linter does not like 'filePath' to be a variable. However this is necessary for file uploads.
-	openedFile, err := os.Open(ovfFilePath)
+	openedFile, err := os.Open(filepath.Clean(ovfFilePath))
 	if err != nil {
 		return Envelope{}, err
 	}

govcd/catalog_test.go

@@ -278,7 +278,9 @@ func (vcd *TestVCD) Test_UploadOvf_ShowUploadProgress_works(check *C) {
 
 	err = uploadTask.ShowUploadProgress()
 	check.Assert(err, IsNil)
-	w.Close()
+	err = w.Close()
+	check.Assert(err, IsNil)
+
 	//read stdin
 	result, _ := io.ReadAll(r)
 	os.Stdout = oldStdout
@@ -522,7 +524,8 @@ func (vcd *TestVCD) Test_CatalogUploadMediaImage_ShowUploadProgress_works(check
 
 	err = uploadTask.ShowUploadProgress()
 	check.Assert(err, IsNil)
-	w.Close()
+	err = w.Close()
+	check.Assert(err, IsNil)
 	//read stdin
 	result, _ := io.ReadAll(r)
 	os.Stdout = oldStdout

govcd/lb_test.go

@@ -204,7 +204,10 @@ func checkLb(queryUrl string, expectedResponses []string, maxRetryTimeout int) e
 			if err == nil {
 				fmt.Printf(".") // progress bar when waiting for responses from all nodes
 				body, _ := io.ReadAll(resp.Body)
-				resp.Body.Close()
+				err = resp.Body.Close()
+				if err != nil {
+					return err
+				}
 				// check if the element is in the list
 				for index, value := range expectedResponses {
 					if value == string(body) {

govcd/media.go

@@ -12,6 +12,7 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -261,12 +262,11 @@ func queryMedia(client *Client, mediaUrl string, newItemName string) (*types.Med
 
 // Verifies provided file header matches standard
 func verifyIso(filePath string) (bool, error) {
-	// #nosec G304 - linter does not like 'filePath' to be a variable. However this is necessary for file uploads.
-	file, err := os.Open(filePath)
+	file, err := os.Open(filepath.Clean(filePath))
 	if err != nil {
 		return false, err
 	}
-	defer file.Close()
+	defer safeClose(file)
 
 	return readHeader(file)
 }

govcd/nsxt_distributed_firewall_test.go

@@ -5,6 +5,7 @@ package govcd
 
 import (
 	"fmt"
+	"github.com/vmware/go-vcloud-director/v2/util"
 	"os"
 	"strconv"
 	"strings"
@@ -277,5 +278,8 @@ func dumpDistributedFirewallRulesToScreen(rules []*types.DistributedFirewallRule
 		fmt.Fprintf(w, "%s\t%s\t%s\t%t\t%s\t%t\t%d\t%d\t%d\t%d\n", rule.Name, rule.Direction, rule.IpProtocol,
 			rule.Enabled, rule.Action, rule.Logging, len(rule.SourceFirewallGroups), len(rule.DestinationFirewallGroups), len(rule.ApplicationPortProfiles), len(rule.NetworkContextProfiles))
 	}
-	w.Flush()
+	err := w.Flush()
+	if err != nil {
+		util.Logger.Printf("Error while dumping Distributed Firewall rules to screen: %s", err)
+	}
 }

govcd/nsxt_firewall_test.go

@@ -4,8 +4,10 @@
 package govcd
 
 import (
+	"crypto/rand"
 	"fmt"
-	"math/rand"
+	"github.com/vmware/go-vcloud-director/v2/util"
+	"math/big"
 	"os"
 	"strconv"
 	"text/tabwriter"
@@ -146,18 +148,18 @@ func createFirewallDefinitions(check *C, vcd *TestVCD) []*types.NsxtFirewallRule
 }
 
 func pickRandomString(in []string) string {
-	randomIndex := rand.Intn(len(in))
-	return in[randomIndex]
+	randomIndex, _ := rand.Int(rand.Reader, big.NewInt(int64(len(in))))
+	return in[randomIndex.Uint64()]
 }
 
 // pickRandomOpenApiRefOrEmpty picks a random OpenAPI entity or an empty one
 func pickRandomOpenApiRefOrEmpty(in []types.OpenApiReference) types.OpenApiReference {
 	// Random value can be up to len+1 (len+1 is the special case when it should return an empty reference)
-	randomIndex := rand.Intn(len(in) + 1)
-	if randomIndex == len(in) {
+	randomIndex, _ := rand.Int(rand.Reader, big.NewInt(int64(len(in)+1)))
+	if randomIndex.Uint64() == uint64(len(in)) {
 		return types.OpenApiReference{}
 	}
-	return in[randomIndex]
+	return in[randomIndex.Uint64()]
 }
 
 func preCreateIpSet(check *C, vcd *TestVCD) *NsxtFirewallGroup {
@@ -236,5 +238,8 @@ func dumpFirewallRulesToScreen(rules []*types.NsxtFirewallRule) {
 		fmt.Fprintf(w, "%s\t%s\t%s\t%t\t%s\t%t\t%d\t%d\t%d\n", rule.Name, rule.Direction, rule.IpProtocol,
 			rule.Enabled, rule.Action, rule.Logging, len(rule.SourceFirewallGroups), len(rule.DestinationFirewallGroups), len(rule.ApplicationPortProfiles))
 	}
-	w.Flush()
+	err := w.Flush()
+	if err != nil {
+		util.Logger.Printf("Error while dumping Firewall rules to screen: %s", err)
+	}
 }

govcd/saml_auth_unit_test.go

@@ -20,6 +20,7 @@ import (
 
 // testVcdMockAuthToken is the expected vcdCli.Client.VCDToken value after `Authentication()`
 // function passes mock SAML authentication process
+// #nosec G101 -- These credentials are fake for testing purposes
 const testVcdMockAuthToken = "e3b02b30b8ff4e87ac38db785b0172b5"
 
 // samlMockServer struct allows to attach HTTP handlers to use additional variables (like

govcd/upload.go

@@ -67,8 +67,7 @@ func uploadFile(client *Client, filePath string, uDetails uploadDetails) (int64,
 	var count int
 	var pieceSize int64
 
-	// #nosec G304 - linter does not like 'filePath' to be a variable. However this is necessary for file uploads.
-	file, err := os.Open(filePath)
+	file, err := os.Open(filepath.Clean(filePath))
 	if err != nil {
 		util.Logger.Printf("[ERROR] during upload process - file open issue : %s, error %s ", filePath, err)
 		*uDetails.uploadError = err
@@ -82,7 +81,7 @@ func uploadFile(client *Client, filePath string, uDetails uploadDetails) (int64,
 		return 0, err
 	}
 
-	defer file.Close()
+	defer safeClose(file)
 
 	fileSize := fileInfo.Size()
 	// when file size in OVF does not exist, use real file size instead

govcd/vdc_test.go

@@ -430,7 +430,8 @@ func (vcd *TestVCD) TestGetVappList(check *C) {
 
 	// Use the search engine to find the known vApp
 	criteria := NewFilterDef()
-	criteria.AddFilter(types.FilterNameRegex, TestSetUpSuite)
+	err = criteria.AddFilter(types.FilterNameRegex, TestSetUpSuite)
+	check.Assert(err, IsNil)
 	queryType := vcd.client.Client.GetQueryType(types.QtVapp)
 	queryItems, _, err := vcd.client.Client.SearchByFilter(queryType, criteria)
 	check.Assert(err, IsNil)
@@ -443,8 +444,10 @@ func (vcd *TestVCD) TestGetVappList(check *C) {
 	check.Assert(vmName, Not(Equals), "")
 	check.Assert(vm.HREF, Not(Equals), "")
 	criteria = NewFilterDef()
-	criteria.AddFilter(types.FilterNameRegex, vmName)
-	criteria.AddFilter(types.FilterParent, vapp.VApp.Name)
+	err = criteria.AddFilter(types.FilterNameRegex, vmName)
+	check.Assert(err, IsNil)
+	err = criteria.AddFilter(types.FilterParent, vapp.VApp.Name)
+	check.Assert(err, IsNil)
 	queryType = vcd.client.Client.GetQueryType(types.QtVm)
 	queryItems, _, err = vcd.client.Client.SearchByFilter(queryType, criteria)
 	check.Assert(err, IsNil)

govcd/vm_test.go

@@ -994,7 +994,8 @@ func (vcd *TestVCD) Test_AddInternalDisk(check *C) {
 	updateVdcFastProvisioning(vcd, check, previousProvisioningValue)
 
 	// delete Vapp early to avoid env capacity issue
-	deleteVapp(vcd, vmName)
+	err = deleteVapp(vcd, vmName)
+	check.Assert(err, IsNil)
 }
 
 // createInternalDisk Finds available VM and creates internal Disk in it.
@@ -1107,7 +1108,8 @@ func (vcd *TestVCD) Test_DeleteInternalDisk(check *C) {
 	updateVdcFastProvisioning(vcd, check, previousProvisioningValue)
 
 	// delete Vapp early to avoid env capacity issue
-	deleteVapp(vcd, vmName)
+	err = deleteVapp(vcd, vmName)
+	check.Assert(err, IsNil)
 }
 
 // Test update internal disk for VM which has independent disk
@@ -1180,7 +1182,8 @@ func (vcd *TestVCD) Test_UpdateInternalDisk(check *C) {
 	updateVdcFastProvisioning(vcd, check, previousProvisioningValue)
 
 	// delete Vapp early to avoid env capacity issue
-	deleteVapp(vcd, vmName)
+	err = deleteVapp(vcd, vmName)
+	check.Assert(err, IsNil)
 }
 
 func attachIndependentDisk(vcd *TestVCD, check *C) (*Disk, error) {
@@ -1449,7 +1452,8 @@ func (vcd *TestVCD) Test_UpdateVmSpecSection(check *C) {
 	check.Assert(updatedVm.VM.Description, Equals, "updateDescription")
 
 	// delete Vapp early to avoid env capacity issue
-	deleteVapp(vcd, vmName)
+	err = deleteVapp(vcd, vmName)
+	check.Assert(err, IsNil)
 }
 
 func (vcd *TestVCD) Test_QueryVmList(check *C) {
@@ -1521,7 +1525,8 @@ func (vcd *TestVCD) Test_UpdateVmCpuAndMemoryHotAdd(check *C) {
 	check.Assert(updatedVm.VM.VMCapabilities.CPUHotAddEnabled, Equals, true)
 
 	// delete Vapp early to avoid env capacity issue
-	deleteVapp(vcd, vmName)
+	err = deleteVapp(vcd, vmName)
+	check.Assert(err, IsNil)
 }
 
 func (vcd *TestVCD) Test_AddNewEmptyVMWithVmComputePolicyAndUpdate(check *C) {

samples/discover/discover.go

@@ -45,6 +45,7 @@ import (
 	"fmt"
 	"net/url"
 	"os"
+	"path/filepath"
 
 	"gopkg.in/yaml.v2"
 
@@ -94,7 +95,7 @@ func check_configuration(conf Config) {
 // Retrieves the configuration from a Json or Yaml file
 func getConfig(config_file string) Config {
 	var configuration Config
-	buffer, err := os.ReadFile(config_file)
+	buffer, err := os.ReadFile(filepath.Clean(config_file))
 	if err != nil {
 		fmt.Printf("Configuration file %s not found\n%s\n", config_file, err)
 		os.Exit(1)