Conversation
Xavrax
left a comment
Xavrax
left a comment
There was a problem hiding this comment.
Some questions, overall okay.
| type ExtendedCryptor interface { | ||
| Cryptor | ||
| EncryptStream(reader io.Reader) (*EncryptedStreamData, error) | ||
| DecryptStream(encryptedData *EncryptedStreamData) (io.Reader, error) | ||
| } |
There was a problem hiding this comment.
I like that extended interface. Imo it works much better than having a big one cryptor - especially that file crypto is something a little bit different than raw data.
| return nil, e | ||
| } | ||
|
|
||
| _, e = buffer.Write([]byte(cryptorId)) |
There was a problem hiding this comment.
I might be wrong, but in our documented example, there is a possibility that we have V1 header for legacy and we don't add the ID into the meta. Here we always write it - is that expected or did I misunderstand something?
There was a problem hiding this comment.
We should not add header at all for legacy therefore this won't be called with legacy cryptorId
| if readErr != nil && readErr != io.EOF { | ||
| return output[:sizeOfCurrentlyRead], readErr | ||
| } |
There was a problem hiding this comment.
Should we return any output in case of error?
There was a problem hiding this comment.
Probably not. I will remove it and if there's any problem report back to you to figure out what to do
| return output, nil | ||
| } | ||
|
|
||
| func (decryptingReader *blockModeDecryptingReader) decryptUntilPFull(p []byte) (n int, err error) { |
There was a problem hiding this comment.
as above - should we return anything in case of error?
Is having any return (even info about how much data have been read) viable for users?
There was a problem hiding this comment.
As above. I'll check it and let you know if I have problems
| var e error | ||
| var readBytes int | ||
|
|
||
| for e == nil { |
There was a problem hiding this comment.
I'm assuming that it waits until encryptingReader.Read() will return error.EOF, am I right?
Maybe it would be better to wait for concrete error?
What will happen if any other error will appear? Will we return false in case of any error?
There was a problem hiding this comment.
I will end the loop for any error just like that but return false for any error other than EOF. Is that alright?
| if err != nil { | ||
| return []byte{}, err | ||
| } | ||
| msg, err := utils.ValueAsString(enc) //TODO WHAT?! |
There was a problem hiding this comment.
What?! was I thinking 🤔
| previousIvFlag bool | ||
| } | ||
|
|
||
| // TODO this needs to be tested |
There was a problem hiding this comment.
How should we test getter function?
There was a problem hiding this comment.
As you can see it's kind of special getter function. It has actually a lot of things going on inside. Hopefully I figure out some not that awful way to test it
There was a problem hiding this comment.
So the only thing we can test is that we're getting nil in proper client configs. I think that there is no better way to do that ;/
Co-authored-by: Mateusz Dahlke <39696234+Xavrax@users.noreply.github.com>
Tests in this file have been order dependent. I broke those dependencies by introducing few additional lines of code in each an every one. And the initHistoryOpts now requires passing pn, so maybe whoever decide to write new tests will realize that it might be nice to create new pubnub and new config instead of reusing them across every test.
In case of cipher key being present we would drop users message and encrypt only padding
|
@pubnub-release-bot release |
|
🚀 Release successfully completed 🚀 |
Tests in this file have been order dependent. I broke those dependencies by introducing few additional lines of code in each an every one. And the initHistoryOpts now requires passing pn, so maybe whoever decide to write new tests will realize that it might be nice to create new pubnub and new config instead of reusing them across every test. Co-authored-by: Mateusz Dahlke <39696234+Xavrax@users.noreply.github.com>
3 participants
feat(crypto): Add crypto module
Update the crypto module structure and add enhanced AES-CBC cryptor.
fix(crypto): Improved security of crypto implementation by increasing the cipher key entropy by a factor of two.
Improved security of crypto implementation by increasing the cipher key entropy by a factor of two.