Add translog encryption #39
Merged
Conversation
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
Signed-off-by: Rajat Gupta <[email protected]>
RajatGupta02
requested review from
asonje,
kumargu,
shubhmkr-amazon and
udabhas
as code owners
Contributor
Author
|
OSB: |
Collaborator
|
I will take a look at the PR but do you know why the throughout dropped and latency increased? |
Collaborator
|
is it solely coming from the Translog operation? can we isolate it by disabling and enabling it? |
Contributor
Author
Not sure but on my cluster in |
3 tasks
udabhas
reviewed
Aug 5, 2025
| private static ChannelFactory createCryptoChannelFactory(KeyIvResolver keyIvResolver, String translogUUID) throws IOException { | ||
| try { | ||
| CryptoChannelFactory channelFactory = new CryptoChannelFactory(keyIvResolver, translogUUID); | ||
| LogManager.getLogger(CryptoTranslog.class).debug("CryptoChannelFactory initialized for translog: {}", translogUUID); |
Collaborator
There was a problem hiding this comment.
we can use logger defined above?
udabhas
reviewed
Aug 5, 2025
Comment on lines
112
to
114
| public KeyIvResolver getKeyIvResolver() { | ||
| return keyIvResolver; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
same here, forgot to clean this.
udabhas
reviewed
Aug 5, 2025
Comment on lines
558
to
560
| public FileChannel getDelegate() { | ||
| return delegate; | ||
| } |
Collaborator
There was a problem hiding this comment.
who is calling this method?
Contributor
Author
There was a problem hiding this comment.
forgot to clean this.
udabhas
reviewed
Aug 5, 2025
| ChunkInfo chunkInfo = getChunkInfo(position); | ||
|
|
||
| // Read-modify-write chunk pattern | ||
| byte[] existingChunk = readAndDecryptChunk(chunkInfo.chunkIndex); |
Collaborator
There was a problem hiding this comment.
we are decrypting entire file everytime and re-encrypting it along with new incoming diff. This may increase overall time. do we know rough idea, how frequently we write to this?
Contributor
Author
There was a problem hiding this comment.
I did not understand, we are encrypting and decrypting in 8kb chunks, not the whole file.
Collaborator
There was a problem hiding this comment.
ohh okay, we are decrypting only last chunk
Signed-off-by: Rajat Gupta <[email protected]>
3 tasks
kumargu
reviewed
Aug 6, 2025
|
|
||
| // Create new engine config by copying all fields from existing config | ||
| // but replace the translog factory with our crypto version | ||
| EngineConfig cryptoConfig = new EngineConfig.Builder() |
Collaborator
There was a problem hiding this comment.
can we just not override (the builder should allow) the cryptoTranslogFactory to an existing config rather than copying over all these configs?
Contributor
Author
There was a problem hiding this comment.
That was the issue, EngineConfig.java doesn't have any copy constructor :/
src/main/java/org/opensearch/index/store/CryptoEngineFactory.java
Outdated
Show resolved
Hide resolved
| * | ||
| * @opensearch.internal | ||
| */ | ||
| public class CryptoFileChannelWrapper extends FileChannel { |
Collaborator
There was a problem hiding this comment.
sounds very very weird to extend the FileChannel. Did we do the same in actual Translog Impl?
Contributor
Author
There was a problem hiding this comment.
In the Translog impl, it was just returning FileChannel::open so it did not need to wrap the file channel
src/main/java/org/opensearch/index/translog/CryptoFileChannelWrapper.java
Outdated
Show resolved
Hide resolved
| * @throws IOException if there is an error setting up the channel | ||
| */ | ||
| public CryptoFileChannelWrapper( | ||
| FileChannel delegate, |
Collaborator
There was a problem hiding this comment.
see this is the weird part, we extend a FileChannel and then use it a constructor args.
Contributor
Author
There was a problem hiding this comment.
We are just using it as a wrapper, when we pass this FileChannel as the delegate field arg we are forwarding actual I/O operations to this delegate
Member
There was a problem hiding this comment.
We actually use a similar pattern here: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/filter/DelegatingRestHandler.java#L28-L31
| return 0; | ||
| } | ||
|
|
||
| positionLock.writeLock().lock(); |
Collaborator
There was a problem hiding this comment.
why do we need a writeLock in read?
Contributor
Author
There was a problem hiding this comment.
Line 280:
if (position == this.position.get()) {
this.position.addAndGet(dst.remaining());
}
this was modifying the position, but yes for stateful reads I should use writeLock and for positional reads readLock, will update this.
Signed-off-by: Rajat Gupta <[email protected]>
cwperks
reviewed
Aug 7, 2025
| buildscript { | ||
| ext { | ||
| opensearch_version = System.getProperty("opensearch.version", "3.1.0-SNAPSHOT") | ||
| opensearch_version = System.getProperty("opensearch.version", "3.2.0-SNAPSHOT") |
Member
There was a problem hiding this comment.
Can you sync with the latest changes on the main branch? Not sure why this is showing here.
Contributor
Author
There was a problem hiding this comment.
Actually, we were using 3.1.0 snapshot earlier, but for translog once the core changes are merged we will have to use 3.2.0 here.
cwperks
reviewed
Aug 7, 2025
src/main/java/org/opensearch/index/store/CryptoEngineFactory.java
Outdated
Show resolved
Hide resolved
added 2 commits
August 11, 2025 14:06
Signed-off-by: Rajat Gupta <[email protected]>
Signed-off-by: Rajat Gupta <[email protected]>
kumargu
reviewed
Aug 13, 2025
|
|
||
| // TranslogHeader constants replicated to avoid cross-classloader access | ||
| private static final String TRANSLOG_CODEC = "translog"; | ||
| private static final int VERSION_PRIMARY_TERM = 3; |
Collaborator
There was a problem hiding this comment.
how do we derive this value?
Contributor
Author
There was a problem hiding this comment.
Not sure, but it was hardcoded here in the TranslogHeader impl: https://github.com/opensearch-project/OpenSearch/blob/8f310f5e37c65f9c3116c05685a6713eb5e68886/server/src/main/java/org/opensearch/index/translog/TranslogHeader.java#L67
Collaborator
There was a problem hiding this comment.
can we use (i.e import) the constants as it is in the core? a change in there will also reflect at our side, otherwise we will corrupt the data.
Signed-off-by: Rajat Gupta <[email protected]>
kumargu
reviewed
Aug 13, 2025
Comment on lines
+49
to
+50
| EngineConfig cryptoConfig = config | ||
| .toBuilder() |
3 tasks
kumargu
reviewed
Aug 13, 2025
| } | ||
|
|
||
| @Override | ||
| public MappedByteBuffer map(MapMode mode, long position, long size) throws IOException { |
Contributor
Author
There was a problem hiding this comment.
We added it here just so that in future if map is used it clearly throws an exception.
kumargu
approved these changes
Aug 13, 2025
kumargu
approved these changes
Aug 28, 2025
Contributor
Author
|
On c5.12xLarge instance |
Collaborator
|
Perfect. we should consider this is our baseline. |
3 tasks
udabhas
pushed a commit
to udabhas/opensearch-storage-encryption
that referenced
this pull request
Oct 1, 2025
This reverts commit 352ac8a.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 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
Adds transaction log (translog) encryption using AES-GCM with 8KB chunks.
Related Issues
Resolves
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.