Skip to content

feat: add test for RSA 4096 operability#488

Draft
TylerTrott wants to merge 7 commits intotrunkfrom
tt/rsa_4096
Draft

feat: add test for RSA 4096 operability#488
TylerTrott wants to merge 7 commits intotrunkfrom
tt/rsa_4096

Conversation

@TylerTrott
Copy link
Copy Markdown

- What I did
Added test that verifies at_c's capability of withstanding a 512-byte key

- How I did it
Updated verify func to determine if 256 or 512-byte key is used, created a test that uses a 512-byte generated public key

- How to verify it
Run the tests and verify that tests pass with

cmake -S . -B build -DATSDK_BUILD_TESTS="unit" && cmake --build build --target all && ./build/packages/atchops/tests/test_rsa_verify

- Description for the changelog
feat: tests for RSA 4096

@TylerTrott TylerTrott self-assigned this Dec 30, 2024
@TylerTrott TylerTrott changed the title Tt/rsa 4096 feat: add test for RSA 4096 operability Dec 31, 2024
@TylerTrott TylerTrott removed the request for review from realvarx January 2, 2025 16:06
Comment on lines -71 to -80
/**
* @brief generate an RSA keypair
*
* @param public_key the public key struct to populate, should be initialized first
* @param private_key the private key struct to populate, should be initialized first
* @param keysize the size of the key to generate, e.g. 2048
*/
int atchops_rsa_generate(atchops_rsa_key_public_key *public_key, atchops_rsa_key_private_key *private_key,
const atchops_md_type md_type);

Copy link
Copy Markdown
Member

@xavierchanth xavierchanth Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was removed from the public API with no replacement for its functionality.

Copy link
Copy Markdown
Author

@TylerTrott TylerTrott Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is redundant code, tests pass without it as no code utilises the func. I believe @JeremyTubongbanua has a different way to generate key pair elsewhere

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int atchops_rsa_key_generate(atchops_rsa_key_public_key *public_key, atchops_rsa_key_private_key *private_key);

generation code is in rsa_key.h/.c

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we added support for specifying the size of the key when we generate?

Copy link
Copy Markdown
Author

@TylerTrott TylerTrott Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not from an end-developer PoV, I suppose. The way the code is structured allows for 256 or 512-sized keys dynamically and is documented in the code, but if the sdk is utilised in the CLI, I don't think this would be known

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, I wonder what's gone wrong with my test then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, for generate, it's probably only testing rsa2048. but for hard-code 4096 keys in the tests, everything (in theory) should be working fine

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to decrypt the ciphertext after, and check that the value is the same though.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or since the key is hardcoded in the tests at least check against the expected ciphertext

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to decrypt the ciphertext after, and check that the value is the same though.

I'll write a test for that such that it'll check against what the expected ciphertext would be

Copy link
Copy Markdown
Member

@xavierchanth xavierchanth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants