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

Add ability to use custom IV in AES/GCM #38

Merged
merged 3 commits into from
Sep 21, 2024
Merged

Conversation

morki
Copy link
Contributor

@morki morki commented Jul 27, 2024

See #36

Copy link
Owner

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for long delay!

Looks good!
I've started tests, but I do see, that klib dump is for some reason missing, have you run apiDump?
Also, it would be nice to add additional testing of custom IV in compatibility test like it was added here: fcd13f5#diff-95b13bb1e657d46d67c27f687519118156c333f974d16a88d646cc6f18d9a6e7

@whyoleg
Copy link
Owner

whyoleg commented Aug 2, 2024

Hey @morki, are you still willing to finish PR? Or I could finish it? The reason I'm asking, is that I do have some changes to be merged which will have conflicts.
If I will merge them, this PR is most likely will need to be redone from scratch (by copy-pasting changes)

@morki
Copy link
Contributor Author

morki commented Aug 2, 2024

Hi @whyoleg, I was planning to create tests etc, but I am on vacations now, so I can not finnish it for about 3 weeks now. If you want and have time, you can finish this before the changes. If not, that is ok too, I can start from stratch after your changes :)

@morki
Copy link
Contributor Author

morki commented Aug 22, 2024

Hi @whyoleg, is it somewhat "stable" now to start to do this PR again from scratch or would you suggest to delay it a bit more due to heavy changes still in progress?

@whyoleg
Copy link
Owner

whyoleg commented Aug 22, 2024

Hey!
You can proceed with the changes. May be even not from scratch 😀

I've decided to postpone big changes for some future release as I was not really happy with the API shape.

@whyoleg
Copy link
Owner

whyoleg commented Sep 10, 2024

Hey @morki are you still willing to work on this PR? I'm planning to do a release by the end of September and wanted to include this.

@morki
Copy link
Contributor Author

morki commented Sep 10, 2024

Hi @whyoleg, yes I am, will try to finnish it in a couple of days :)

@morki morki marked this pull request as draft September 10, 2024 11:26
@morki
Copy link
Contributor Author

morki commented Sep 10, 2024

I think it is ready for what I can do for it. But it has one problem with tests.

WebCrypto and OpenSSL providers are ok, but JDK are throwing Cannot reuse iv for GCM encryption, because of built-in "security" and how are tests working now.

I tried but I really don't know how to solve it. Can you please take a look @whyoleg?

@morki morki requested a review from whyoleg September 10, 2024 20:42
@morki morki marked this pull request as ready for review September 10, 2024 20:43
Copy link
Owner

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

I'm very sorry again for the long delay, I was not very active last week with my OpenSource projects :(

The changes look fine, thank you! A bit small fixes and we are ready to merge!
I was able to find workaround for JDK GCM IV reuse issue!
Also, could you please run apiDump task locally? Without it the CI will fail

@morki morki mentioned this pull request Sep 19, 2024
@morki
Copy link
Contributor Author

morki commented Sep 19, 2024

Hey @whyoleg, i fixed those little things and applied your patch to tests. The Cipher interface hierarchy is something over my power, I tried in #45 but don't think it is the right way.

@morki
Copy link
Contributor Author

morki commented Sep 19, 2024

Ou, and the apiDump. I tried it even before, but locally for me it is failing with:

* What went wrong:
Execution failed for task ':cryptography-provider-apple:watchosArm64ApiInfer'.
> There was a failure while executing work items
   > A failure occurred while executing kotlinx.validation.KlibInferAbiWorker
      > The target watchosArm64 is not supported by the host compiler and there are no targets similar to watchosArm64 to infer a dump from it.

@whyoleg
Copy link
Owner

whyoleg commented Sep 20, 2024

Hm, I see. It needs macOS :)
For this PR probably it should be enough to run :cryptography-core:apiDump as only this module has changed it ABI. Could you try it?
If not, I will merge the commits manually after running apiDump on my side.
Otherwise, looks good!

@morki
Copy link
Contributor Author

morki commented Sep 20, 2024

I run it and it works, thank you for the guidance :)

Copy link
Owner

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

Once more, thanks for contribution!

CI is green!

@whyoleg whyoleg merged commit 10c71fc into whyoleg:main Sep 21, 2024
31 checks passed
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.

2 participants