feat: Decrypt input secrets if there are some #45
Conversation
drobnikj
commented
Feb 1, 2023
•
drobnikj
commented
- Missing unit tests, I will do it once feat: add the rest unit tests for actor #40 was merged.
github-actions
bot
added
the
t-platform
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request is neither linked to an issue or epic nor labeled as adhoc!
|
Yeah, I really need a spell checker in my vs code 😄 sorry about that. |
Co-authored-by: František Nesveda <[email protected]>
There was a problem hiding this comment.
Pull request is neither linked to an issue or epic nor labeled as adhoc!
There was a problem hiding this comment.
Pull request is neither linked to an issue or epic nor labeled as adhoc!
| password_bytes, | ||
| padding.OAEP( | ||
| mgf=padding.MGF1(algorithm=hashes.SHA1()), | ||
| algorithm=hashes.SHA1(), |
There was a problem hiding this comment.
SHA-1 is a deprecated hash algorithm that has practical known collision attacks. You are strongly discouraged from using it. Existing applications should strongly consider moving away.
There was a problem hiding this comment.
You are right, it is deprecated for the encryption, but in this case, we are using sha1 for padding and it is not the issue. In short, sha1 is there used for generating a hash which fills the block of the message for encryption. It is quite complex stuff, but the message itself together with the hash is then encrypted using the public key(RSA). There is a nice picture and explanation of how OAEP works.
The sha1 is used to This combination(MGF1 + SHA1 for padding) for RSA encryption OAEP (RSA_PKCS1_OAEP_PADDING) is recommended by open SSL, and it is used for example in node js by default. As it is used in node js by default.
| password_bytes = key_bytes + initialized_vector_bytes | ||
|
|
||
| # NOTE: Auth Tag is appended to the end of the encrypted data, it has length of 16 bytes and ensures integrity of the data. | ||
| cipher = Cipher(algorithms.AES(key_bytes), modes.GCM(initialized_vector_bytes, min_tag_length=ENCRYPTION_AUTH_TAG_LENGTH)) |
There was a problem hiding this comment.
min_tag_length (int) – The minimum length tag must be. By default this is 16, meaning tag truncation is not allowed. Allowing tag truncation is strongly discouraged for most applications.
min_tag_length is 16 by default, why override with 16?
There was a problem hiding this comment.
Just to be sure that default will change the decryption still work. We are strictly setting tag to 16 chars in length same as in node js version of this method.
| label=None, | ||
| ), | ||
| ) | ||
| return { |
There was a problem hiding this comment.
Why do you .decode('utf-8') here? Do you need the values as strings or?
There was a problem hiding this comment.
Yes, I need the value as a string. The function is aligned with the same one from JS for better accountability. Basically, we didn't use this function in python yet. The function is there mainly for testing and maybe for the future.
| Returns: | ||
| str: Decrypted value. | ||
| """ | ||
| encrypted_password_bytes = base64.b64decode(encrypted_password.encode('utf-8')) |
There was a problem hiding this comment.
I think if you didn't decode in encrypt, you can omit encode here?
There was a problem hiding this comment.
It is true. But the encrypted secret(password and value) is stored in input JSON, and the base64 string is better for handling as it has a subset of characters and we can easily match it.
fnesveda
added
the
validated
