-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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: HMAC support in Crypto APIs #43536
Conversation
Needs some testing still before merge but this is the general approach I am going for @Faless |
ab0433b
to
65c5d7b
Compare
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.
Some stylistic changes for documentation.
@Calinou thanks. I'm going through and trying to address all of the static check issues from CI. Once that's done, I'll accept your suggestions. |
c623c7e
to
362efe2
Compare
Can you provide a test project? It'll make testing faster. |
@fire yep i def plan to. Still need to do some testing myself though as mentioned above. Pulled an all nighter to implement this though, so ill do so after i rest a bit more haha |
a1ee02e
to
7d9c84d
Compare
@fire I opted instead to create unit tests for everything. If you feel an example project is still useful, I can whip one together though. let me know. |
14111fe
to
643c530
Compare
@akien-mga @Faless oh cool. Ill work on adding thise soon then |
5492de7
to
38cc3f7
Compare
@Calinou @akien-mga @Faless this is ready again for y'all. |
print(hmac.hex_encode()) | ||
|
||
[/gdscript] | ||
[csharp] |
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.
@Calinou @akien-mga I'm not super familiar with Godot's C# jazz. Mind checking my work here?
0c80d36
to
84f7009
Compare
@Faless I've addressed all of your feedback. This is ready for you again. :) |
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.
See my comments
core/crypto/crypto.cpp
Outdated
@@ -90,6 +125,8 @@ void Crypto::_bind_methods() { | |||
ClassDB::bind_method(D_METHOD("verify", "hash_type", "hash", "signature", "key"), &Crypto::verify); | |||
ClassDB::bind_method(D_METHOD("encrypt", "key", "plaintext"), &Crypto::encrypt); | |||
ClassDB::bind_method(D_METHOD("decrypt", "key", "ciphertext"), &Crypto::decrypt); | |||
ClassDB::bind_method(D_METHOD("hmac_digest", "hash_type", "key", "msg"), &Crypto::hmac_digest); | |||
ClassDB::bind_method(D_METHOD("constant_time_compare", "hm1", "hm2"), &Crypto::constant_time_compare); |
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 was wondering if we should rename the parameters to something more sensible. Would byte_array
/other_byte_array
make sense?
There is also the case where the 2 sizes do not match, so a timing attack could still successfully discover the size of the compared value.
Would it make sense to call the first parameter trusted_bytes
and the second one received_bytes
, so we can, in the future maybe, improve it knowing which one the user consider trusted (which I think is how safe_memcmp
s work, but I'm no expert, so the function is fine for now).
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 renamed the parameters for the function. I didn't do anything about the size comparison shortcut though, as we discussed on IRC.
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 seems you changed the name of the function parameters, not the one in the method_bind
(which are the names used by the doc generation tool).
rename hm1
and hm2
in bind_method
to: trusted
, received
.
Possibly rename trusted
, received
in the method definition to p_trusted
, p_received
.
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.
And remember to re-run the doctool to update the XML
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.
@Faless done. Let me know if you see anything else. Thanks!
Ready again for you @Faless |
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.
Almost ready. See my comment about the parameter renames.
Looks good otherwise.
Not sure if the @godotengine/documentation team has some other remarks.
Please remember to squash your commits afterwards: https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase
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.
Great work! 🥇
Thanks! |
fixes: godotengine/godot-proposals#1098
Couple different ways to use this. The more low level approach (especially useful for HMACing streamed content:
Or the more high level approach that is going to be most useful 99% of the time:
I've also provided a convenience function for comparing two HMAC digests in a way that is impervious to timing attacks: