Skip to content

Add pluggable page-level AEAD encryption support - #866

Open
HTHou wants to merge 21 commits into
developfrom
codex/tsfile-tde-page-aead
Open

HTHou wants to merge 21 commits into
developfrom
codex/tsfile-tde-page-aead

Conversation

@HTHou

@HTHou HTHou commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add an optional file encryption header carrying provider, profile, key, wrapped data key, and file identity metadata
  • introduce a provider-neutral AEAD SPI and registry without embedding a concrete cryptographic implementation
  • encrypt compressed page bodies for non-aligned, aligned, and table-model write paths, and decrypt them on the corresponding read paths
  • preserve existing unencrypted TsFile behavior and propagate encryption metadata through append, recovery, lazy loading, and sketch tooling
  • add compatibility, round-trip, tamper-detection, append, and recovery tests

Motivation

TsFile currently has legacy encryption interfaces but no self-describing file-level context for pluggable page-level authenticated encryption. This change establishes the format and I/O integration points while leaving algorithm and key-management implementations to external providers.

Compatibility

  • Existing unencrypted TsFiles keep their current layout and remain readable.
  • Encrypted files require a registered provider matching the identifiers stored in the encryption header.
  • Page metadata remains readable while compressed page bodies are protected with AEAD.
  • The encrypted-file format and public API are still under discussion and are not intended as a compatibility commitment in this draft.

Validation

  • ./mvnw spotless:check -P with-java -pl java/tsfile -am
  • ./mvnw test -P with-java -pl java/tsfile -am -Dtest='PageCryptoContextTest,FileEncryptionHeaderTest,TDEPageAeadTsFileTest,UnClosedTsFileReaderTest,ForceAppendTsFileWriterTest,RestorableTsFileIOWriterTest,TimePageWriterTest' -Dsurefire.failIfNoSpecifiedTests=false
  • 30 targeted tests passed with no failures or errors

Draft discussion points

  • finalize the encryption-header schema and format-version semantics
  • bind page authentication data to an unambiguous chunk/page identity
  • refine page-index ownership and the separation between legacy encryption parameters and file AEAD context
  • confirm append/recovery behavior and backward-compatibility expectations before stabilizing the format

@codecov-commenter

codecov-commenter commented Jul 15, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.52551% with 274 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.93%. Comparing base (d22008b) to head (9ca3576).

Files with missing lines Patch % Lines
...pache/tsfile/file/header/FileEncryptionHeader.java 78.78% 21 Missing ⚠️
...e/read/reader/block/SingleDeviceTsBlockReader.java 74.07% 21 Missing ⚠️
...org/apache/tsfile/write/writer/TsFileIOWriter.java 81.13% 20 Missing ⚠️
...sfile/write/v4/AbstractTableModelTsFileWriter.java 40.00% 18 Missing ⚠️
...che/tsfile/encrypt/EncryptionProviderRegistry.java 67.30% 17 Missing ⚠️
...a/org/apache/tsfile/read/TsFileSequenceReader.java 86.72% 15 Missing ⚠️
...a/org/apache/tsfile/encrypt/PageCryptoContext.java 77.77% 14 Missing ⚠️
...g/apache/tsfile/read/reader/chunk/ChunkReader.java 68.88% 14 Missing ⚠️
...ava/org/apache/tsfile/file/header/ChunkHeader.java 89.16% 13 Missing ⚠️
.../read/query/timegenerator/TsFileTimeGenerator.java 54.16% 11 Missing ⚠️
... and 26 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #866      +/-   ##
===========================================
+ Coverage    65.26%   65.93%   +0.67%     
===========================================
  Files          761      767       +6     
  Lines        53610    54856    +1246     
  Branches      8547     8718     +171     
===========================================
+ Hits         34986    36167    +1181     
- Misses       16978    17043      +65     
  Partials      1646     1646              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JackieTien97 and others added 5 commits August 6, 2026 16:37
