Initial push of crypto directory#3
Merged
kumargu merged 21 commits intoopensearch-project:mainfrom Jun 25, 2025
Merged
Conversation
Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com>
asonje
requested review from
kumargu,
shubhmkr-amazon and
udabhas
as code owners
Collaborator
|
thanks @asonje . will start taking a look. |
…ion. Signed-off-by: Shubham <shubhmkr@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
added 4 commits
May 12, 2025 14:21
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
kumargu
force-pushed
the
main
branch
3 times, most recently
from
6e76c92 to
4b98c6c
Compare
Signed-off-by: Gulshan <kumargu@amazon.com>
added 3 commits
May 16, 2025 10:25
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
kumargu
force-pushed
the
main
branch
2 times, most recently
from
7833802 to
a31c9b5
Compare
Collaborator
|
My observations were similar while testing via a opensearch-bechmark. so we are on same page. thanks! |
Collaborator
|
fyi: there is memory leak currently in the panama_v2 branch. I am trying to fix it up. Its is probably because I'm somewhere not closing the arena properly. Hopefully I'll be able to fix it today. |
Contributor
|
When the plugin needs to load a OpenSSL library (.so/.dylib) is it the plan for on-premise OpenSearch to: Advantages/Disadvantages: Maybe we align its behavoiur with other native plugins like knn, ml etc Wdyt? |
Contributor
And a typo i think: EgarDecryptedCryptoMMapDirectory -> EagerDecryptedCryptoMMapDirectory |
added 2 commits
June 13, 2025 15:05
Signed-off-by: Gulshan <kumargu@amazon.com> So far the best performing batch decryption Signed-off-by: Gulshan <kumargu@amazon.com> Use on-fly decryption for small file extensions Signed-off-by: Gulshan <kumargu@amazon.com> Small and Large file routing Signed-off-by: Gulshan <kumargu@amazon.com> Lazy decryption Signed-off-by: Gulshan <kumargu@amazon.com> Seems to work well Signed-off-by: Gulshan <kumargu@amazon.com> works in 814 seconds Signed-off-by: EC2 Default User <ec2-user@ip-172-31-0-33.ec2.internal> tim files to lazy SUCCESS (took 965 seconds) Signed-off-by: EC2 Default User <ec2-user@ip-172-31-0-33.ec2.internal> Per file based routing Signed-off-by: EC2 Default User <ec2-user@ip-172-31-0-33.ec2.internal> Best performer so far median: 315451 finishes in 910 Signed-off-by: EC2 Default User <ec2-user@ip-172-31-0-33.ec2.internal> Excellent performer Signed-off-by: EC2 Default User <ec2-user@ip-172-31-0-33.ec2.internal> Finishes a bit faster than batch size of 2L-4L Signed-off-by: EC2 Default User <ec2-user@ip-172-31-0-33.ec2.internal> QOL Signed-off-by: Gulshan <kumargu@amazon.com> Route CFS to lazy Signed-off-by: EC2 Default User <ec2-user@ip-172-31-0-33.ec2.internal> More Qol Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Signed-off-by: Gulshan <kumargu@amazon.com>
Collaborator
|
(benchmarking results on https_logs) Baseline i.e no crytpo------------------------------------------------------
Using Index level Enc in this repo
LUKS based (transparent) encryption
|
Collaborator
|
@salyh looks like opensearch-project/OpenSearch#18085 is ready. I will rebase by Opensearch core against it and try out replacing OpenSSL parts with SunJCE. |
3 tasks
Contributor
The issue with the Java standard Cipher architecture is that it can get hard to do a real in-place decryption. Had some problems with passing native/direct ByteBuffers into the doFinal() or update() methods. Maybe you have more luck :-) |
salyh
reviewed
Jun 22, 2025
| } | ||
|
|
||
| @SuppressWarnings("unused") | ||
| private static void decryptAndProtectPageByPage( |
Contributor
There was a problem hiding this comment.
@kumargu decryptAndProtectPageByPage() this is yet unused
Contributor
There was a problem hiding this comment.
Is decryptAndProtect() faster?
Collaborator
There was a problem hiding this comment.
yes decryptAndProtect is slightly faster because it can decrypt contiguous range of pages in one-shot. I still need to figure out a proper error handling for it.
But decryptAndProtectPageByPage is much safer as unsetting the bits is much easier on an exception.
salyh
reviewed
Jun 23, 2025
| MemorySegment dst = arena.allocate(length); | ||
| MemorySegment outLen = arena.allocate(ValueLayout.JAVA_INT); | ||
|
|
||
| rc = (int) EVP_EncryptUpdate.invoke(ctx, dst, outLen, src, (int) length); |
Contributor
There was a problem hiding this comment.
@kumargu (int) length may lead to overflow. There are a lot of potentially unsafe casts to int in this file and in others. I suggest whenever a cast is inevitable to use a method which checks for boundaries
There was a problem hiding this comment.
It is also adviseable to use invokeExact() for performance, because we know the exact signature and no adaption is required.
There was a problem hiding this comment.
I think the casting to int is fine here, because we know that the decoding buffer is always smaller, it is just nowere enforced. IMHO the length parameter of the decryptInto method should be int from the beginning - and if not it should be checked around line 236 (0<=x<=Integer.MAX_VALUE).
Of course for such accesses the check should not be done on every access in tight loops. Therefore in the original MMapDirectory we have many try-catch blocks catching IllegalArgument, ArrayIndexOutOfBounds and NullPointerException because we do not check on every access and for the rare case that someone changes to another segment while reading we do the logic in the catch block (also to check if Input was already closed).
Collaborator
There was a problem hiding this comment.
IMHO the length parameter of the decryptInto method should be int from the beginning - and if not it should be checked around line 236 (0<=x<=Integer.MAX_VALUE)
yeah agreed. this is a strong enforcement.
Contributor
@kumargu Watch out for this netty/netty#15324 (comment) openjdk/jdk#25324 when dealing with ByteBuffers derived from MemorySegments in doFinal()/update() in Java <25 |
Collaborator
we are already doing it here. Am i missing something? |
Contributor
Depends on how the MemorySegment is created/scoped. The issue arise when its a shared one. Global and confined seems working. |
Collaborator
|
Ok. I will check with shared arena again, currently we are using confined which works. Shared arena hasn't be super performant (i need to figure out why), but AFAIR, i had worked for me. |
Contributor
and i just noticed you copy the bytes first to byte[] instead of using the bytebuffer directly in update(bytebuffer, bytebuffer) |
Collaborator
|
This PR will be still open for comments but I think it's time to merge this so that others can start contributing on top of it and address comments.Next week a member from my team will add CI flows and figure out how we can run Opensearch core tests with enc directory. I will create issues from comments which are already not addressed in this PR and we will work upon them in parralel. But it's time to get this merged because of its growing size of changes. I have few more ideas on MMAP directory, so I am gonna restrain from unit tests for an another week and see how it shapes eventually. |
kumargu
approved these changes
Jun 25, 2025
Bukhtawar
reviewed
Jun 26, 2025
| * - READONCE is just DEFAULT with SEQUENTIAL | ||
| */ | ||
|
|
||
| public static int getMAdviseFlags(IOContext context, String fileName) { |
There was a problem hiding this comment.
Are we extending Lucene's IOContext here?
Collaborator
There was a problem hiding this comment.
yes. but we will also talk to Lucene on the best way to consume Lucene IO context, given MADVISE_RANDOM had some problems recenlty.
There was a problem hiding this comment.
I think the madvise has no real value here, because you consume using a copy-on-write private segment. I'd remove that completely.
If you move to your own, hand written buffer cache as suggested in other issue, then you can implement readahead and madvise on your own.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request adds index level encryption features to OpenSearch based on the issue #3469. Each OpenSearch index is individually encrypted based on user provided encryption keys. A new cryptofs store type index.store.type is introduced which instantiates a CryptoDirectory that encrypts and decrypts files as they are written and read respectively
Related Issues
Resolves opensearch-project/OpenSearch#3469
Related to opensearch-project/OpenSearch#12902
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.