From 997e5fa46e5b47abf4095d172fe7d9e16b1a7f31 Mon Sep 17 00:00:00 2001 From: Christian Mesh Date: Mon, 4 Mar 2024 08:30:30 -0500 Subject: [PATCH] State Encryption Error Handling / Diagnostics (#1294) Signed-off-by: Christian Mesh --- internal/encryption/base.go | 110 +++++++++++----------------- internal/encryption/encryption.go | 42 +++++------ internal/encryption/example_test.go | 11 ++- internal/encryption/plan.go | 14 +++- internal/encryption/state.go | 14 +++- 5 files changed, 87 insertions(+), 104 deletions(-) diff --git a/internal/encryption/base.go b/internal/encryption/base.go index 8e2b8de36f..3bf597a09f 100644 --- a/internal/encryption/base.go +++ b/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 { - return &baseEncryption{ +func newBaseEncryption(enc *encryption, target *config.TargetConfig, enforced bool, name string) (*baseEncryption, hcl.Diagnostics) { + 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{ - 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 nil, fmt.Errorf("encryption of %q is enforced, and therefore requires a method to be provided", s.name) } return data, nil } encd, err := encryptor.Encrypt(data) if err != nil { - return nil, append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Encryption failed for " + s.name, - Detail: err.Error(), - }) + return nil, fmt.Errorf("encryption failed for %s: %w", s.name, err) } es.Data = encd jsond, err := json.Marshal(es) if err != nil { - return nil, append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Unable to encode encrypted data as json", - Detail: err.Error(), - }) + return nil, fmt.Errorf("unable to encode encrypted data as json: %w", err) } - 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{ - Severity: hcl.DiagError, - Summary: "Invalid data format for decryption", - Detail: err.Error(), - }} + return nil, fmt.Errorf("invalid data format for decryption: %w", err) } if len(es.Version) == 0 { // Not a valid payload, might be already decrypted err = validator(data) - if err == nil { - // Yep, it's already decrypted - return data, nil - } else { + if err != nil { // Nope, just bad input - return nil, hcl.Diagnostics{&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Unable to determine data structure during decryption", - }} + return nil, fmt.Errorf("unable to determine data structure during decryption: %w", err) } + // Yep, it's already decrypted + return data, nil } if es.Version != encryptionVersion { - return nil, hcl.Diagnostics{&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid encrypted payload version", - Detail: fmt.Sprintf("%s != %s", es.Version, encryptionVersion), - }} + return nil, fmt.Errorf("invalid encrypted payload version: %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 { - // No methods/fallbacks specified and data is valid payload - return data, diags - } else { + if err != nil { // TODO improve this error message - return nil, append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: err.Error(), - }) + return nil, err } + // 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 - methodDiags = append(methodDiags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Attempted decryption failed for " + s.name, - Detail: err.Error(), - }) + // TODO improve this error message + errs = append(errs, fmt.Errorf("payload is not already decrypted: %w", err)) 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{ - Severity: hcl.DiagError, - Summary: "Attempted decryption failed for " + s.name, - Detail: err.Error(), - }) + errs = append(errs, fmt.Errorf("attempted decryption failed for %s: %w", s.name, err)) } - // Record the overall failure - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Decryption failed", - Detail: "All methods of decryption provided failed for " + s.name, - }) - - return nil, append(diags, methodDiags...) + // This is good enough for now until we have better/distinct errors + errMessage := "decryption failed for all provided methods: " + sep := "" + for _, err := range errs { + errMessage += err.Error() + sep + sep = "\n" + } + return nil, fmt.Errorf(errMessage) } diff --git a/internal/encryption/encryption.go b/internal/encryption/encryption.go index 2c9287ba23..664a3f6cf1 100644 --- a/internal/encryption/encryption.go +++ b/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 { - return &stateEncryption{ - base: newBaseEncryption(e, e.cfg.StateFile.AsTargetConfig(), e.cfg.StateFile.Enforced, "statefile"), - } +func (e *encryption) StateFile() (StateEncryption, hcl.Diagnostics) { + return newStateEncryption(e, e.cfg.StateFile.AsTargetConfig(), e.cfg.StateFile.Enforced, "statefile") } -func (e *encryption) PlanFile() PlanEncryption { - return &planEncryption{ - base: newBaseEncryption(e, e.cfg.PlanFile.AsTargetConfig(), e.cfg.PlanFile.Enforced, "planfile"), - } +func (e *encryption) PlanFile() (PlanEncryption, hcl.Diagnostics) { + return newPlanEncryption(e, e.cfg.PlanFile.AsTargetConfig(), e.cfg.PlanFile.Enforced, "planfile") } -func (e *encryption) Backend() StateEncryption { - return &stateEncryption{ - base: newBaseEncryption(e, e.cfg.StateFile.AsTargetConfig(), e.cfg.StateFile.Enforced, "backend"), - } +func (e *encryption) Backend() (StateEncryption, hcl.Diagnostics) { + return newStateEncryption(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. - base: newBaseEncryption(e, remoteTarget.AsTargetConfig(), false, "remote.remote_state_datasource."+remoteTarget.Name), - } + // TODO the addr here should be generated in one place. + addr := "remote.remote_state_datasource." + remoteTarget.Name + return newStateEncryption( + e, remoteTarget.AsTargetConfig(), false, addr, + ) } } - return &stateEncryption{ - base: newBaseEncryption(e, e.cfg.Remote.Default, false, "remote.default"), - } + return newStateEncryption(e, e.cfg.Remote.Default, false, "remote.default") } diff --git a/internal/encryption/example_test.go b/internal/encryption/example_test.go index 397036b0eb..d2ef2e1db1 100644 --- a/internal/encryption/example_test.go +++ b/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) - handleDiags(diags) + encrypted, err := sfe.EncryptState(sourceData) + 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) } diff --git a/internal/encryption/plan.go b/internal/encryption/plan.go index 72122fccd1..7e36cbbdf0 100644 --- a/internal/encryption/plan.go +++ b/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" { diff --git a/internal/encryption/state.go b/internal/encryption/state.go index 27a2dd65e7..d42c65ce9e 100644 --- a/internal/encryption/state.go +++ b/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"`