…x/tsfile-tde-page-aead

# Conflicts:
#	java/tsfile/src/main/java/org/apache/tsfile/write/v4/AbstractTableModelTsFileWriter.java
#	java/tsfile/src/main/java/org/apache/tsfile/write/writer/TsFileIOWriter.java
@HTHou
HTHou marked this pull request as ready for review October 9, 2026 09:35
@HTHou
HTHou requested a balanced review from Copilot October 9, 2026 09:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

4 open findings
What changed in this PR

This PR introduces pluggable page-level AEAD encryption for TsFile by adding a self-describing file encryption header, a provider-neutral encryption SPI/registry, and integrating page body encrypt/decrypt across write and read paths while preserving unencrypted TsFile compatibility.

Changes:

  • Added FileEncryptionHeader (written after the TsFile version byte) and propagated encryption context through writers/readers, append, and recovery flows.
  • Introduced provider-neutral AEAD SPI (IEncryptProvider, EncryptionProviderRegistry) and page-associated-data binding (PageCryptoContext) with per-chunk ordinals and per-page indices.
  • Added/updated tests to validate round-trips, tamper detection, ordinal continuity across append/recovery, and reader behavior.
File Description
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​write/​writer/​TimePageWriterTest.java Adds an AEAD encrypt/decrypt page round-trip test using the test provider.
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​write/​writer/​RestorableTsFileIOWriterTest.java Adds recovery/ownership tests for encrypted headers and chunk ordinal continuation.
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​write/​writer/​ForceAppendTsFileWriterTest.java Adds force-append coverage for encrypted files and chunk ordinal uniqueness.
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​write/​TDEPageAeadTsFileTest.java New integration tests for encrypted read/write paths, tamper detection, and chunk reuse rejection.
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​read/​UnClosedTsFileReaderTest.java Adds reading encrypted pages from file header in unclosed reader scenario.
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​file/​header/​FileEncryptionHeaderTest.java New round-trip and validation tests for the file encryption header format.
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​encrypt/​TestAeadEncryptionProvider.java Adds a JDK-only AES-GCM test provider implementing the new SPI.
java/​tsfile/​src/​test/​java/​org/​apache/​tsfile/​encrypt/​PageCryptoContextTest.java Adds tests for AAD binding/tamper detection and ordinal allocation behavior.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​writer/​TsFileIOWriter.java Writes encryption header before data, threads encryption context through chunk headers, and blocks encrypted chunk copy.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​writer/​RestorableTsFileIOWriter.java Loads encryption header on recovery, resumes ordinal allocation, and closes owned AEAD parameters.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​writer/​ForceAppendTsFileWriter.java Loads encryption header for append, resumes ordinal allocation, and closes owned AEAD parameters.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​v4/​AbstractTableModelTsFileWriter.java Propagates file-level AEAD context and ensures owned AEAD parameters are closed.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​page/​ValuePageWriter.java Uses centralized PageBodyEncoder and adds pageIndex/chunkOrdinal support.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​page/​TimePageWriter.java Uses centralized PageBodyEncoder and adds pageIndex/chunkOrdinal support.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​page/​PageWriter.java Uses centralized PageBodyEncoder and adds pageIndex/chunkOrdinal support.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​page/​PageBodyEncoder.java New helper to compress + encrypt page bodies (including AEAD AAD binding).
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​page/​EncodedPageBody.java New lightweight holder for encoded page body bytes (data/offset/size).
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​chunk/​ValueChunkWriter.java Allocates per-chunk ordinals and passes pageIndex/chunkOrdinal to page writers + chunk headers.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​chunk/​TimeChunkWriter.java Allocates per-chunk ordinals and passes pageIndex/chunkOrdinal to page writers + chunk headers.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​chunk/​ChunkWriterImpl.java Allocates per-chunk ordinals and passes pageIndex/chunkOrdinal to page writers + chunk headers.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​TsFileWriter.java Propagates file-level AEAD context and closes owned AEAD parameters on close.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​utils/​TsFileSketchTool.java Uses reader.getDataStartOffset() to handle optional encryption header.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​read/​reader/​page/​LazyLoadPageData.java Adds AEAD decrypt path using pageIndex/chunkOrdinal and validates plaintext sizes.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​read/​reader/​chunk/​ChunkReader.java Threads pageIndex/chunkOrdinal into page readers and adds AEAD page decrypt/uncompress overloads.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​read/​reader/​chunk/​AbstractAlignedChunkReader.java Threads pageIndex/chunkOrdinal through aligned chunk page reads.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​read/​common/​Chunk.java Rejects merging AEAD-encrypted chunks and ensures ordinal allocation during rewrite.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​read/​UnClosedTsFileReader.java Uses file-level encrypt param directly for unclosed reader.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​read/​TsFileSequenceReader.java Parses optional encryption header, tracks data start offset, records chunk ordinals, and supports AEAD page decryption with AAD.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​file/​header/​FileEncryptionHeader.java New persisted encryption header format (versioned, size-bounded, wrapped key only).
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​file/​header/​ChunkHeader.java Adds optional chunk ordinal field for AEAD binding and updates (de)serialization accordingly.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​file/​MetaMarker.java Adds ENCRYPTION_HEADER marker for file-level encryption header.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​PageCryptoContext.java New canonical AEAD page context with stable associated data construction.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​IEncryptor.java Adds encryptor selection for AEAD params and encryptPage default method.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​IEncryptProvider.java New SPI interface for provider implementations.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​IEncrypt.java Extends AutoCloseable and adds getPageBodyOverhead contract for AEAD profiles.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​IDecryptor.java Adds decryptor selection for AEAD params and decryptPage default method.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​EncryptionProviderRegistry.java New registry (and service loader integration) for stable provider ids.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​EncryptUtils.java Adds AEAD-aware getEncryptParameter and getEncrypt overload.
java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​EncryptParameter.java Extends parameters for AEAD (provider/profile ids, wrapped key, file id, chunk ordinal allocation, destroy/close).
java/​common/​src/​main/​resources/​org/​apache/​tsfile/​i18n/​messages_zh.properties Adds i18n strings for new encryption header/provider/AAD errors.
java/​common/​src/​main/​resources/​org/​apache/​tsfile/​i18n/​messages.properties Adds i18n strings for new encryption header/provider/AAD errors.

