Conversation
Add integration tests that reproduce opentdf/platform#3070 where EC decrypt (rewrap) fails for P-384 and P-521 keys because UncompressECPubKey hardcodes elliptic.P256(). Fixtures: - managed_key_km2_ec_p384 / managed_key_km2_ec_p521: managed keys - base_key_ec384 / base_key_ec521: base key configurations Tests: - test_encrypt_decrypt_with_base_key_ec384: encrypt/decrypt with P-384 - test_encrypt_decrypt_with_base_key_ec521: encrypt/decrypt with P-521 These tests will fail against the unfixed platform with: "ecdh failure: ecdsa: invalid public key" Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn@virtru.com>
pflynn-virtru
changed the title
Summary of ChangesHello @pflynn-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a set of integration tests designed to expose and validate a fix for a critical bug affecting Elliptic Curve P-384 and P-521 key decryption. By simulating the problematic scenario, these tests ensure that the platform correctly handles these cryptographic operations after the underlying issue is resolved, improving the robustness of the key management system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces new fixtures and integration tests to address a critical bug related to EC P-384/P-521 key handling during decryption. The new tests effectively reproduce the reported issue and are well-documented. While the functionality is correct and addresses the bug, there is a significant amount of code duplication across the newly added fixtures and test cases. Refactoring these into more generic, parameterized functions or fixtures would greatly improve maintainability and readability. This is a common pattern in pytest, and applying it here would make future additions of similar key types much simpler.
X-Test Failure Report✅ js-main |
…ation Address review feedback and broaden #3070 test coverage: - Remove xfail markers (TDD: tests must fail visibly against unfixed platform) - Parametrize base key test across all container types (ztdf + ztdf-ecwrap) - Add multi-KAS test with P-384 (km1) + P-521 (km2) attribute-level assignment - Extract _get_or_create_managed_key and _ensure_base_key shared helpers - Remove unused managed_key_km2_ec_p384/p521 fixtures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Paul Flynn <pflynn@virtru.com>
|
|
/gemini review |
pflynn-virtru
changed the title
pflynn-virtru
changed the title
There was a problem hiding this comment.
Code Review
This pull request adds integration tests for EC P-384 and P-521 curves, which is a great addition to reproduce and verify the fix for the reported issue. The refactoring to reduce code duplication in the test fixtures is well-executed and improves the maintainability of the test suite. I have one suggestion to make a test fixture more robust.
| ["p384", "p521"], | ||
| ) | ||
| assert attr.values and len(attr.values) == 2 | ||
| v384, v521 = attr.values |
There was a problem hiding this comment.
The unpacking of attr.values assumes a specific order of the returned values, which might not be guaranteed by the underlying API. This could lead to brittle tests that fail if the order changes. Sorting the values before unpacking will make this more robust.
| v384, v521 = attr.values | |
| v384, v521 = sorted(attr.values, key=lambda v: v.value) |
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def base_key_ec521( |
There was a problem hiding this comment.
I'd recommend against using base_keys for testing most things, since they are global tests that use them must be sequenced carefully
| assert filecmp.cmp(pt_file, rt_file) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
ooh I need to learn how to use parameterize properly. I've also been thinking about using subtests
X-Test Failure Report✅ js-main |
2 participants
Summary
Adds integration tests reproducing opentdf/platform#3070 where EC P-384/P-521 decrypt (rewrap) fails because
UncompressECPubKeyhardcodeselliptic.P256()instead of using the passedcurveparameter.These tests are written TDD-style — they will fail against the unfixed platform with:
Related platform PR: opentdf/platform#3071
What's included
Fixtures (
xtest/fixtures/keys.py)_get_or_create_managed_key()— used by all managed key fixtures_ensure_base_key()— used by all base key fixturesmanaged_key_km1_ec384— EC P-384 on km1managed_key_km2_ec521— EC P-521 on km2base_key_ec384— EC P-384 base key on km1base_key_ec521— EC P-521 base key on km1attribute_allof_with_ec384_and_ec521_keys— ALL_OF attribute with P-384 (km1) + P-521 (km2) keys at attribute levelTests (
xtest/test_abac.py)test_encrypt_decrypt_all_containers_with_base_key_ec_curve[P-384]test_encrypt_decrypt_all_containers_with_base_key_ec_curve[P-521]test_autoconfigure_key_management_ec384_ec521Coverage matrix
test_..._base_key_e1test_..._ec_curve[P-384]test_..._ec_curve[P-521]test_..._two_kas_two_keystest_..._ec384_ec521test_..._ec384_ec521Test plan
ecdh failure: ecdsa: invalid public keylib/ocrypto/ec_key_pair.go(Curve: curveinstead ofCurve: elliptic.P256()) and verify all tests pass🤖 Generated with Claude Code