-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Index Level Encryption plugin #12902
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com>
Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com>
Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com>
Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com>
Hey @bruno-roustant , I was reading https://github.com/apache/solr-sandbox/blob/main/ENCRYPTION.md. I have following questions if you could please help answer.
I believe there no support for OS level anywhere in the solr-sandbox?
What does "commit" metadata mean? How is this metadata managed when keys are added, deleted and rotated. Sorry, i haven't read the code pointers you have pointed, but would be helpful if you can give a high level overview.
@asonje Do you have any perf benchmarks with your implementation? Also, have you thought how would key rotation work, we would need to reindex? |
@kumargu i dont have any performance data to share; i have so far focused on functionality and correctness. Also in this implementation, you would have re-index in order to rotate the data keys. |
Ok, makes sense. [1] I also think we should come to performance sooner (when you are convinced with correctness). I am hoping we'll have very minimal penalty; let me know what your assumptions are. [2] We should list out the scope of this PR -- listing features / enhancements that we will pick post this PR e,g IV chunking for shards > 64 GB, snapshot support, seem-less key rotation. I can help with some of those PRs. The scope will help us to get a clear idea on what we are delivering and we are not taking one-way-door decisions. [3] Question: When a key is disabled, what do we do with the CyprtoDir for that key, its remains dangling? Edit: @asonje I know you have put a lot of work on this PR; but I also want us to be open to the solar implementation details that was brought up earlier. thanks! |
There will be a noticeable impact to performance of the crypto directory relative to the HybridDirectory. There is no crypto equivalent yet for MMapDirectory. That would be a good addition for performance as certain index file types are read via mmap for performance.
Yes, if a key is disabled or access is revoked we do not do anything explicitly to the crypto directory |
Having the numbers handy would be really helpful :)
Something to add in the todo list :) |
Right, OS level encryption is managed differently.
It means the custom 'user' metadata optionally stored in each Lucene segment when there is a commit.
Yes it is, but they manage an encryption key for the whole machine. It is not per tenant, it is per machine/volume.
Creating or not creating additional files does not impact perf. What impacts perf is the number of AES blocks to decrypt. |
@bruno-roustant thanks, again for your detail explanation.
Customer managed Keys (CMK) is seeing high adoption in cloud (I am sure of AWS). A CMK support will allow building a multi-tentant env with lot of control to enterprise customers to manage access controls. The initial motivation of this PR calls it out very clearly. For multi tenant support, I think [1] key management and [2] performance are equally important. This is what I have been missing in the journey for this PR so far. I agree with you that we will not be able to use OS level encryption (like [1] A possible usage of |
Actually I used the generic term "OS-level encryption" to include both the block device level (dm-crypt) and file system level (fscrypt).
If we look at the definition of encryption at rest, fscrypt correctly meets the expectation, since it requires a key to unlock a directory. But when running on a cloud provider host, we don't know exactly when is the rest, and we may not accept read rights from admin users while the directory is unlocked (so not at rest). So, yes, I considered the fscrypt option, and yes multi-tenancy is a strong requirement for the encryption module in solr-sandbox. At the end, to be able to work without compromise on cloud provider hosts, and guarantee that no admin rights can read, we chose the encryption at Java level even if it impacts performance. This is the encryption module in solr-sandbox. |
sorry @bruno-roustant, i am often late to replies -- this week has been busy. As next steps, I am going to research how "admin" right are managed on multi tenant cloud -- for example Amazon Opensearch vs Amazon Opensearch serverless have different multi tenant architectures, i.e isolation of user and admin rights are managed differently. |
Hi @asonje, were you able to gather any perf numbers? this would be important to understand the trade-offs. |
With the crypto directory, performance (elapsed time) drops by about 20% when compared with the ‘niofs’ directory and about 65% relative to the ‘hybridfs’ directory. I mentioned already that there isn’t yet a crypto equivalent to the mmap directory so the comparison with the niofs directory is the more direct measure of the cost of encryption/decryption. These measurements were collected on a single node using the stack overflow workload in opensearch-benchmark |
Thank you @asonje for the perf numbers. This is a very useful piece of information for the direction of this PR! it seems like |
Also, if you have the numbers still handy, please could you post here.
|
@kumargu The results shown are averaged over 5 runs. stackoverflow is an indexing workload (no search) |
can we please run |
@asonje requesting for search & indexing benchmarks. |
Here are the results:
I also identified 2 potential optimizations; the first is due to a workaround to avoid using a forbidden API Filechannel.read. The synchronized bock could be replaced with
The second optimization would be to cache the |
Thank-you @asonje ! The numbers are exciting for range queries. Great work identifying the contention at the synchronized block. Looking forward for the [1] Since [2] Throughput (ops/s) = 25 is too low for benchmarking. I was expecting it to be in 1000s. @cwperks could you suggest what we have been using for benchmarks. |
{"run-benchmark-test": "id_1"} |
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/1540/ . Final results will be published once the job is completed. |
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/1540/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/22/
|
@asonje, fyi, I ran a standard benchmark via commenting. Maybe you could remove the synchronize block and rerun the standard benchmark? |
Signed-off-by: Olasoji Denloye <olasoji.denloye@intel.com>
{"run-benchmark-test": "id_2"} |
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/1618/ . Final results will be published once the job is completed. |
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/1618/
|
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/23/
|
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 #3469
Check List
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.