🧠 Review effort: Lite


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread java/tsfile/src/main/java/org/apache/tsfile/file/header/ChunkHeader.java Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved context-validation, fail-open SPI, and missing last-reader decryption paths can produce insecure or unreadable files.

4 open findings
4 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Decrypt aligned BLOB/OBJECT pages before last-point decoding

java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​write/​page/​PageBodyEncoder.java:69

Encrypting every page body leaves TsFileLastReader.readAlignedLastPoint incompatible with encrypted aligned BLOB/OBJECT columns: that path slices the stored body and passes it directly to IUnCompressor/ValuePageReader without decryption (TsFileLastReader.java:179-216). It will feed ciphertext to the decoder (or decompressor), so last-point reads fail for these supported types. Route that path through the AEAD-aware page deserialization while tracking the selected page index and chunk ordinal.

🧠 Review effort: Balanced

Comment thread java/tsfile/src/main/java/org/apache/tsfile/encrypt/IDecryptor.java Outdated
Comment thread java/tsfile/src/main/java/org/apache/tsfile/encrypt/IEncryptor.java Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Headerless recovery can create unreadable mixed-format files, and encryption-context validation and lifetime issues remain unresolved.

2 open findings
4 resolved since last review
Previously missed (2)

In code that hasn't changed since last review

Medium severity Normalize provider IDs during lookup

java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​encrypt/​EncryptionProviderRegistry.java:58

