-
Notifications
You must be signed in to change notification settings - Fork 19
refactor keys #1177
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
base: main
Are you sure you want to change the base?
refactor keys #1177
Conversation
|
/run-security-scan |
alexcos20
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI automated code review (Gemini 3).
Overall risk: medium
Summary:
This pull request introduces a significant architectural refactoring focused on centralizing key management and blockchain interaction within OceanNode. It introduces two new core components: KeyManager and BlockchainRegistry. The KeyManager abstracts different key sources (e.g., raw private keys, future KMS integrations) and consolidates all cryptographic operations. The BlockchainRegistry provides a centralized, cached mechanism for obtaining Blockchain instances for specific chain IDs. The Blockchain class itself has been updated to use ethers.FallbackProvider for more robust RPC handling and now integrates with the KeyManager. The changes also include a cleaner approach to passing caller information (IP/PeerID) through command objects instead of a mutable remoteCaller property on the OceanNode singleton. This refactoring greatly improves the modularity, extensibility, and security posture of the node, while simplifying the Blockchain class by leveraging ethers.FallbackProvider's built-in retry logic.
Comments:
• [WARNING][bug] The PRIVATE_KEY environment variable is marked as required: false here, but src/utils/config/builder.ts (line 224 in current diff) explicitly throws an error if privateKey is missing when buildMergedConfig is called. If RawPrivateKeyProvider is the default and only currently working key provider, then PRIVATE_KEY is effectively required. This inconsistency should be resolved. Either required should be true or the buildMergedConfig logic needs to be adapted to allow alternative key sources to omit PRIVATE_KEY.
• [WARNING][style] The getLibp2pPrivateKey(): any return type is very broad. If a specific type from @libp2p/interface/keys is available for PrivateKey, it would be better to use that for stronger type safety.
• [WARNING][bug] The KeyProviderType enum is defined as 'raw' | 'gcp-kms'. However, docs/KeyManager.md mentions AwsKmsProvider as a future provider, implying 'aws' should also be part of this type. Please update the type definition to include all planned providers for completeness.
• [INFO][performance] The getEvmSigner uses chainId as the cache key. While FallbackProvider abstracts multiple RPCs into a single provider, if in the future the system needs to support multiple distinct FallbackProvider instances for the same chain ID (e.g., for different purposes or configurations), this cache key might lead to conflicts. The 'TO DO' comment indicates this is known. For now, it's likely fine, but keep this in mind for future enhancements if such a scenario arises.
• [INFO][performance] The encryptStream method for ECIES collects the entire stream into a buffer before encrypting and then pushing it as a single chunk. This means it's not truly streaming for ECIES and could consume significant memory for very large files. While ECIES often isn't stream-friendly by design, it's an important performance/memory characteristic to note, especially if large files are expected to be encrypted via ECIES. Consider adding a comment in the code to highlight this behavior.
• [STYLE][style] Please remove or replace the console.log('detectNetwork block', block) with a CORE_LOGGER.debug statement. console.log should generally be avoided in production code.
• [INFO][other] The OceanNode constructor has keyManager?: KeyManager, blockchainRegistry?: BlockchainRegistry as optional. However, if they are undefined, they are initialized using new KeyManager(config) and new BlockchainRegistry(this.keyManager, config). This is a common pattern, but it means that if external code relies on injecting specific KeyManager or BlockchainRegistry instances (e.g., for advanced testing or custom implementations), they must explicitly pass them. Otherwise, default instances are created. This is generally acceptable, but it's important to be aware of the default behavior.
• [INFO][style] The checkRateLimit method now correctly takes caller as a parameter, and the remoteCaller property on OceanNode has been removed. This refactoring is a positive change, as it promotes better separation of concerns by passing request-specific context directly through arguments rather than relying on mutable state on a global singleton.
|
Will handle the ```[WARNING][bug]`` in separate issue |
Refactor: Centralize Key and Blockchain Management in OceanNode
Summary
This PR refactors the OceanNode singleton to centralize key management and blockchain connection management. The refactoring introduces a new
KeyManagercomponent with a pluggable key provider architecture, and aBlockchainRegistryto manage blockchain instances per chain ID. This improves modularity, testability, and sets the foundation for future key management solutions (e.g., GCP KMS).Architecture Changes
Before
config.keysBlockchaininstances were created ad-hoc in 22+ locationsAfter
Key Components
1. KeyManager (
src/components/KeyManager/index.ts)Centralizes all key management for OceanNode:
peerId, libp2p keys, EVM address, and EVM signersIKeyProviderinterface to support multiple key sourcesRawPrivateKeyProviderfor raw private keysGcpKmsProvideror other key providersKey Methods:
getPeerId(): Returns libp2p PeerIdgetLibp2pPrivateKey(): Returns libp2p private keygetEthAddress(): Returns Ethereum addressgetEvmSigner(provider): Returns cached EVM signer for a providerencrypt(data, algorithm): Encrypts data using node keysdecrypt(data, algorithm): Decrypts data using node keysencryptStream(stream, algorithm): Encrypts streams (AES/ECIES)decryptStream(stream, algorithm): Decrypts streams (AES/ECIES)2. IKeyProvider Interface (
src/@types/KeyManager.ts)Defines the contract for all key provider implementations:
getType(): Returns provider type ('raw' | 'gcp-kms')getPeerId(),getLibp2pPrivateKey(),getPublicKey()getEthAddress(),getEthWallet()getRawPrivateKeyBytes(): For EVM signer creationencrypt(),decrypt(): Data encryption/decryptionencryptStream(),decryptStream(): Stream encryption/decryptioncleanup?(): Optional cleanup method3. RawPrivateKeyProvider (
src/components/KeyManager/providers/RawPrivateKeyProvider.ts)Implements
IKeyProviderfor raw private keys:initialize()needed)peerId, libp2p keys, and Ethereum address from raw private keyTransformstreams for efficient processing4. BlockchainRegistry (
src/components/BlockchainRegistry/index.ts)Manages
Blockchaininstances per chain ID:Blockchaininstances on-demandBlockchaininstances for the same chain IDgetBlockchain(chainId)method that returnsBlockchain | null5. Refactored Blockchain Class (
src/utils/blockchain.ts)Updated to work with the new architecture:
KeyManagerand uses it to get signersOceanNodeConfigfor signer creationKeyManager.getEvmSigner(provider)6. Updated OceanNode (
src/OceanNode.ts)Enhanced singleton with new components:
KeyManagerandBlockchainRegistryas parameters (dependency injection)getInstance()method updated to require these componentsgetKeyManager(): Returns KeyManager instancegetBlockchainRegistry(): Returns BlockchainRegistry instancegetBlockchain(chainId): Convenience method delegating to BlockchainRegistryMigration Changes
Handlers Updated
All handlers now use
OceanNode.getBlockchain()instead of creating newBlockchaininstances:ddoHandler.ts: UsesoceanNode.getBlockchain(chainId)for blockchain accessdownloadHandler.ts: Migrated to use centralized blockchain accessstartCompute.ts: Updated multiple locations to useoceanNode.getBlockchain(chainId)initialize.ts: Updated compute initialization to use centralized blockchaincollectFeesHandler.ts: Admin handler now usesoceanNode.getBlockchain(chainId)Escrow Refactoring (
src/components/core/utils/escrow.ts)BlockchainRegistryas a parametergetBlockchain(chainId)helper methodgetPaymentAmountInWei,getNumberFromWei,getUserAvailableFunds,getLocks,getAuthorizations,createLock,claimLock,cancelExpiredLocks) now use the centralized helperexistingChainparameter patterns - cleaner APIIndexer Updates (
src/components/Indexer/index.ts)BlockchainRegistryas a parameterstartThread()method retrievesBlockchaininstances viablockchainRegistry.getBlockchain(chainID)P2P Updates (
src/components/P2P/index.ts)KeyManageras a parameterconfig.keys.peerId,config.keys.publicKey,config.keys.privateKeywithKeyManagermethods:keyManager.getPeerIdString()keyManager.getLibp2pPublicKey()keyManager.getLibp2pPrivateKey()Main Initialization (
src/index.ts)KeyManagerinstance:new KeyManager(config)BlockchainRegistryinstance:new BlockchainRegistry(keyManager, config)OceanNode.getInstance()await keyManager.initialize()call (initialization happens in constructor)Configuration Changes
OceanNodeKeys Interface (
src/@types/OceanNode.ts)Enhanced to support key provider selection:
type?: 'raw' | 'gcp-kms': Specifies the key provider typerawPrivateKey?: string: For raw private key configuration (future)gcpKmsConfig?: For future GCP KMS integrationNote: The
typefield is currently inferred from the presence ofprivateKeyin config, but the interface supports explicit type specification for future extensibility.Stream Encryption/Decryption
Added support for streaming encryption/decryption in
RawPrivateKeyProvider:encryptStream(inputStream, algorithm): Encrypts aReadablestreamcrypto.createCipheriv()for streaming encryptiondecryptStream(inputStream, algorithm): Decrypts aReadablestreamcrypto.createDecipheriv()for streaming decryptionBenefits
KeyManager, making it easier to audit and secureTesting
Updated test files to work with the new architecture:
src/test/integration/credentials.test.ts: UsesblockchainRegistry.getBlockchain()src/test/integration/accessLists.test.ts: Updated to use new blockchain access patternsrc/test/unit/blockchain.test.ts: TestsBlockchainRegistryfunctionalityFiles Changed
New Files
src/@types/KeyManager.ts: Type definitions for key provider architecturesrc/components/KeyManager/index.ts: Main KeyManager classsrc/components/KeyManager/providers/RawPrivateKeyProvider.ts: Raw private key provider implementationsrc/components/BlockchainRegistry/index.ts: Blockchain registry implementationdocs/KeyManager.md: Documentation for KeyManagerModified Files
src/OceanNode.ts: Added KeyManager and BlockchainRegistry integrationsrc/index.ts: Updated initialization flowsrc/utils/blockchain.ts: Refactored to use KeyManagersrc/@types/OceanNode.ts: Enhanced OceanNodeKeys interfacesrc/components/P2P/index.ts: Uses KeyManager for P2P keyssrc/components/Indexer/index.ts: Uses BlockchainRegistrysrc/components/core/utils/escrow.ts: Refactored to use BlockchainRegistrysrc/components/core/handler/ddoHandler.ts: Uses OceanNode.getBlockchain()src/components/core/handler/downloadHandler.ts: Uses OceanNode.getBlockchain()src/components/core/compute/startCompute.ts: Uses OceanNode.getBlockchain()src/components/core/compute/initialize.ts: Uses OceanNode.getBlockchain()src/components/core/admin/collectFeesHandler.ts: Uses OceanNode.getBlockchain()Related Issues
This refactoring addresses the need for:
Later edit:
Rate limit refactor
Refactor rate limiting to pass caller as command parameter instead of using OceanNode state
This commit refactors rate limiting by:
This removes stateful caller tracking from OceanNode and makes the caller explicit in the command, improving clarity and testability.