State Encryption Error Handling / Diagnostics (#1294)

Signed-off-by: Christian Mesh <christianmesh1@gmail.com>

internal/encryption/base.go

@@ -27,13 +27,17 @@ type baseEncryption struct {
 	name     string
 }
 
-func newBaseEncryption(enc *encryption, target *config.TargetConfig, enforced bool, name string) *baseEncryption {
+func newBaseEncryption(enc *encryption, target *config.TargetConfig, enforced bool, name string) (*baseEncryption, hcl.Diagnostics) {
-	return &baseEncryption{
+	base := &baseEncryption{
 		enc:      enc,
 		target:   target,
 		enforced: enforced,
 		name:     name,
 	}
+	// This performs a e2e validation run of the config -> methods flow.  It serves as a validation step and allows us to
+	// return detailed diagnostics here and simple errors below
+	_, diags := base.buildTargetMethods(make(map[keyprovider.Addr][]byte))
+	return base, diags
 }
 
 type basedata struct {
@@ -42,7 +46,8 @@ type basedata struct {
 	Version string                      `json:"encryption_version"` // This is both a sigil for a valid encrypted payload and a future compatability field
 }
 
-func (s *baseEncryption) encrypt(data []byte) ([]byte, hcl.Diagnostics) {
+func (s *baseEncryption) encrypt(data []byte) ([]byte, error) {
+	// No configuration provided, don't do anything
 	if s.target == nil {
 		return data, nil
 	}
@@ -55,6 +60,8 @@ func (s *baseEncryption) encrypt(data []byte) ([]byte, hcl.Diagnostics) {
 	// Mutates es.Meta
 	methods, diags := s.buildTargetMethods(es.Meta)
 	if diags.HasErrors() {
+		// This cast to error here is safe as we know that at least one error exists
+		// This is also quite unlikely to happen as the constructor already has checked this code path
 		return nil, diags
 	}
 
@@ -66,40 +73,27 @@ func (s *baseEncryption) encrypt(data []byte) ([]byte, hcl.Diagnostics) {
 	if encryptor == nil {
 		// ensure that the method is defined when Enforced is true
 		if s.enforced {
-			diags = append(diags, &hcl.Diagnostic{
+			return nil, fmt.Errorf("encryption of %q is enforced, and therefore requires a method to be provided", s.name)
-				Severity: hcl.DiagError,
-				Summary:  "Encryption method required",
-				Detail:   fmt.Sprintf("%q is enforced, and therefore requires a method to be provided", s.name),
-			})
-			return nil, diags
 		}
 		return data, nil
 	}
 
 	encd, err := encryptor.Encrypt(data)
 	if err != nil {
-		return nil, append(diags, &hcl.Diagnostic{
+		return nil, fmt.Errorf("encryption failed for %s: %w", s.name, err)
-			Severity: hcl.DiagError,
-			Summary:  "Encryption failed for " + s.name,
-			Detail:   err.Error(),
-		})
 	}
 
 	es.Data = encd
 	jsond, err := json.Marshal(es)
 	if err != nil {
-		return nil, append(diags, &hcl.Diagnostic{
+		return nil, fmt.Errorf("unable to encode encrypted data as json: %w", err)
-			Severity: hcl.DiagError,
-			Summary:  "Unable to encode encrypted data as json",
-			Detail:   err.Error(),
-		})
 	}
 
-	return jsond, diags
+	return jsond, nil
 }
 
 // TODO Find a way to make these errors actionable / clear
-func (s *baseEncryption) decrypt(data []byte, validator func([]byte) error) ([]byte, hcl.Diagnostics) {
+func (s *baseEncryption) decrypt(data []byte, validator func([]byte) error) ([]byte, error) {
 	if s.target == nil {
 		return data, nil
 	}
@@ -107,90 +101,68 @@ func (s *baseEncryption) decrypt(data []byte, validator func([]byte) error) ([]b
 	es := basedata{}
 	err := json.Unmarshal(data, &es)
 	if err != nil {
-		return nil, hcl.Diagnostics{&hcl.Diagnostic{
+		return nil, fmt.Errorf("invalid data format for decryption: %w", err)
-			Severity: hcl.DiagError,
-			Summary:  "Invalid data format for decryption",
-			Detail:   err.Error(),
-		}}
 	}
 
 	if len(es.Version) == 0 {
 		// Not a valid payload, might be already decrypted
 		err = validator(data)
-		if err == nil {
+		if err != nil {
-			// Yep, it's already decrypted
-			return data, nil
-		} else {
 			// Nope, just bad input
-			return nil, hcl.Diagnostics{&hcl.Diagnostic{
+			return nil, fmt.Errorf("unable to determine data structure during decryption: %w", err)
-				Severity: hcl.DiagError,
-				Summary:  "Unable to determine data structure during decryption",
-			}}
 		}
+		// Yep, it's already decrypted
+		return data, nil
 	}
 
 	if es.Version != encryptionVersion {
-		return nil, hcl.Diagnostics{&hcl.Diagnostic{
+		return nil, fmt.Errorf("invalid encrypted payload version: %s != %s", es.Version, encryptionVersion)
-			Severity: hcl.DiagError,
-			Summary:  "Invalid encrypted payload version",
-			Detail:   fmt.Sprintf("%s != %s", es.Version, encryptionVersion),
-		}}
 	}
 
 	methods, diags := s.buildTargetMethods(es.Meta)
 	if diags.HasErrors() {
+		// This cast to error here is safe as we know that at least one error exists
+		// This is also quite unlikely to happen as the constructor already has checked this code path
 		return nil, diags
 	}
 
 	if len(methods) == 0 {
 		err = validator(data)
-		if err == nil {
+		if err != nil {
-			// No methods/fallbacks specified and data is valid payload
-			return data, diags
-		} else {
 			// TODO improve this error message
-			return nil, append(diags, &hcl.Diagnostic{
+			return nil, err
-				Severity: hcl.DiagError,
-				Summary:  err.Error(),
-			})
 		}
+		// No methods/fallbacks specified and data is valid payload
+		return data, nil
 	}
 
-	var methodDiags hcl.Diagnostics
+	errs := make([]error, 0)
 	for _, method := range methods {
 		if method == nil {
 			// No method specified for this target
 			err = validator(data)
 			if err == nil {
-				return data, diags
+				return data, nil
 			}
-			// toDO improve this error message
+			// TODO improve this error message
-			methodDiags = append(methodDiags, &hcl.Diagnostic{
+			errs = append(errs, fmt.Errorf("payload is not already decrypted: %w", err))
-				Severity: hcl.DiagError,
-				Summary:  "Attempted decryption failed for " + s.name,
-				Detail:   err.Error(),
-			})
 			continue
 		}
 		uncd, err := method.Decrypt(es.Data)
 		if err == nil {
 			// Success
-			return uncd, diags
+			return uncd, nil
 		}
 		// Record the failure
-		methodDiags = append(methodDiags, &hcl.Diagnostic{
+		errs = append(errs, fmt.Errorf("attempted decryption failed for %s: %w", s.name, err))
-			Severity: hcl.DiagError,
-			Summary:  "Attempted decryption failed for " + s.name,
-			Detail:   err.Error(),
-		})
 	}
 
-	// Record the overall failure
+	// This is good enough for now until we have better/distinct errors
-	diags = append(diags, &hcl.Diagnostic{
+	errMessage := "decryption failed for all provided methods: "
-		Severity: hcl.DiagError,
+	sep := ""
-		Summary:  "Decryption failed",
+	for _, err := range errs {
-		Detail:   "All methods of decryption provided failed for " + s.name,
+		errMessage += err.Error() + sep
-	})
+		sep = "\n"
-
+	}
-	return nil, append(diags, methodDiags...)
+	return nil, fmt.Errorf(errMessage)
 }

internal/encryption/encryption.go

@@ -6,6 +6,7 @@
 package encryption
 
 import (
+	"github.com/hashicorp/hcl/v2"
 	"github.com/opentofu/opentofu/internal/encryption/config"
 	"github.com/opentofu/opentofu/internal/encryption/registry"
 )
@@ -14,17 +15,17 @@ import (
 // purpose. If no encryption configuration is present, it should return a pass through method that doesn't do anything.
 type Encryption interface {
 	// StateFile produces a StateEncryption overlay for encrypting and decrypting state files for local storage.
-	StateFile() StateEncryption
+	StateFile() (StateEncryption, hcl.Diagnostics)
 
 	// PlanFile produces a PlanEncryption overlay for encrypting and decrypting plan files.
-	PlanFile() PlanEncryption
+	PlanFile() (PlanEncryption, hcl.Diagnostics)
 
 	// Backend produces a StateEncryption overlay for storing state files on remote backends, such as an S3 bucket.
-	Backend() StateEncryption
+	Backend() (StateEncryption, hcl.Diagnostics)
 
 	// RemoteState produces a ReadOnlyStateEncryption for reading remote states using the terraform_remote_state data
 	// source.
-	RemoteState(string) ReadOnlyStateEncryption
+	RemoteState(string) (ReadOnlyStateEncryption, hcl.Diagnostics)
 }
 
 type encryption struct {
@@ -41,34 +42,27 @@ func New(reg registry.Registry, cfg *config.Config) Encryption {
 	}
 }
 
-func (e *encryption) StateFile() StateEncryption {
+func (e *encryption) StateFile() (StateEncryption, hcl.Diagnostics) {
-	return &stateEncryption{
+	return newStateEncryption(e, e.cfg.StateFile.AsTargetConfig(), e.cfg.StateFile.Enforced, "statefile")
-		base: newBaseEncryption(e, e.cfg.StateFile.AsTargetConfig(), e.cfg.StateFile.Enforced, "statefile"),
-	}
 }
 
-func (e *encryption) PlanFile() PlanEncryption {
+func (e *encryption) PlanFile() (PlanEncryption, hcl.Diagnostics) {
-	return &planEncryption{
+	return newPlanEncryption(e, e.cfg.PlanFile.AsTargetConfig(), e.cfg.PlanFile.Enforced, "planfile")
-		base: newBaseEncryption(e, e.cfg.PlanFile.AsTargetConfig(), e.cfg.PlanFile.Enforced, "planfile"),
-	}
 }
 
-func (e *encryption) Backend() StateEncryption {
+func (e *encryption) Backend() (StateEncryption, hcl.Diagnostics) {
-	return &stateEncryption{
+	return newStateEncryption(e, e.cfg.StateFile.AsTargetConfig(), e.cfg.StateFile.Enforced, "backend")
-		base: newBaseEncryption(e, e.cfg.StateFile.AsTargetConfig(), e.cfg.StateFile.Enforced, "backend"),
-	}
 }
 
-func (e *encryption) RemoteState(name string) ReadOnlyStateEncryption {
+func (e *encryption) RemoteState(name string) (ReadOnlyStateEncryption, hcl.Diagnostics) {
 	for _, remoteTarget := range e.cfg.Remote.Targets {
 		if remoteTarget.Name == name {
-			return &stateEncryption{
+			// TODO the addr here should be generated in one place.
-				// TODO the addr here should be generated in one place.
+			addr := "remote.remote_state_datasource." + remoteTarget.Name
-				base: newBaseEncryption(e, remoteTarget.AsTargetConfig(), false, "remote.remote_state_datasource."+remoteTarget.Name),
+			return newStateEncryption(
-			}
+				e, remoteTarget.AsTargetConfig(), false, addr,
+			)
 		}
 	}
-	return &stateEncryption{
+	return newStateEncryption(e, e.cfg.Remote.Default, false, "remote.default")
-		base: newBaseEncryption(e, e.cfg.Remote.Default, false, "remote.default"),
-	}
 }

internal/encryption/example_test.go

@@ -64,11 +64,16 @@ func Example() {
 	// Construct the encryption object
 	enc := encryption.New(reg, cfg)
 
+	sfe, diags := enc.StateFile()
+	handleDiags(diags)
+
 	// Encrypt the data, for this example we will be using the string "test",
 	// but in a real world scenario this would be the plan file.
 	sourceData := []byte("test")
-	encrypted, diags := enc.StateFile().EncryptState(sourceData)
+	encrypted, err := sfe.EncryptState(sourceData)
-	handleDiags(diags)
+	if err != nil {
+		panic(err)
+	}
 
 	if string(encrypted) == "test" {
 		panic("The data has not been encrypted!")
@@ -77,7 +82,7 @@ func Example() {
 	println(string(encrypted))
 
 	// Decrypt
-	decryptedState, err := enc.StateFile().DecryptState(encrypted)
+	decryptedState, err := sfe.DecryptState(encrypted)
 	if err != nil {
 		panic(err)
 	}

internal/encryption/plan.go

@@ -9,6 +9,7 @@ import (
 	"fmt"
 
 	"github.com/hashicorp/hcl/v2"
+	"github.com/opentofu/opentofu/internal/encryption/config"
 )
 
 // PlanEncryption describes the methods that you can use for encrypting a plan file. Plan files are opaque values with
@@ -27,7 +28,7 @@ type PlanEncryption interface {
 	// Make sure that you pass a valid plan file as an input. Failing to provide a valid plan file may result in an
 	// error. However, output values may not be valid plan files and you should not pass the encrypted plan file to any
 	// additional functions that normally work with plan files.
-	EncryptPlan([]byte) ([]byte, hcl.Diagnostics)
+	EncryptPlan([]byte) ([]byte, error)
 
 	// DecryptPlan decrypts an encrypted plan file.
 	//
@@ -41,18 +42,23 @@ type PlanEncryption interface {
 	//
 	// Pass a potentially encrypted plan file as an input, and you will receive the decrypted plan file or an error as
 	// a result.
-	DecryptPlan([]byte) ([]byte, hcl.Diagnostics)
+	DecryptPlan([]byte) ([]byte, error)
 }
 
 type planEncryption struct {
 	base *baseEncryption
 }
 
-func (p planEncryption) EncryptPlan(data []byte) ([]byte, hcl.Diagnostics) {
+func newPlanEncryption(enc *encryption, target *config.TargetConfig, enforced bool, name string) (PlanEncryption, hcl.Diagnostics) {
+	base, diags := newBaseEncryption(enc, target, enforced, name)
+	return &planEncryption{base}, diags
+}
+
+func (p planEncryption) EncryptPlan(data []byte) ([]byte, error) {
 	return p.base.encrypt(data)
 }
 
-func (p planEncryption) DecryptPlan(data []byte) ([]byte, hcl.Diagnostics) {
+func (p planEncryption) DecryptPlan(data []byte) ([]byte, error) {
 	return p.base.decrypt(data, func(data []byte) error {
 		// Check magic bytes
 		if len(data) < 4 || string(data[:4]) != "PK" {

internal/encryption/state.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 
 	"github.com/hashicorp/hcl/v2"
+	"github.com/opentofu/opentofu/internal/encryption/config"
 )
 
 const StateEncryptionMarkerField = "encryption"
@@ -32,7 +33,7 @@ type ReadOnlyStateEncryption interface {
 	// function. Do not attempt to determine if the state file is encrypted as this function will take care of any
 	// and all encryption-related matters. After the function returns, use the returned byte array as a normal state
 	// file.
-	DecryptState([]byte) ([]byte, hcl.Diagnostics)
+	DecryptState([]byte) ([]byte, error)
 }
 
 // StateEncryption describes the interface for encrypting state files.
@@ -56,18 +57,23 @@ type StateEncryption interface {
 	// Pass in a valid JSON-serialized state file as an input and store the output. Note that you should not pass the
 	// output to any additional functions that require a valid state file as it may not contain the fields typically
 	// present in a state file.
-	EncryptState([]byte) ([]byte, hcl.Diagnostics)
+	EncryptState([]byte) ([]byte, error)
 }
 
 type stateEncryption struct {
 	base *baseEncryption
 }
 
-func (s *stateEncryption) EncryptState(plainState []byte) ([]byte, hcl.Diagnostics) {
+func newStateEncryption(enc *encryption, target *config.TargetConfig, enforced bool, name string) (StateEncryption, hcl.Diagnostics) {
+	base, diags := newBaseEncryption(enc, target, enforced, name)
+	return &stateEncryption{base}, diags
+}
+
+func (s *stateEncryption) EncryptState(plainState []byte) ([]byte, error) {
 	return s.base.encrypt(plainState)
 }
 
-func (s *stateEncryption) DecryptState(encryptedState []byte) ([]byte, hcl.Diagnostics) {
+func (s *stateEncryption) DecryptState(encryptedState []byte) ([]byte, error) {
 	return s.base.decrypt(encryptedState, func(data []byte) error {
 		tmp := struct {
 			FormatVersion string `json:"format_version"`