Provider IDs are trimmed when registering and unregistering, but lookup uses the untrimmed persisted value. A provider whose declared ID contains surrounding whitespace is therefore registered successfully under the trimmed key yet can never be resolved from a parameter/header carrying that declared ID. Apply the same normalization during lookup (or reject non-canonical IDs consistently).

Medium severity Preserve encryption context for materialized chunks

java/​tsfile/​src/​main/​java/​org/​apache/​tsfile/​read/​TsFileSequenceReader.java:2417

The same EncryptParameter instance is attached to every Chunk returned by readMemChunk() and to its lazy page readers. Destroying it here means an already materialized in-memory chunk becomes unusable as soon as the sequence reader is closed; subsequent page loading fails with parameter_destroyed. Give returned chunks an independently owned context or otherwise coordinate the context lifetime instead of invalidating it with the file handle.

🧠 Review effort: Balanced

@HTHou
HTHou requested a balanced review from Copilot October 12, 2026 04:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@JackieTien97 JackieTien97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for working on this. The overall direction looks good: a self-describing file header, a provider SPI, and per-page AEAD with the AAD bound to fileCryptoId / chunk ordinal / page index. The fail-closed checks on chunk copy/merge, the immutable file context and the ordinal validation are nice, and the lifecycle tests cover a lot of edge paths.

Validation: I ran the 7 test classes listed in the description plus the two new lifecycle test classes (72 tests), and the full java/tsfile suite (1038 tests, 0 failures, 10 skipped), on b58e5c37. The only change since then, fc88fb87, adds a pageWriter null check in reset() and doesn't affect the comments below. I reproduced each issue with a small probe test; the repro steps are in the inline comments.

Summary of the inline comments:

  1. Blocking: page-level raw copy (writePageHeaderAndDataIntoBuff(ByteBuffer, PageHeader)) is not fail-closed for page AEAD and silently writes undecryptable files. This is the IoTDB fast-compaction path.
  2. The AAD binds pages to chunk ordinals, but nothing binds chunks to their series: swapping whole chunks between s1 and s2 goes undetected.
  3. TsFileReader now strongly retains every QueryDataSet, which changes the contract for existing users of unencrypted files.
  4. With the reference-counted Chunk / ChunkReader ownership, cleanup only happens if every caller closes every chunk.
  5. Recovery and unsealed reads: headerless file + AEAD parameter, a torn header surfacing as an unchecked exception, and the getEncryptParam() fallback.
  6. Parameter ownership, SPI hardening (reachability of the legacy encrypt/decrypt, per-file key generation, nonce guidance), and a few nits.

}
}

