Introduce EnclaveID as identifier #1760
Conversation
|
Warning Rate Limit Exceeded@BedrockSquirrel has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 4 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates introduce a new functionality for managing enclave IDs, enhancing the system's ability to identify and handle enclaves. A unique ID, derived from the hash of the enclave's public key, is now generated and associated with each enclave. This addition is reflected across various components, including the enclave interface, logging mechanisms, and RPC servers, to ensure a cohesive and secure identification process throughout the system. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (3)
go/common/rpc/generated/enclave.pb.gois excluded by:!**/*.pb.go, !**/generated/**go/common/rpc/generated/enclave.protois excluded by:!**/generated/**go/common/rpc/generated/enclave_grpc.pb.gois excluded by:!**/*.pb.go, !**/generated/**
Files selected for processing (7)
- go/common/enclave.go (1 hunks)
- go/common/log/log.go (1 hunks)
- go/common/types.go (1 hunks)
- go/enclave/enclave.go (4 hunks)
- go/enclave/rpc_server.go (1 hunks)
- go/host/enclave/guardian.go (3 hunks)
- go/host/rpc/enclaverpc/enclave_client.go (1 hunks)
Files skipped from review due to trivial changes (1)
- go/common/types.go
Additional comments: 10
go/common/log/log.go (1)
- 24-24: The addition of the
EnclaveIDKeyconstant is consistent with the naming conventions and context of existing log keys. This change is straightforward and does not introduce any issues.go/common/enclave.go (1)
- 48-50: The addition of the
EnclaveID()method to theEnclaveinterface is correctly implemented. The method signature is consistent with the interface's design, returning both anEnclaveIDand aSystemError, which aligns with the error handling pattern used throughout the interface.go/enclave/rpc_server.go (1)
- 110-117: The implementation of the
EnclaveIDmethod in theRPCServerstruct correctly retrieves the enclave's ID from the underlying enclave interface. Error handling and logging are appropriately implemented, following the established patterns in the file.go/host/rpc/enclaverpc/enclave_client.go (1)
- 149-161: The implementation of the
EnclaveIDmethod in theClienttype correctly initiates an RPC call to retrieve the enclave ID, with appropriate timeout handling and error checking. The method returns the expectedEnclaveIDtype and aSystemError, aligning with the interface's design.go/host/enclave/guardian.go (2)
- 41-41: The introduction of
_emptyEnclaveIDas a global variable is a good practice for representing an uninitialized or default state ofEnclaveID. This approach simplifies comparisons and checks throughout the code.- 78-78: Adding
enclaveIDto theGuardianstruct aligns with the PR's objective to track and utilize the unique identifier of enclaves. This field is essential for distinguishing between different enclaves, especially in systems managing multiple enclaves.go/enclave/enclave.go (4)
- 90-90: The
enclaveIDfield in theenclaveImplstruct is correctly added to support the newEnclaveIDfunctionality.- 172-173: The calculation of
enclaveIDusinggethcrypto.Keccak256on the serialized public key is a secure and appropriate method for generating a unique identifier based on the enclave's cryptographic identity.- 271-271: Assigning the calculated
enclaveIDto theenclaveImplstruct'senclaveIDfield is correctly done, ensuring that the enclave's identifier is stored and accessible.- 907-909: The addition of the
EnclaveID()method to theenclaveImplstruct is correctly implemented, providing a way to retrieve the enclave's identifier.
go/host/enclave/guardian.go
Outdated
| if g.enclaveID != _emptyEnclaveID { | ||
| return errors.New("guardian already started") | ||
| } | ||
|
|
||
| // Identify the enclave before starting (the enclave generates its ID immediately at startup) | ||
| // (retry until we get the enclave ID or the host is stopping) | ||
| for g.enclaveID == _emptyEnclaveID && !g.hostInterrupter.IsStopping() { | ||
| enclID, err := g.enclaveClient.EnclaveID() | ||
| if err != nil { | ||
| g.logger.Warn("could not get enclave ID", log.ErrKey, err) | ||
| time.Sleep(_retryInterval) | ||
| continue | ||
| } | ||
| g.enclaveID = enclID | ||
| // include the enclave ID in guardian log messages (for multi-enclave nodes) | ||
| g.logger = g.logger.New(log.EnclaveIDKey, g.enclaveID) | ||
| g.logger.Info("Starting guardian process.") |
There was a problem hiding this comment.
The logic added to the Start method for retrieving and setting the EnclaveID is well-implemented. The use of a loop with a retry mechanism (lines 107-113) ensures that the Guardian can successfully obtain the EnclaveID from the enclave client, even if initial attempts fail. Additionally, the inclusion of the enclaveID in log messages (line 116) enhances the traceability of actions related to specific enclaves. However, consider adding a maximum retry limit or a timeout to prevent an indefinite loop in scenarios where the EnclaveID cannot be retrieved due to persistent issues.
+ var maxRetries = 5 // or another appropriate limit
+ var retries = 0
for g.enclaveID == _emptyEnclaveID && !g.hostInterrupter.IsStopping() && retries < maxRetries {
...
+ retries++
}
+ if retries == maxRetries {
+ return errors.New("failed to obtain enclave ID after maximum retries")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if g.enclaveID != _emptyEnclaveID { | |
| return errors.New("guardian already started") | |
| } | |
| // Identify the enclave before starting (the enclave generates its ID immediately at startup) | |
| // (retry until we get the enclave ID or the host is stopping) | |
| for g.enclaveID == _emptyEnclaveID && !g.hostInterrupter.IsStopping() { | |
| enclID, err := g.enclaveClient.EnclaveID() | |
| if err != nil { | |
| g.logger.Warn("could not get enclave ID", log.ErrKey, err) | |
| time.Sleep(_retryInterval) | |
| continue | |
| } | |
| g.enclaveID = enclID | |
| // include the enclave ID in guardian log messages (for multi-enclave nodes) | |
| g.logger = g.logger.New(log.EnclaveIDKey, g.enclaveID) | |
| g.logger.Info("Starting guardian process.") | |
| if g.enclaveID != _emptyEnclaveID { | |
| return errors.New("guardian already started") | |
| } | |
| // Identify the enclave before starting (the enclave generates its ID immediately at startup) | |
| // (retry until we get the enclave ID or the host is stopping) | |
| + var maxRetries = 5 // or another appropriate limit | |
| + var retries = 0 | |
| for g.enclaveID == _emptyEnclaveID && !g.hostInterrupter.IsStopping() && retries < maxRetries { | |
| enclID, err := g.enclaveClient.EnclaveID() | |
| if err != nil { | |
| g.logger.Warn("could not get enclave ID", log.ErrKey, err) | |
| time.Sleep(_retryInterval) | |
| + retries++ | |
| continue | |
| } | |
| g.enclaveID = enclID | |
| // include the enclave ID in guardian log messages (for multi-enclave nodes) | |
| g.logger = g.logger.New(log.EnclaveIDKey, g.enclaveID) | |
| g.logger.Info("Starting guardian process.") | |
| + if retries == maxRetries { | |
| + return errors.New("failed to obtain enclave ID after maximum retries") | |
| + } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- go/common/types.go (1 hunks)
- go/enclave/enclave.go (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- go/common/types.go
- go/enclave/enclave.go
go/enclave/enclave.go
Outdated
| @@ -87,6 +87,7 @@ type enclaveImpl struct { | |||
|
|
|||
| enclaveKey *ecdsa.PrivateKey // this is a key specific to this enclave, which is included in the Attestation. Used for signing rollups and for encryption of the shared secret. | |||
There was a problem hiding this comment.
feels like we should group these fields and the logic into a struct that only has that responsibility.
There was a problem hiding this comment.
Yeah good shout. Is the new commit the sort of thing you had in mind?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
go/common/rpc/generated/enclave.protois excluded by:!**/generated/**
Files selected for processing (6)
- go/enclave/crypto/enclave_key.go (1 hunks)
- go/enclave/enclave.go (9 hunks)
- go/enclave/nodetype/sequencer.go (5 hunks)
- go/enclave/storage/interfaces.go (1 hunks)
- go/enclave/storage/storage.go (1 hunks)
- go/host/enclave/guardian.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- go/host/enclave/guardian.go
Additional comments: 12
go/enclave/crypto/enclave_key.go (2)
- 12-18: The function
GenerateEnclaveKeycorrectly generates a new enclave key using Ethereum's crypto library and wraps it withNewEnclaveKey. This encapsulation ensures that the enclave key, along with its derived enclave ID, is properly initialized.- 30-51: The
EnclaveKeystruct and its associated methods (PrivateKey,PublicKey,EnclaveID,PublicKeyBytes) are correctly implemented, providing encapsulated access to the enclave's cryptographic materials and the derived enclave ID. This encapsulation promotes good security practices by controlling access to sensitive information.go/enclave/storage/interfaces.go (1)
- 117-118: The update to
EnclaveKeyStorageinterface to use*crypto.EnclaveKeyfor both storing and retrieving enclave keys aligns with the objective of enhancing the management of enclave keys. This change ensures that the enclave key and its associated data, such as the enclave ID, are handled together, improving the system's overall design coherence.go/enclave/nodetype/sequencer.go (4)
- 57-57: The replacement of
enclavePrivateKeywithenclaveKeyof type*crypto.EnclaveKeyin thesequencerstruct is a significant improvement. It ensures that the sequencer has access to both the private key and the enclave ID, facilitating more secure and identifiable operations.- 63-69: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [66-78]
The
NewSequencerfunction's signature change to accept*crypto.EnclaveKeyis correctly implemented. This change is necessary to accommodate the updatedsequencerstruct and ensures that the sequencer is initialized with the correct type of key.
- 425-425: The
signBatchmethod's update to uses.enclaveKey.PrivateKey()for signing batches is correct and necessary due to the changes in thesequencerstruct. This ensures that the method continues to function correctly with the new key management approach.- 435-435: Similarly, the
signRollupmethod's update to uses.enclaveKey.PrivateKey()for signing rollups is correctly implemented. This consistency in using thecrypto.EnclaveKeytype across the sequencer's operations is crucial for maintaining the integrity of the signing process.go/enclave/enclave.go (5)
- 87-87: The
enclaveKeyfield has been modified to use the*crypto.EnclaveKeytype. Ensure that all references and operations onenclaveKeythroughout the codebase are updated to reflect this change.- 858-861: The method
Attestationchecks ifenclaveKeyisnilbefore proceeding. This is a good practice for null safety. However, ensure that there are no scenarios whereenclaveKeycould benilduring normal operation, as this would indicate a more significant issue in the enclave initialization process.- 879-879: The method
GenerateSecretusescrypto.EncryptSecretwhich is expected to handle the encryption securely. Verify that the encryption standards (e.g., AES-GCM) and key management practices are up to the current security standards.- 892-892: In
InitEnclave, the secret is decrypted withcrypto.DecryptSecret. Ensure that error handling around decryption is robust, especially handling decryption failures due to incorrect keys or tampered data.- 904-906: The addition of the
EnclaveIDmethod is correctly implemented and follows the changes described. This method provides a straightforward way to retrieve theEnclaveID, enhancing the code's readability and maintainability.
| func NewEnclaveKey(privKey *ecdsa.PrivateKey) *EnclaveKey { | ||
| pubKey := gethcrypto.CompressPubkey(&privKey.PublicKey) | ||
| enclaveID := common.EnclaveID(pubKey[gethcommon.HashLength-gethcommon.AddressLength:]) | ||
| return &EnclaveKey{ | ||
| privateKey: privKey, | ||
| publicKeyBytes: pubKey, | ||
| enclaveID: enclaveID, | ||
| } | ||
| } |
There was a problem hiding this comment.
The NewEnclaveKey function correctly generates the enclave ID from the public key's bytes. However, the slicing operation to derive the enclave ID from the public key bytes assumes the length of the public key bytes and the constants from gethcommon will align correctly. This could be fragile if underlying assumptions change. Consider adding a check to ensure pubKey has the expected length before slicing.
+ if len(pubKey) < gethcommon.HashLength {
+ return nil, errors.New("public key bytes length is less than expected")
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func NewEnclaveKey(privKey *ecdsa.PrivateKey) *EnclaveKey { | |
| pubKey := gethcrypto.CompressPubkey(&privKey.PublicKey) | |
| enclaveID := common.EnclaveID(pubKey[gethcommon.HashLength-gethcommon.AddressLength:]) | |
| return &EnclaveKey{ | |
| privateKey: privKey, | |
| publicKeyBytes: pubKey, | |
| enclaveID: enclaveID, | |
| } | |
| } | |
| func NewEnclaveKey(privKey *ecdsa.PrivateKey) *EnclaveKey { | |
| pubKey := gethcrypto.CompressPubkey(&privKey.PublicKey) | |
| if len(pubKey) < gethcommon.HashLength { | |
| return nil, errors.New("public key bytes length is less than expected") | |
| } | |
| enclaveID := common.EnclaveID(pubKey[gethcommon.HashLength-gethcommon.AddressLength:]) | |
| return &EnclaveKey{ | |
| privateKey: privKey, | |
| publicKeyBytes: pubKey, | |
| enclaveID: enclaveID, | |
| } | |
| } |
| func (s *storageImpl) StoreEnclaveKey(enclaveKey *crypto.EnclaveKey) error { | ||
| defer s.logDuration("StoreEnclaveKey", measure.NewStopwatch()) | ||
| if enclaveKey == nil { | ||
| return errors.New("enclaveKey cannot be nil") | ||
| } | ||
| keyBytes := gethcrypto.FromECDSA(enclaveKey) | ||
| keyBytes := gethcrypto.FromECDSA(enclaveKey.PrivateKey()) |
There was a problem hiding this comment.
The StoreEnclaveKey method correctly serializes the private key part of crypto.EnclaveKey for storage. This approach is secure as it only stores the private key, which is necessary for enclave operations. However, ensure that the key is stored securely and consider encrypting the key before storage for enhanced security.
| func (s *storageImpl) GetEnclaveKey() (*crypto.EnclaveKey, error) { | ||
| defer s.logDuration("GetEnclaveKey", measure.NewStopwatch()) | ||
| keyBytes, err := enclavedb.FetchConfig(s.db.GetSQLDB(), enclaveKeyKey) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| enclaveKey, err := gethcrypto.ToECDSA(keyBytes) | ||
| ecdsaKey, err := gethcrypto.ToECDSA(keyBytes) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("unable to construct ECDSA private key from enclave key bytes - %w", err) | ||
| } | ||
| return enclaveKey, nil | ||
| return crypto.NewEnclaveKey(ecdsaKey), nil |
There was a problem hiding this comment.
The GetEnclaveKey method correctly retrieves and constructs an *crypto.EnclaveKey from stored bytes. This method ensures that the enclave key can be reconstructed accurately for use in the system. However, consider adding error handling for cases where the stored key might not exist or is corrupted.
go/enclave/enclave.go
Outdated
| } | ||
| } | ||
|
|
||
| serializedEnclavePubKey := gethcrypto.CompressPubkey(&enclaveKey.PublicKey) | ||
| logger.Info(fmt.Sprintf("Generated public key %s", gethcommon.Bytes2Hex(serializedEnclavePubKey))) | ||
| serializedEnclavePubKey := gethcrypto.CompressPubkey(enclaveKey.PublicKey()) | ||
| pubKeyHash := gethcrypto.Keccak256(serializedEnclavePubKey) | ||
| // the enclave ID is the address for its keypair (like eth addresses, the last 20 bytes of the hash of the public key) | ||
| enclaveID := pubKeyHash[gethcommon.HashLength-gethcommon.AddressLength:] | ||
| logger.Info(fmt.Sprintf("Generated public key. EnclaveID=%s, publicKey=%s", enclaveID, gethcommon.Bytes2Hex(serializedEnclavePubKey))) | ||
|
|
||
| obscuroKey := crypto.GetObscuroKey(logger) | ||
| rpcEncryptionManager := rpc.NewEncryptionManager(ecies.ImportECDSA(obscuroKey)) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [158-171]
The logic for generating the EnclaveID based on the public key's hash is correct and follows the Ethereum address generation pattern. However, consider encapsulating this logic within the crypto package or a method on the EnclaveKey type to improve modularity and reusability.
- serializedEnclavePubKey := gethcrypto.CompressPubkey(enclaveKey.PublicKey())
- pubKeyHash := gethcrypto.Keccak256(serializedEnclavePubKey)
- enclaveID := pubKeyHash[gethcommon.HashLength-gethcommon.AddressLength:]
+ enclaveID := enclaveKey.GenerateEnclaveID()And in crypto package:
func (k *EnclaveKey) GenerateEnclaveID() common.EnclaveID {
serializedPubKey := gethcrypto.CompressPubkey(k.PublicKey())
pubKeyHash := gethcrypto.Keccak256(serializedPubKey)
return pubKeyHash[gethcommon.HashLength-gethcommon.AddressLength:]
}
Why this change is needed
EnclaveID will be the identifier for enclaves that join the protocol.
It is calculated from the enclave's public key generated when it first starts up.
What changes were made as part of this PR
Add method to provide EnclaveID to the host. Have the guardian request the EnclaveID before starting its main loop and use it in its logging (this will be necessary when a host has more than one guardian).
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks