Skip to content
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

Update CryptoKey documentation to mention ECC. #91348

Merged
merged 1 commit into from
May 10, 2024

Conversation

basicer
Copy link
Contributor

@basicer basicer commented Apr 30, 2024

The documentation makes it seem like CryptoKey can only hold an RSA key. This is compounded by the fact that Cypto only has a function generate an RSA based key. Godot however is perfectly happy loading and using ECC based keys.

@basicer
Copy link
Contributor Author

basicer commented Apr 30, 2024

Example showing signing and verifying with an secp256k1 key.

var key = CryptoKey.new()
key.load_from_string("""-----BEGIN EC PRIVATE KEY-----
MHQCAQEEIGIumMFpusqAcqCkwN15oFS/QXNIf9LsmTCYMNwp9/G8oAcGBSuBBAAK
oUQDQgAEigp/7pvDMxn0mNDDuJuepWgWeXTw71jNk/aSo8LqyTSHXULaUbZh3G57
G3P3LqXcwRgm2Wz7J+9JoGzDokr1Pw==
-----END EC PRIVATE KEY-----""")

print(key.save_to_string())
print(key.save_to_string(true))
var pub = CryptoKey.new()
pub.load_from_string(key.save_to_string(true), true)

var ctx = HashingContext.new()
ctx.start(HashingContext.HASH_SHA256)
ctx.update("Hi!".to_ascii_buffer())
var hash = ctx.finish()


var sig = Crypto.new().sign(HashingContext.HASH_SHA256, hash, key)
print("SIG", sig)
print("Ok?", Crypto.new().verify(HashingContext.HASH_SHA256, hash, sig, pub))

@AThousandShips AThousandShips added this to the 4.x milestone Apr 30, 2024
@akien-mga akien-mga changed the title Update CryptoKey.xml to mention ECC. Update CryptoKey documentation to mention ECC. Apr 30, 2024
@akien-mga akien-mga requested a review from a team April 30, 2024 08:02
@mhilbrunner
Copy link
Member

mhilbrunner commented Apr 30, 2024

As we're just passing this through to mbedtls, I'd expect both RSA and ECC to work, yes. Adding a link to the Crypto class for actually generating keys would also make sense. We should also add ECC key generation there. :) Would be worth opening an issue for.

@basicer
Copy link
Contributor Author

basicer commented May 1, 2024

We should also add ECC key generation there.

I was looking into this, but held off for now. It needs a bit of API design on how the user specifies which curve they want to use and also how to enumerate the available curves along with their bit size.

@basicer basicer marked this pull request as ready for review May 1, 2024 03:19
@basicer basicer requested a review from a team as a code owner May 1, 2024 03:19
@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 8, 2024
@akien-mga
Copy link
Member

akien-mga commented May 8, 2024

Could you amend the commit message to be more explicit, like the PR title?

Edit: Let me know if you're not familiar with how to do this with Git, I can also do this update myself.

The documentation makes it seem like CryptoKey can only hold an RSA key.  This is compounded by the fact that Cypto only has a function generate an RSA based key.  Godot however is perfectly happy loading and using ECC based keys.
@basicer
Copy link
Contributor Author

basicer commented May 9, 2024

Could you amend the commit message to be more explicit, like the PR title?

Edit: Let me know if you're not familiar with how to do this with Git, I can also do this update myself.

Done :)

@akien-mga akien-mga merged commit 19219f7 into godotengine:master May 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants