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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Decrypt input secrets if there are some #45
feat: Decrypt input secrets if there are some #45
Changes from 1 commit
80ce441
b4b23d8
1408ea7
184c963
1442fda
3f49bd2
ad19f3c
73f642d
5566410
dbbef6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
https://cryptography.io/en/latest/hazmat/primitives/cryptographic-hashes/#cryptography.hazmat.primitives.hashes.SHA1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you
.decode('utf-8')
here? Do you need the values as strings or?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you didn't decode in encrypt, you can omit encode here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.