Enforce alternatives to %v and %+v (#1639)

Co-authored-by: Matthew Christopher <matthchr@microsoft.com>

Taskfile.yml

@@ -54,13 +54,13 @@ tasks:
 
   ############### Generator targets ###############
   generator:quick-checks:
-    deps: [header-check, generator:format-code, generator:test]
+    deps: [header-check, specifier-check, generator:format-code, generator:test]
     # Lint is forced to the end because it expects the code is formatted
     cmds:
     - task: generator:lint
 
   generator:ci:
-    deps: [header-check, generator:lint-full, generator:test-cover]
+    deps: [header-check, specifier-check, generator:lint-full, generator:test-cover]
 
   generator:test:
     desc: Run {{.GENERATOR_APP}} unit tests.
@@ -121,16 +121,16 @@ tasks:
 
   ############### Controller targets ###############
   controller:quick-checks:
-    deps: [header-check, controller:format-code, controller:test]
+    deps: [header-check, specifier-check, controller:format-code, controller:test]
     # Lint is forced to the end because it expects the code is formatted
     cmds:
       - task: controller:lint
 
   controller:ci:
-    deps: [header-check, controller:lint-full, controller:test-integration-envtest-cover]
+    deps: [header-check, specifier-check, controller:lint-full, controller:test-integration-envtest-cover]
 
   controller:ci-live:
-    deps: [header-check, controller:lint-full, controller:test-integration-envtest-live]
+    deps: [header-check, specifier-check, controller:lint-full, controller:test-integration-envtest-live]
 
   controller:lint:
     desc: Run fast lint checks.
@@ -252,7 +252,7 @@ tasks:
 
   ############### Crossplane targets ###############
   crossplane:quick-checks:
-    deps: [header-check, crossplane:format-code]
+    deps: [header-check, specifier-check, crossplane:format-code]
     # Lint is forced to the end because it expects the code is formatted
     cmds:
       # - task: crossplane:lint
@@ -310,7 +310,7 @@ tasks:
       - ./bin/{{.GENERATOR_APP}} gen-types azure-crossplane.yaml
 
   crossplane:ci:
-    deps: [header-check, crossplane:generate-crds]
+    deps: [header-check, specifier-check, crossplane:generate-crds]
 
   ############### Shared targets ###############
 
@@ -331,3 +331,15 @@ tasks:
   header-check:
     desc: Ensure all files have an appropriate license header.
     cmds: [python3 ./scripts/check_headers.py]
+
+  specifier-check:
+    desc: Check that format specifiers %v and %+v are not used
+    # Both %v and %+v result in all the values from structs being dumped into the string. If that 
+    # struct happens to contain a secret or sensitive information, it ends up dumped out in an 
+    # uncontrolled way, potentially leading to a security issue or a problem with PII disclosure.
+    # The buried risk here is that while %v might be safe now, a future change to the struct might
+    # introduce a disclosure later on. 
+    cmds:
+      - cmd: echo "==> Checking format specifiers <=="
+        silent: true
+      - cmd: git grep -e "%+v" -e "%v" --break --heading --line-number -I '*.go'; if [ $? -eq 0 ]; then exit -1; else exit 0; fi

controllers/helpers.go

@@ -255,13 +255,14 @@ func EnsureInstanceWithResult(ctx context.Context, t *testing.T, tc TestContext,
 
 		statused := ConvertToStatus(instance)
 		// if we expect this resource to end up with provisioned == true then failedProvisioning == true is unrecoverable
+		// TODO: Implement fmt.Stringer on Status types
 		if provisioned && statused.Status.FailedProvisioning {
 			if strings.Contains(statused.Status.Message, "already exists") || strings.Contains(statused.Status.Message, "AlreadyExists") {
 				t.Log("")
 				t.Log("-------")
 				t.Log("unexpected failed provisioning encountered")
-				t.Logf("%+v\n", statused.Status)
-				t.Logf("current time %v\n", time.Now())
+				t.Logf("%s (%s)\n", statused.Status.Message, statused.Status.State)
+				t.Logf("current time %s\n", time.Now().Format("2006-01-02 15:04:05"))
 				t.Log("-------")
 				t.Log("")
 			}
@@ -333,7 +334,7 @@ func EnsureSecretsWithValue(ctx context.Context, t *testing.T, tc TestContext, i
 			return err
 		}
 		if !strings.Contains(string(secrets[secretSubKey]), secretvalue) {
-			return fmt.Errorf("secret with key %+v not equal to %s", secretKey, secretvalue)
+			return fmt.Errorf("secret with key %s not equal to %s", secretKey, secretvalue)
 		}
 
 		return nil

controllers/suite_test.go

@@ -14,6 +14,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/pkg/errors"
+
 	"github.com/gobuffalo/envy"
 	"k8s.io/client-go/kubernetes/scheme"
 	kscheme "k8s.io/client-go/kubernetes/scheme"
@@ -939,7 +941,7 @@ func setup() error {
 	if result.Response.StatusCode != 204 {
 		_, err = resourceGroupManager.CreateGroup(context.Background(), resourceGroupName, resourceGroupLocation)
 		if err != nil {
-			return fmt.Errorf("ResourceGroup creation failed: %v", err)
+			return errors.Wrap(err, "resource creation failed")
 		}
 	}
 
@@ -1013,23 +1015,23 @@ func TestMain(m *testing.M) {
 
 	err = setup()
 	if err != nil {
-		log.Println(fmt.Sprintf("could not set up environment: %v\n", err))
+		log.Println(fmt.Sprintf("could not set up environment: %s\n", err))
 		os.Exit(1)
 	}
 
 	code = m.Run()
 
 	err = teardown()
 	if err != nil {
-		log.Println(fmt.Sprintf("could not tear down environment: %v\n; original exit code: %v\n", err, code))
+		log.Println(fmt.Sprintf("could not tear down environment: %s\n; original exit code: %d\n", err, code))
 	}
 
 	os.Exit(code)
 }
 
 func PanicRecover(t *testing.T) {
 	if err := recover(); err != nil {
-		t.Logf("caught panic in test: %v", err)
+		t.Logf("caught panic in test: %s", err)
 		t.Logf("stacktrace from panic: \n%s", string(debug.Stack()))
 		t.Fail()
 	}

hack/generator/cmd/gen_kustomize.go

@@ -68,10 +68,10 @@ func NewGenKustomizeCommand() (*cobra.Command, error) {
 }
 
 func logAndExtractStack(str string, err error) error {
-	klog.Errorf("%s:\n%v\n", str, err)
+	klog.Errorf("%s:\n%s\n", str, err)
 	stackTrace := findDeepestTrace(err)
 	if stackTrace != nil {
-		klog.V(4).Infof("%+v", stackTrace)
+		klog.V(4).Infof("%s", stackTrace)
 	}
 	return err
 }

hack/generator/cmd/gen_types.go

@@ -30,7 +30,7 @@ func NewGenTypesCommand() (*cobra.Command, error) {
 
 			cg, err := codegen.NewCodeGeneratorFromConfigFile(configFile)
 			if err != nil {
-				klog.Errorf("Error creating code generator: %v\n", err)
+				klog.Errorf("Error creating code generator: %s\n", err)
 				return err
 			}
 

hack/generator/cmd/root.go

@@ -14,7 +14,7 @@ import (
 func Execute() {
 	cmd, err := newRootCommand()
 	if err != nil {
-		klog.Fatalf("fatal error: commands failed to build! %v\n", err)
+		klog.Fatalf("fatal error: commands failed to build! %s\n", err)
 	}
 
 	if err := cmd.Execute(); err != nil {

hack/generator/pkg/astmodel/armconversion/convert_to_arm_function_builder.go

@@ -353,7 +353,7 @@ func (builder *convertToARMBuilder) fixedValuePropertyHandler(propertyName astmo
 
 		enumType, ok := def.Type().(*astmodel.EnumType)
 		if !ok {
-			panic(fmt.Sprintf("Enum %v definition was not of type EnumDefinition", enumTypeName))
+			panic(fmt.Sprintf("Enum %s definition was not of type EnumDefinition", enumTypeName))
 		}
 
 		optionId := astmodel.GetEnumValueId(def.Name().Name(), enumType.Options()[0])

hack/generator/pkg/astmodel/armconversion/shared.go

@@ -7,6 +7,7 @@ package armconversion
 
 import (
 	"fmt"
+	"strings"
 	"sync"
 
 	"github.com/dave/dst"
@@ -46,7 +47,19 @@ func (builder conversionBuilder) propertyConversionHandler(
 		}
 	}
 
-	panic(fmt.Sprintf("No property found for %s in method %s\nFrom: %+v\nTo: %+v", toProp.PropertyName(), builder.methodName, *builder.kubeType, *builder.armType))
+	var kubeDescription strings.Builder
+	builder.kubeType.WriteDebugDescription(&kubeDescription, nil)
+
+	var armDescription strings.Builder
+	builder.armType.WriteDebugDescription(&armDescription, nil)
+
+	message := fmt.Sprintf(
+		"No property found for %q in method %s()\nFrom: %s\nTo: %s",
+		toProp.PropertyName(),
+		builder.methodName,
+		kubeDescription.String(),
+		armDescription.String())
+	panic(message)
 }
 
 type propertyConversionHandler = func(toProp *astmodel.PropertyDefinition, fromType *astmodel.ObjectType) []dst.Stmt
@@ -82,7 +95,7 @@ func getReceiverObjectType(codeGenerationContext *astmodel.CodeGenerationContext
 	receiverType, ok := rt.Type().(*astmodel.ObjectType)
 	if !ok {
 		// Don't expect to have any wrapper types left at this point
-		panic(fmt.Sprintf("receiver for ARMConversionFunction is not of expected type. TypeName: %v, Type %T", receiver, rt.Type()))
+		panic(fmt.Sprintf("receiver for ARMConversionFunction is not of expected type. TypeName: %s, Type %T", receiver, rt.Type()))
 	}
 
 	return receiverType

hack/generator/pkg/astmodel/code_generation_context.go

@@ -92,7 +92,7 @@ func (codeGenContext *CodeGenerationContext) GetGeneratedPackage(reference Packa
 
 	packageDef, ok := codeGenContext.generatedPackages[reference]
 	if !ok {
-		return nil, errors.Errorf("%v not imported", reference)
+		return nil, errors.Errorf("%s not imported", reference)
 	}
 	return packageDef, nil
 }
@@ -129,7 +129,7 @@ func (codeGenContext *CodeGenerationContext) GetTypesInPackage(packageRef Packag
 func (codeGenContext *CodeGenerationContext) GetTypesInCurrentPackage() Types {
 	def, ok := codeGenContext.GetTypesInPackage(codeGenContext.currentPackage)
 	if !ok {
-		msg := fmt.Sprintf("Should always have definitions for the current package %v", codeGenContext.currentPackage)
+		msg := fmt.Sprintf("Should always have definitions for the current package %s", codeGenContext.currentPackage)
 		panic(msg)
 	}
 

hack/generator/pkg/astmodel/conversion_function_builder.go

@@ -314,7 +314,9 @@ func IdentityConvertComplexMapProperty(builder *ConversionFunctionBuilder, param
 	}
 
 	if _, ok := destinationType.KeyType().(*PrimitiveType); !ok {
-		panic(fmt.Sprintf("map had non-primitive key type: %v", destinationType.KeyType()))
+		var keyDescription strings.Builder
+		destinationType.KeyType().WriteDebugDescription(&keyDescription, nil)
+		panic(fmt.Sprintf("map had non-primitive key type: %s", keyDescription.String()))
 	}
 
 	depth := params.CountArraysAndMapsInConversionContext()

hack/generator/pkg/astmodel/file_definition.go

@@ -152,7 +152,7 @@ func (file *FileDefinition) generateImports() *PackageImportSet {
 	// Resolve any conflicts and report any that couldn't be fixed up automatically
 	err := requiredImports.ResolveConflicts()
 	if err != nil {
-		klog.Errorf("File %s: %v", file.packageReference, err)
+		klog.Errorf("File %s: %s", file.packageReference, err)
 	}
 
 	return requiredImports

hack/generator/pkg/astmodel/kubebuilder_validations.go

@@ -37,20 +37,33 @@ func GenerateKubebuilderComment(validation KubeBuilderValidation) string {
 			// handle slice values which should look like "{"x","y","z"}
 			var values []string
 			for i := 0; i < value.Len(); i++ {
-				values = append(values, fmt.Sprintf("%v", value.Index(i)))
+				values = append(values, valueAsString(value.Index(i)))
 			}
 
 			return fmt.Sprintf("%s%s={%s}", prefix, validation.name, strings.Join(values, ","))
 		}
 
 		// everything else
-		return fmt.Sprintf("%s%s=%v", prefix, validation.name, validation.value)
+		return fmt.Sprintf("%s%s=%s", prefix, validation.name, valueAsString(value))
 	}
 
 	// validation without argument
 	return fmt.Sprintf("%s%s", prefix, validation.name)
 }
 
+func valueAsString(value reflect.Value) string {
+	switch v := value.Interface().(type) {
+	case int, int16, int32, int64:
+		return fmt.Sprintf("%d", v)
+	case bool:
+		return fmt.Sprintf("%t", v)
+	case string:
+		return v
+	default:
+		panic(fmt.Sprintf("unexpected value for kubebuilder comment - %s", value.Kind()))
+	}
+}
+
 func AddValidationComments(commentList *dst.Decorations, validations []KubeBuilderValidation) {
 	// generate validation comments:
 	for _, validation := range validations {
@@ -139,7 +152,7 @@ func ValidateMaximum(value *big.Rat) KubeBuilderValidation {
 	} else {
 		floatValue, ok := value.Float64()
 		if !ok {
-			klog.Warningf("inexact maximum: %s ⇒ %v", value.String(), floatValue)
+			klog.Warningf("inexact maximum: %s ⇒ %g", value.String(), floatValue)
 		}
 
 		return KubeBuilderValidation{MaximumValidationName, floatValue}
@@ -152,7 +165,7 @@ func ValidateMinimum(value *big.Rat) KubeBuilderValidation {
 	} else {
 		floatValue, ok := value.Float64()
 		if !ok {
-			klog.Warningf("inexact minimum: %s ⇒ %v", value.String(), floatValue)
+			klog.Warningf("inexact minimum: %s ⇒ %g", value.String(), floatValue)
 		}
 
 		return KubeBuilderValidation{MinimumValidationName, floatValue}
@@ -173,7 +186,7 @@ func ValidateMultipleOf(value *big.Rat) KubeBuilderValidation {
 	} else {
 		floatValue, ok := value.Float64()
 		if !ok {
-			klog.Warningf("inexact multiple-of: %s ⇒ %v", value.String(), floatValue)
+			klog.Warningf("inexact multiple-of: %s ⇒ %g", value.String(), floatValue)
 		}
 
 		return KubeBuilderValidation{MultipleOfValidationName, floatValue}

hack/generator/pkg/astmodel/package_definition.go

@@ -42,7 +42,7 @@ func (p *PackageDefinition) GetDefinition(typeName TypeName) (TypeDefinition, er
 		}
 	}
 
-	return TypeDefinition{}, errors.Errorf("no error with name %v found", typeName)
+	return TypeDefinition{}, errors.Errorf("no error with name %s found", typeName)
 }
 
 // AddDefinition adds a Definition to the PackageDefinition
@@ -78,7 +78,7 @@ func (p *PackageDefinition) emitFiles(filesToGenerate map[string][]TypeDefinitio
 	for fileName, defs := range filesToGenerate {
 		codeFilePath := filepath.Join(
 			outputDir,
-			fmt.Sprintf("%v_types%v.go", fileName, CodeGeneratedFileSuffix))
+			fmt.Sprintf("%s_types%s.go", fileName, CodeGeneratedFileSuffix))
 
 		err := p.writeCodeFile(codeFilePath, defs, generatedPackages)
 		if err != nil {
@@ -87,7 +87,7 @@ func (p *PackageDefinition) emitFiles(filesToGenerate map[string][]TypeDefinitio
 
 		testFilePath := filepath.Join(
 			outputDir,
-			fmt.Sprintf("%v_types%v_test.go", fileName, CodeGeneratedFileSuffix))
+			fmt.Sprintf("%s_types%s_test.go", fileName, CodeGeneratedFileSuffix))
 
 		err = p.writeTestFile(testFilePath, defs, generatedPackages)
 		if err != nil {

hack/generator/pkg/astmodel/package_import.go

@@ -70,7 +70,7 @@ func (pi PackageImport) Equals(ref PackageImport) bool {
 
 func (pi PackageImport) String() string {
 	if len(pi.name) > 0 {
-		return fmt.Sprintf("%v %v", pi.name, pi.packageReference)
+		return fmt.Sprintf("%s %s", pi.name, pi.packageReference)
 	}
 
 	return pi.packageReference.String()

hack/generator/pkg/astmodel/package_import_set.go

@@ -149,7 +149,7 @@ func (set *PackageImportSet) ResolveConflicts() error {
 		}
 
 		remappedImports[imp.packageReference] = imp
-		klog.V(3).Infof("Remapped %v to %v", imp.packageReference, name)
+		klog.V(3).Infof("Remapped %s to %s", imp.packageReference, name)
 		return imp.WithName(name)
 	})
 
@@ -159,7 +159,7 @@ func (set *PackageImportSet) ResolveConflicts() error {
 		// Only rename imports we already renamed above
 		if _, ok := remappedImports[imp.packageReference]; ok {
 			name := imp.VersionedNameForImport()
-			klog.V(3).Infof("Remapped %v to %v", imp.packageReference, name)
+			klog.V(3).Infof("Remapped %s to %s", imp.packageReference, name)
 			return imp.WithName(name)
 		}
 

hack/generator/pkg/astmodel/test_file_definition.go

@@ -115,7 +115,7 @@ func (file *TestFileDefinition) generateImports() *PackageImportSet {
 	// Resolve any conflicts and report any that couldn't be fixed up automatically
 	err := requiredImports.ResolveConflicts()
 	if err != nil {
-		klog.Errorf("File %s: %v", file.packageReference, err)
+		klog.Errorf("File %s: %s", file.packageReference, err)
 	}
 
 	return requiredImports

hack/generator/pkg/astmodel/type_definition.go

@@ -161,11 +161,11 @@ func (def TypeDefinition) ApplyObjectTransformation(transform func(*ObjectType)
 
 	newType, err := visitor.Visit(def.theType, nil)
 	if err != nil {
-		return TypeDefinition{}, errors.Wrapf(err, "transformation of %v failed", def.name)
+		return TypeDefinition{}, errors.Wrapf(err, "transformation of %s failed", def.name)
 	}
 
 	if !visited {
-		return TypeDefinition{}, errors.Errorf("transformation was not applied to %v (expected object type, found %v)", def.name, def.theType)
+		return TypeDefinition{}, errors.Errorf("transformation was not applied to %s (expected object type, found %s)", def.name, def.theType)
 	}
 
 	result := def.WithType(newType)