-
-
Notifications
You must be signed in to change notification settings - Fork 745
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
Changes in the hash algorithm #5161
base: master
Are you sure you want to change the base?
Changes in the hash algorithm #5161
Conversation
SHA-1 is not used in encryption, it is used in HMAC, which is, to my knowledge, still currently considered secure. We should walk, but not necessarily run, away from SHA1 as HMAC. |
Thanks for the contribution. As @blag pointed out, SHA1 is used for MAC and not for actual symmetric encryption. In theory, I'm fine with changing it, but in case we do decide to change the MAC algorithm we should re-evaluate if the current implementation (both for the MAC and actual symmetric encryption) is indeed (still recommended) and a safe choice - and if there are more things we should change, we should change those as well (but of course since it's a security sensitive code, we need to be careful). And if we do it, maybe we should just use sha512 to be future proof - that code is not used in that many places so I don't think it should add tons of overhead. We used to use keyczar which is not supported anymore so we migrated the same code and algorithms to As far as implementation and roll out goes - StackStorm is a distributed system which means we should approach all such changes in a specific manner to ensure a consistent roll out without issues (it basically means that the code needs to support both versions for a while aka until changes are rolled out to all the services and migration script finishes). We don't have database object versioning in place and we also can't rely on a migration script by itself - the code needs to support both implementations - basically on read we need to support sha1 and sha256, but when the value is updated, we should write it out in a new format. This way we also don't need to add another database field. And if we want to make sure all the values are indeed migrated in a reasonable time frame, we should instruct users to run a migration script or perhaps go with the read-repair approach (on the read, if it still uses a new format, re-write the object with a new format - probably the migration script is better and easier to do). |
The comment about backward compatibility makes a lot of sense to me 👍 |
I think the discussion should be focus on whether the sha1 function use for hmac poses security risk or tainted image of st2 on a vulnerability scan. On the lesser issue on migration, I need some explanation why users cannot run a migration script during st2 upgrade. User just run the script to re-encrypt KVP in the datastore and move on. We've asked users to run migration script before in prior releases. Why is this an issue now? |
There's better understanding on the issue here after some discussion. The data is still encrypted with AES-256 per docs at https://docs.stackstorm.com/datastore.html?highlight=encryption_key_path#securing-secrets-admin-only. The sha1 hash is used to generate the hmac hash to verify the encrypted text. So technically, this is low risk. However, per feedback, there could be collisions and change is still recommended because of the collision weaknesses. |
We just reformatted the code with black. (Hooray!) And this PR got caught in the cross fire too. (Arrgh!) |
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 don't think this is ready for 3.6.0, as it doesn't look finished to me. I suspect we'll need to discuss it more before we can merge it as well.
crypto_key = AESKey.generate() | ||
new_encrypted_value = symmetric_encrypt(crypto_key, plaintext) |
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 are we generating a new crypto_key for every keyvalue entry?
I would imagine that we should generate the new key once, and save it to some path, and then use that key to reencrypt all the values.
value = me.StringField() | ||
new_encrypted_value = me.StringField() |
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 is there a new field for the value?
I see where new_encrypted_value
gets written in the migration script, but I don't see anything else that consumes it (eg to copy it to the value
field after successfully reencrypting all the values).
@ashwini-orchestral Can you:
|
I'm marking this as a draft until it the implementation is complete. |
For security purposes, the hash function for symmetric encryption is changed ie. from SHA1 to SHA256. Also, a migration script is added to update the database with new encrypted values.