private static byte[] buildAssociatedData(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The AAD binds a page to (fileCryptoId, chunkOrdinal, pageIndex, sizes), but nothing binds a chunk to its series.

measurementId, dataType, encoding and compressionType (and the device) are not in the associated data. The ordinal lives in the unauthenticated chunk header, so it moves together with the chunk.

Repro: write d1.s1 = 11 and d1.s2 = 22 (PLAIN/UNCOMPRESSED, one point each, so both chunks are 79 bytes). Swap the two chunks' whole byte ranges (header + pages) in the file. Querying d1.s1 returns 22, with no authentication error.

Truncating trailing pages or chunks, or editing the plaintext statistics so pages get skipped, also goes undetected. testSwappingEqualSizedPagesBetweenChunksFailsAuthentication only covers swapping pages between chunks.

Suggestion: put the fields that are fixed when a page is encrypted (dataType, encoding, compressionType, time/value column mask) into the AAD. measurementId and the device need more thought, because names can be remapped at flush time (measurementNameRemapper) and chunk writers don't know the device. Either way, please document that page AEAD does not provide chunk-level integrity (deletion, truncation, reordering).

@HTHou HTHou Oct 13, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed the whole-chunk swap repro in 2f2a452 with a separate position proof in encrypted Chunk Headers. Immediately before writing the header, the provider authenticates a domain/version, fileCryptoId, the actual absolute chunk-marker offset, and chunkOrdinal. Sequential and positional readers verify the proof before using the ordinal or page body.

Updated in ecb0b6b: the proof is no longer hardcoded to 28 bytes by TsFile. Each immutable provider profile declares its size through getChunkAuthenticationSize(); the encrypted Chunk Header stores the proof length and payload, and readers validate the length against the profile before reading the payload. The current TimechoDB SM4-GCM profile uses 28 bytes (12-byte IV plus 16-byte tag), but other profiles may declare a different bounded size. Tests cover variable-length serialization, a 32-byte proof through query/both read paths/recovery, and invalid or mismatched lengths.

Page AAD and page-buffer encryption timing remain unchanged. Scope clarification: this protects relocation of an existing chunk, not general metadata integrity. Measurement/device mapping, codec fields, statistics and indexes remain unauthenticated; pruning based on altered statistics, deletion/truncation and index remapping are not claimed to be detected by this design.

Comment thread java/tsfile/src/main/java/org/apache/tsfile/read/TsFileReader.java Outdated
* A materialized chunk owning a page-AEAD reference that survives file reader closure. Close it
* after use, or transfer it to a chunk reader that releases the reference when closed.
*/
public class Chunk implements AutoCloseable {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With reference counting, provider cleanup only happens if every caller closes every Chunk, and existing callers don't.

Every Chunk built with a page-AEAD parameter now retain()s it (copying the plaintext key if the parameter carries one) and must be closed. Existing callers of readMemChunk don't close chunks. For example, IoTDB's ChunkCache caches the Chunk returned by readMemChunk and doesn't close it on eviction. With the cache disabled, it re-wraps the loaded chunk in constructChunk and never closes the original. In practice the count rarely reaches zero, so "the provider is closed after the last reference" doesn't hold. Meanwhile every downstream call site has to learn a new close contract.

Suggestion: decouple the provider context's lifetime from object lifetimes. For example, keep a bounded process-wide cache of IEncrypt keyed by (providerId, profileId, keyId, keyVersion, wrappedDataKey) that closes entries on eviction, and let Chunk hold only the immutable file encryption descriptor. Then Chunk doesn't need to be closeable, and unencrypted paths stay untouched.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We are keeping explicit ownership for the new page-AEAD feature rather than adding a process-wide crypto cache to TsFile. Chunk retains the shared file-level provider context, and ChunkReader owns/releases the supplied chunk; retained chunks do not independently unwrap a key on each read. The plaintext path does not acquire crypto references. The consuming TimechoDB branch has been adapted: ChunkCache closes entries on eviction, closes temporary loaded chunks after creating retained query views, and handles eviction/retain races; query readers release their owned views. This downstream adaptation is required before enabling page AEAD. We do not claim that arbitrary unmodified downstream callers will release encrypted resources automatically.

}

@Override
public void close() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Closing a ChunkReader now destroys the encryption reference of the caller's Chunk.

Before this PR, close() here was a no-op. Now it closes the supplied chunk.

Repro: Chunk c = reader.readMemChunk(meta), read it through new ChunkReader(c), then close(). Afterwards c.getEncryptParam().isDestroyed() is true. Re-wrapping the chunk's data with its parameter, new Chunk(c.getHeader(), data, c.getEncryptParam()) (the ChunkCache#constructChunk pattern), throws Encryption parameters have been destroyed. That worked before this PR and still works for unencrypted chunks. It's a behavior change worth documenting, or avoiding with the approach suggested on Chunk.

Also, ChunkReader and AbstractAlignedChunkReader now own resources but aren't AutoCloseable, so try-with-resources can't be used.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The ownership transfer for page-AEAD chunks is intentional and documented on ChunkReader: closing it releases the supplied chunk's reference. A cache must pass an independently retained Chunk view, not its own cache entry. The TimechoDB integration now follows that rule and closes cache-owned references on eviction. Plaintext chunks do not have this crypto lifetime requirement. In d391241, IChunkReader also extends AutoCloseable, so scalar and aligned readers can be used with try-with-resources.

Comment thread java/tsfile/src/main/java/org/apache/tsfile/read/TsFileSequenceReader.java Outdated
Comment thread java/tsfile/src/main/java/org/apache/tsfile/write/writer/TsFileIOWriter.java Outdated

@JackieTien97 JackieTien97 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please address the two inline findings before merging: page authentication failures can be silently converted into incomplete table-query results, and concurrent first use can fail while service-provider registration is still in progress. Both issues were reproduced locally on b58e5c3; the affected code paths are unchanged in fc88fb8.

Comment thread java/tsfile/src/main/java/org/apache/tsfile/encrypt/EncryptUtils.java Outdated
Comment thread java/tsfile/src/main/java/org/apache/tsfile/encrypt/IDecryptor.java
Comment thread java/tsfile/src/main/java/org/apache/tsfile/read/TsFileSequenceReader.java Outdated
Comment thread java/tsfile/src/main/java/org/apache/tsfile/read/TsFileSequenceReader.java Outdated
Comment thread java/tsfile/src/main/java/org/apache/tsfile/write/page/EncodedPageBody.java Outdated
Comment on lines +56 to +68
/**
* Returns a fixed-size opaque proof authenticating a chunk's physical position. The proof is
* exactly {@link #getChunkAuthenticationSize()} bytes. Providers using AEAD must use a fresh
* nonce under the file DEK, independently of page encryption calls.
*/
default byte[] authenticateChunk(ChunkCryptoContext context) {
throw new EncryptException(Messages.get("error.encrypt.chunk_authentication_unsupported"));
}

/** Verifies the proof before a chunk identity is used; unsupported providers fail closed. */
default void verifyChunkAuthentication(byte[] authentication, ChunkCryptoContext context) {
throw new EncryptException(Messages.get("error.encrypt.chunk_authentication_unsupported"));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

May provide the implementation name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9ca3576. Both default authenticateChunk and verifyChunkAuthentication failures now include getClass().getName() in the localized exception message. Added EncryptionProviderRegistryTest.testUnsupportedChunkAuthenticationNamesImplementation to verify both fail-closed paths identify the implementation. The targeted run passed 99 TsFile tests and 5 common MessagesTest cases.

Comment on lines +499 to +514
if (marker != MetaMarker.ENCRYPTION_HEADER) {
switch ((byte) marker) {
case MetaMarker.CHUNK_GROUP_HEADER:
case MetaMarker.CHUNK_HEADER:
case MetaMarker.SEPARATOR:
case MetaMarker.VERSION:
case MetaMarker.OPERATION_INDEX_RANGE:
case MetaMarker.ONLY_ONE_PAGE_CHUNK_HEADER:
case MetaMarker.TIME_CHUNK_HEADER:
case MetaMarker.VALUE_CHUNK_HEADER:
case MetaMarker.ONLY_ONE_PAGE_TIME_CHUNK_HEADER:
case MetaMarker.ONLY_ONE_PAGE_VALUE_CHUNK_HEADER:
break;
default:
MetaMarker.handleUnexpectedMarker((byte) marker);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

May use enhanced switch to report an error during compilation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated in 9ca3576 to the enhanced switch syntax with grouped known-marker cases and an explicit default rejection. The selector is a byte rather than an enum, so this does not provide enum-style compile-time exhaustiveness checking; unknown markers still fail closed at runtime. The targeted encryption/header/query/recovery and read-utility tests passed.

if (ioSizeRecorder != null) {
ioSizeRecorder.accept(dataStartOffset - BASE_FILE_HEADER_SIZE);
}
} catch (org.apache.tsfile.exception.encrypt.EncryptException e) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

mind the import

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9ca3576: EncryptException is now imported normally and the catch clause no longer uses its fully qualified name. Spotless and the targeted test run 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.

5 participants