[Misc]Fix static analysis issues#561
Conversation
There was a problem hiding this comment.
Code Review
This pull request performs various refactors, such as replacing deprecated ioutil functions with os equivalents, optimizing string comparisons using strings.EqualFold, and standardizing error message casing. Feedback highlights a critical breaking change in the vault utility where switching from CFB to CTR encryption mode without renaming functions will break existing data decryption and lead to misleading API names. Additionally, it is recommended to return errors instead of panicking during JSON unmarshaling in the metrics aggregator to improve error handling.
| } | ||
|
|
||
| stream := cipher.NewCFBEncrypter(block, iv) | ||
| stream := cipher.NewCTR(block, iv) |
There was a problem hiding this comment.
The implementation has been switched from CFB to CTR mode, but the function name CFBEncrypt (and CFBDecrypt at line 77) remains unchanged. This is misleading. More importantly, CTR and CFB are incompatible encryption modes; this change will break decryption for any data previously encrypted with the CFB implementation. If a migration to CTR is intended, the functions should be renamed and a transition plan for existing ciphertexts should be established. If compatibility is required, you should continue using CFB despite its deprecation, or use an AEAD mode like GCM with a proper migration.
There was a problem hiding this comment.
Go's deprecation message for CFB suggests CTR as a replacement only because CTR is the "least bad" unauthenticated stream mode (faster, parallelizable, FIPS-validated).
In the code, CFB Only called in util.go (definition) and util_test.go (tests). Zero production callers. No code ever encrypts data with CFB, so no encrypted data exists in the wild.
so deprecated it
| err := json.Unmarshal([]byte(maConfigVal), &ma) | ||
| if err != nil { | ||
| panic(fmt.Errorf("Unable to unmarshall %v json string due to %w ", MetricsAggregatorConfigMapKeyName, err)) | ||
| panic(fmt.Errorf("unable to unmarshall %v json string due to %w ", MetricsAggregatorConfigMapKeyName, err)) |
There was a problem hiding this comment.
The function newMetricsAggregator returns an error as its second return value, but it currently panics when JSON unmarshaling fails. It is better practice to return the error to the caller instead of panicking, allowing for more graceful error handling.
| panic(fmt.Errorf("unable to unmarshall %v json string due to %w ", MetricsAggregatorConfigMapKeyName, err)) | |
| return nil, fmt.Errorf("unable to unmarshall %v json string due to %w ", MetricsAggregatorConfigMapKeyName, err) |
4cb280c to
507e07e
Compare
507e07e to
aa38d08
Compare
What this PR does
pkg/: deprecated APIs, redundant code, style violationscipher.NewCFBEncrypter/NewCFBDecrypterwithcipher.NewCTRin vault utilsio/ioutilwith os package equivalentslen()in configmap reconciler (S1009)strings.EqualFoldinstead of manual strings.ToLower comparison (SA6005)Deprecation ciper.NewCFBEncrypter/NewCFBDecrypter:
Why we need it
Fixes #556
How to test
Checklist
make testpasses locally