HBASE-29822 Add API surface and refactoring for key management feature (HBASE-29368) #7584
Conversation
…ement feature This commit prepares the codebase for the upcoming key management feature (HBASE-29368) by introducing the necessary API definitions, protocol buffer changes, and infrastructure refactoring. No functional changes are included; all implementation will follow in the feature PR. This precursor PR essentially extracts the API surface definitions and infrastructure refactoring from the main feature PR (apache#7421) to facilitate easier review. By separating the ~15k line feature PR into a smaller precursor containing interface definitions, protocol changes, and method signature updates, the subsequent feature PR will focus purely on implementation logic. API Surface Additions: * New interfaces: - KeymetaAdmin: Admin API for key management operations - Server methods for cache management (getManagedKeyDataCache, getSystemKeyCache) * Protocol buffer definitions: - ManagedKeys.proto: Definitions for managed key data and operations - Admin.proto: RPC methods for key management admin operations - Procedure.proto: Key rotation procedure support Infrastructure Refactoring: * Encryption context creation: - Moved createEncryptionContext from EncryptionUtil (client) to SecurityUtil (server) where it properly belongs, as it requires server-side resources - Added overloads to support future key encryption key (KEK) parameters * Method signature updates: - Added ManagedKeyDataCache and SystemKeyCache parameters to encryption-related methods throughout HRegion, HStore, HStoreFile, and HFile classes - Updated constructors and factory methods to thread cache references - All cache parameters are currently null/unused, enabling gradual feature rollout * New utility methods: - Encryption.encryptWithGivenKey() / decryptWithGivenKey(): Extract method refactoring to support both subject-based and KEK-based encryption - EncryptionUtil.wrapKey() / unwrapKey() overloads with KEK parameter - Bytes.add() 4-argument overload for concatenation Stub Infrastructure: * Blank place holder shells for some public data classes such as ManagedKeyData and KeymetaAdminClient * Stub implementations for key management services and caches that return null or throw UnsupportedOperationException, clearly documented as placeholders * New package org.apache.hadoop.hbase.keymeta for key management classes * Mock services updated to support new cache getter methods for testing Code Organization: * Procedure framework: Added support for region-level server name tracking to support future key rotation procedures * Testing infrastructure updated to support new constructor signatures All stub implementations clearly document they are placeholders for the upcoming feature PR. Existing encryption functionality remains unchanged and continues to work as before. Testing: * All existing tests pass (precursor introduces no functional changes) * Build completes successfully with new API surface * Backward compatibility maintained for non-key-management code paths
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
There are 4 test failures in (TestGzipFilter,TestNamespacesInstanceResource,TestScannersWithFilters,TestSchemaResource) but I can't repro them locally so they appear to have flapped. The 4 checkstyle errors can be ignored, they will go away in my next PR. There are apparently 28 errors from spotless:check, but when I ran spotless:apply, only the 2 unused imports were removed nothing else, so CI seems to be overreporting it (I have observed this previously too). |
I also think sometimes it does overreport. |
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Still reviewing, so far looks good overall, left few comments
| * @param d fourth fourth | ||
| * @return New array made from a, b, c, and d | ||
| */ | ||
| public static byte[] add(final byte[] a, final byte[] b, final byte[] c, final byte[] d) { |
There was a problem hiding this comment.
Instead of adding more byte[], how about we let core logic to keep using existing utilities? e.g. if we have to concatenate 4 byte[] (a, b, c, d), we can first concatenate c + d, and then concatenate (a + b + result of c+d)?
There was a problem hiding this comment.
Your suggestion is possible, in fact from this angle the current 3 param variant can also be done using the 2 param variant, but I would assume that a dedicated method was created to make it more efficient and produces less garbage, so I extended the same philosophy to the new 4 param variant. IMHO, the duplication is worth having from the point of squeezing out additional performance.
There was a problem hiding this comment.
No worries, for now we can add note in the javadoc that if any use case needs more than 4 bytes, divide and use any of the existing utilities. So that in future, no one keeps adding more variants.
There was a problem hiding this comment.
You mean in the javadoc of all the current variants?
There was a problem hiding this comment.
I missed to see another variant that takes an array of byte[] (currently unused) that should suffice, perhaps we an take a vararg there so I can revert this change.
| * STUB IMPLEMENTATION - Feature not yet complete. This class represents encryption key data. Full | ||
| * implementation will be in HBASE-29368 feature PR. | ||
| */ | ||
| @InterfaceAudience.Public |
There was a problem hiding this comment.
Are we expecting downstreamers to directly use ManagerKeyData class? If no, we should keep IA.Private. If yes, we should still add IS.Unstable with IA.Public for now.
These are not final consumable APIs anyway so we can't only keep IA.Public.
There was a problem hiding this comment.
The ManagedKeyData is exposed to the hbase clients so I think it qualifies as a public interface.
There was a problem hiding this comment.
I will add IS.Unstable per your suggestion.
There was a problem hiding this comment.
Isn't IS.Evolving a better fit?
| * KeymetaAdmin is an interface for administrative functions related to managed keys. It handles the | ||
| * following methods: | ||
| */ | ||
| @InterfaceAudience.Public |
There was a problem hiding this comment.
Same here, applies to all new interfaces and apis
There was a problem hiding this comment.
This is used outside the hbase-common module so I marked it as public, is that not a good enough reason to make it public?
There was a problem hiding this comment.
Actually, this too is exposed to the client applications so seems appropriate. I will double check any other usages.
There was a problem hiding this comment.
Phoenix will directly use the class? If so, yes we can expose it but still we should mark it IS.Unstable because it is in dev phase and not stabilitized. We can also change signatures in future and after a couple years and few big releases, we can mark it stable.
| rpc RefreshSystemKeyCache(EmptyMsg) | ||
| returns(EmptyMsg); | ||
|
|
||
| rpc EjectManagedKeyDataCacheEntry(ManagedKeyEntryRequest) | ||
| returns(BooleanMsg); | ||
|
|
||
| rpc ClearManagedKeyDataCache(EmptyMsg) | ||
| returns(EmptyMsg); |
There was a problem hiding this comment.
Are we expecting more apis in future?
We might want to consider having ManagedKeyAdmin interface?
There was a problem hiding this comment.
I don't see a need to add more APIs. Adding these here made it simpler to take advantage of the existing async framework and the primitives for parallel execution (across all the RS) for the admin API. The higher level KeymetaAdmin RPC interface is blocking and makes use of these primitives.
| } catch (Throwable e) { | ||
| // Try the old signature for the sake of test code. | ||
| return createInstance(conf, ctorArgTypes.subList(0, ctorArgTypes.size() - 1), | ||
| ctorArgs.subList(0, ctorArgs.size() - 1)); | ||
| } |
There was a problem hiding this comment.
Is this fallback necessary? We can catching any type of Throwable here. Or is it going to be a lot of changes in tests to use new constructor?
There was a problem hiding this comment.
Yes, if you look for the usage of HConstants.REGION_IMPL, you would see many test classes each with their own test/mock class so I wanted to reduce the change. However, I think I can be more specific on the exception types.
There was a problem hiding this comment.
Actually, the existing createInstance is already doing a catch all and convert it to IllegalStateException, so there is technically no difference between catching Throwable or more specific IllegalStateException in this catch block. I will still change it to be clear about the intention and avoid any potential confusion.
There was a problem hiding this comment.
Sure, IllegalStateException will be better
| public static Region openHRegion(final Region other, final CancelableProgressable reporter) | ||
| throws IOException { | ||
| return openHRegion((HRegion) other, reporter); | ||
| } |
There was a problem hiding this comment.
While HRegion is IA.Private, downstreamers with coproc still tend to use some utilities. It would be better to keep this. It is more generic Region instead of HRegion anyway.
There was a problem hiding this comment.
OK, I can restore this method.
| decryptWithGivenKey(key, out, in, outLen, alterCipher, iv); | ||
| } else { | ||
| throw new IOException(e); | ||
| throw e; |
There was a problem hiding this comment.
don't we want to retain throw new IOException(e)? unless this is easier to catch later
There was a problem hiding this comment.
Actually, e is already of type IOException so I didn't think wrapping it again with another IOException is adding any value. Do you see any issue here?
There was a problem hiding this comment.
I also thought that it should not be a problem, i was just curious if it breaks any exception catching and validation of the error message based on the current logic. If everything is fine, we are good.
| @InterfaceStability.Evolving | ||
| public class SecurityUtil { | ||
| public final class SecurityUtil { | ||
| private static final Logger LOG = LoggerFactory.getLogger(SecurityUtil.class); |
There was a problem hiding this comment.
nit: do we need this later?
There was a problem hiding this comment.
Yes, it is being used in the feature PR.
haridsv
force-pushed
the
HBASE-29368-precursor
branch
from
3ab4b81 to
fbacd7d
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
changed the title
This PR implements the key management feature for HBase encryption at rest, building on the API surface and refactoring introduced in the precursor PR (apache#7584). Jira: [HBASE-29368](https://issues.apache.org/jira/browse/HBASE-29368) Design doc: https://docs.google.com/document/d/1ToW_rveXHXUc1F6eFNQfu5LOeMAjzgq6FcYUDbdZrSM/edit?usp=sharing Discussion thread: https://lists.apache.org/thread/q7g2rr2xcgl64rkn9j3mnokf6fvohp2y Cumulative changes from feature branch corresponding to the following sub-tasks: 1. [Phase 1: Key caching and minimal service](https://issues.apache.org/jira/browse/HBASE-29402) 2. [Phase 2: Integrate key management with existing encryption](https://issues.apache.org/jira/browse/HBASE-29495) 3. [Phase 2: Migration path from current encryption to managed encryption](https://issues.apache.org/jira/browse/HBASE-29617) 4. [Phase 2: Admin API to trigger for System Key rotation detection as an alternative to failover.](https://issues.apache.org/jira/browse/HBASE-29643) 5. [Phase 3: Additional key management APIs](https://issues.apache.org/jira/browse/HBASE-29666) This feature introduces a comprehensive key management system that extends HBase's existing encryption-at-rest capabilities. The implementation provides enterprise-grade key lifecycle management with support for key rotation, hierarchical namespace resolution for key lookup, key caching and improved integration with key management systems to handle key life cycles and external key changes. **1. Managed Keys Infrastructure** - Introduction of `ManagedKeyProvider` interface for pluggable key provider implementations on the lines of the existing `KeyProvider` interface. - The new interface can also return Data Encryption Keys (DEKs) and a lot more details on the keys. - Comes with the default `ManagedKeyStoreKeyProvider` implementation using Java KeyStore, similar to the existing `KeyStoreKeyProvider`. - Enables logical key isolation for multi-tenant scenarios through custodian identifiers (future use cases) and the special default global custodian. - Hierarchical namespace resolution for DEKs with automatic fallback: explicit CF namespace attribute → constructed `table/family` namespace → table name → global namespace **2. System Key (STK) Management** - Cluster-wide system key for wrapping data encryption keys (DEKs). This is equivalent to the existing master key, but better managed and operation friendly. - Secure storage in HDFS with support for automatic key rotation during boot up. - Admin API to trigger key rotation and propagation to all RegionServers without needing to do a rolling restart. - Preserves the current double-wrapping architecture: DEKs wrapped by STK, STK sourced from external KMS **3. KeymetaAdmin API** - `enableKeyManagement(keyCust, keyNamespace)` - Enable key management for a custodian/namespace pair - `getManagedKeys(keyCust, keyNamespace)` - Query key status and metadata - `rotateSTK()` - Check for and propagate new system keys - `disableKeyManagement(keyCust, keyNamespace)` - Disable all the keys for a custodian/namespace - `disableManagedKey(keyCust, keyNamespace, keyMetadataHash)` - Disable a specific key - `rotateManagedKey(keyCust, keyNamespace)` - Rotate the active key - `refreshManagedKeys(keyCust, keyNamespace)` - Refresh from external KMS to validate all the keys. - Internal cache management operations for convenience and meeting SLAs. **4. Persistent Key Metadata Storage** - New system table `hbase:keymeta` for storing key metadata and state which acts as an `L2` cache. - Tracks key lifecycle: `ACTIVE`, `INACTIVE`, `DISABLED`, `FAILED` states - Stores wrapped DEKs and metadata for key lookup without depending on external KMS. - Optimized for high-priority access with in-memory column families - Key metadata tracking with cryptographic hashes for integrity verification **5. Multi-Layer Caching** - L1: In-memory Caffeine cache on RegionServers for hot key data - L2: Keymeta table for persistent key metadata that is shared across all RegionServers. - L3: Dynamic lookup from external KMS as fallback when not found in L2. - Cache invalidation mechanism for key rotation scenarios **6. HBase Shell Integration** - `enable_key_management` - Enable key management for a custodian and namespace - `show_key_status` - Display key status and metadata - `rotate_stk` - Trigger system key rotation - `disable_key_management` - Disable key management for a custodian and namespace - `disable_managed_key` - Disable a specific key - `rotate_managed_key` - Rotate the active key - `refresh_managed_keys` - Refresh all keys for a custodian and namespace - **Backward Compatibility:** Changes are fully compatible with existing encryption-at-rest configuration - **Gradual step-by-step migration**: Well defined migration path from existing configuration to new configuration - **Performance:** Minimal overhead through efficient caching and lazy key loading - **Security:** Cryptographic verification of key metadata, secure key wrapping - **Operability:** Administrative tools for key life cycle and cache management - **Extensibility:** Plugin architecture for custom key provider implementations - **Testing:** Comprehensive unit and integration tests coverage The implementation follows a layered architecture: 1. **Provider Layer:** Pluggable `ManagedKeyProvider` for KMS integration 2. **Management Layer:** `KeyMetaAdmin` API for administrative operations 3. **Persistence Layer:** `KeymetaTableAccessor` for metadata storage 4. **Cache Layer:** `ManagedKeyDataCache` and `SystemKeyCache` for performance 5. **Service Layer:** Coprocessor endpoints for client-server communication I would particularly appreciate feedback on: 1. **API Design:** Is the `KeymetaAdmin` API intuitive and complete for common key management scenarios? 2. **Security Model:** Does the double-wrapping architecture (DEK wrapped by STK, STK from KMS) provide appropriate security guarantees? 3. **Performance:** Are there potential bottlenecks in the caching strategy or table access patterns? 4. **Operational Aspects:** Are the administrative commands sufficient for the needs of operations and monitoring? 5. **Testing Coverage:** Are there additional test scenarios we should cover? 6. **Documentation:** Is the design document clear? What additional documentation would be helpful? 7. **Compatibility:** Any concerns about interaction with existing HBase features? After incorporating community feedback, I plan to: 1. Address any issues identified during review 2. Implement the work identified for future phases 3. Add additional documentation to the reference guide This PR introduces changes across multiple modules, so I recommend focusing on these **core components** first: **Core Architecture:** 1. Design document (linked above) - architectural overview 2. `ManagedKeyProvider`, `KeymetaAdmin`, `ManagedKeyData` interfaces (hbase-common) 3. `ManagedKeys.proto` - protocol definitions 4. `HMaster` and misc. procedure changes - initialization of `keymeta` in a predictable order 5. `FixedFileTrailer` + reader/writer changes - encode/decode additional encryption key in store files **Key Implementation:** 1. `KeymetaAdminImpl`, `KeymetaTableAccessor`, `ManagedKeyUtils`, `SystemKeyManager`, `SystemKeyAccessor` - admin operations and persistence 2. `ManagedKeyDataCache`, `SystemKeyCache` - caching layer 3. `SecurityUtil` - encryption context creation **Client & Shell:** 1. `KeymetaAdminClient` - client API 2. Shell commands and Ruby wrappers **Tests & Examples:** 1. `TestKeymetaAdminImpl`, `TestManagedKeymeta` - for usage patterns 2. `key_provider_keymeta_migration_test.rb` - E2E migration steps
3 participants
This commit prepares the codebase for the upcoming key management feature (HBASE-29368) by introducing the necessary API definitions, protocol buffer changes, and infrastructure refactoring. No functional changes are included; all implementation will follow in the feature PR.
This precursor PR essentially extracts the API surface definitions and infrastructure refactoring from the main feature PR (#7421) to facilitate easier review. By separating the ~15k line feature PR into a smaller precursor containing interface definitions, protocol changes, and method signature updates, the subsequent feature PR will focus purely on implementation logic.
API Surface Additions:
New interfaces:
Protocol buffer definitions:
Infrastructure Refactoring:
Encryption context creation:
Method signature updates:
New utility methods:
Stub Infrastructure:
Code Organization:
All stub implementations clearly document they are placeholders for the upcoming feature PR. Existing encryption functionality remains unchanged and continues to work as before.
Testing: