fix(resolution): restore hex.Decode/Encode when loading/storing ciphered data from database #395
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…red data from database Previously, we were using symmecrypt.DecryptMarshal to load data from the database, and we switched to symmecrypt.Decrypt with bf23fbb. This changed the behaviour because DecryptMarshal was also calling hex.Decode on the ciphered text before doing the Decrypt. We had to restore this behaviour to read old data from our database. Same for EncryptMarshal/Encrypt/hex.Encode Signed-off-by: Romain Beuque <556072 [email protected]>
Inozuma
requested changes
Nov 22, 2022
models/resolution/resolution.go
Outdated
| @@ -166,6 167,9 @@ func Create(dbp zesty.DBProvider, t *task.Task, resolverInputs map[string]interf | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
|
|
|||
| dst := make([]byte, 0, hex.EncodedLen(len(encryptedSteps))) | |||
There was a problem hiding this comment.
Suggested change
| dst := make([]byte, 0, hex.EncodedLen(len(encryptedSteps))) | |
| dst := make([]byte, hex.EncodedLen(len(encryptedSteps))) |
From the official documentation, it uses the size of the buffer: https://pkg.go.dev/encoding/hex#Encode
There was a problem hiding this comment.
Note: (correcting myself) the functions uses the byte slice indexes, and not append, so the slice must be of the proper length, enough capacity with a 0 length won't work.
models/resolution/resolution.go
Outdated
| @@ -377,6 391,8 @@ func (r *Resolution) Update(dbp zesty.DBProvider) (err error) { | |||
| return err | |||
| } | |||
|
|
|||
| dst := make([]byte, 0, hex.EncodedLen(len(compressedSteps))) | |||
There was a problem hiding this comment.
Suggested change
| dst := make([]byte, 0, hex.EncodedLen(len(compressedSteps))) | |
| dst := make([]byte, hex.EncodedLen(len(compressedSteps))) |
models/resolution/resolution.go
Outdated
| @@ -247,7 251,17 @@ func load(dbp zesty.DBProvider, publicID string, locked bool, lockNoWait bool) ( | |||
| return nil, err | |||
| } | |||
|
|
|||
| compressedSteps, err := models.EncryptionKey.Decrypt(r.EncryptedSteps, []byte(r.PublicID)) | |||
| dst := make([]byte, 0, hex.DecodedLen(len(r.EncryptedSteps))) | |||
There was a problem hiding this comment.
Suggested change
| dst := make([]byte, 0, hex.DecodedLen(len(r.EncryptedSteps))) | |
| dst := make([]byte, hex.DecodedLen(len(r.EncryptedSteps))) |
models/resolution/resolution.go
Outdated
| // often. | ||
| // See https://github.com/ovh/utask/commit/bf23fbb10b62bb487ac4ea01b1e519f85480e58b and migration | ||
| // from symmecrypt.Key.DecryptMarshal to symmecrypt.Key.Decrypt | ||
| _, _ = hex.Decode(dst, r.EncryptedSteps) |
There was a problem hiding this comment.
Should probably check returned error and set dst to r.EncryptedSteps if there was an error.
Signed-off-by: Thomas Bétrancourt <[email protected]>
Inozuma
approved these changes
Nov 22, 2022
wI2L
approved these changes
Nov 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Romain Beuque 556072 [email protected]
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix
What is the current behavior? (You can also link to an open issue here)
Failed to load resolution from public id: chacha20poly1305: message authentication failedWhat is the new behavior (if this is a feature change)?
Previously, we were using symmecrypt.DecryptMarshal to load data from the database, and we switched to symmecrypt.Decrypt with bf23fbb.
This changed the behaviour because DecryptMarshal was also calling hex.Decode on the ciphered text before doing the Decrypt. We had to restore this behaviour to read old data from our database.
Same for EncryptMarshal/Encrypt/hex.Encode
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No
Other information: