From 795ead76c18dfc6a6d5d425a7ee471160d50a986 Mon Sep 17 00:00:00 2001 From: Peter Nose Date: Wed, 6 Dec 2023 20:29:21 +0100 Subject: [PATCH 1/5] runtime: Use immutable context in key manager RPC calls --- keymanager/src/runtime/methods.rs | 32 +++++++++++++-------------- runtime/src/enclave_rpc/dispatcher.rs | 10 ++++----- runtime/src/macros.rs | 2 +- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/keymanager/src/runtime/methods.rs b/keymanager/src/runtime/methods.rs index c2fce15f5cd..6d81b6e4368 100644 --- a/keymanager/src/runtime/methods.rs +++ b/keymanager/src/runtime/methods.rs @@ -66,7 +66,7 @@ const MAX_EPHEMERAL_KEY_AGE: EpochTime = 10; const MAX_FRESH_HEIGHT_AGE: u64 = 50; /// Initialize the Kdf. -pub fn init_kdf(ctx: &mut RpcContext, req: &InitRequest) -> Result { +pub fn init_kdf(ctx: &RpcContext, req: &InitRequest) -> Result { let rctx = runtime_context!(ctx, KmContext); let runtime_id = rctx.runtime_id; let storage = ctx.untrusted_local_storage; @@ -96,7 +96,7 @@ pub fn init_kdf(ctx: &mut RpcContext, req: &InitRequest) -> Result Result { +pub fn get_or_create_keys(ctx: &RpcContext, req: &LongTermKeyRequest) -> Result { authorize_private_key_generation(ctx, &req.runtime_id)?; validate_height_freshness(ctx, req.height)?; @@ -109,7 +109,7 @@ pub fn get_or_create_keys(ctx: &mut RpcContext, req: &LongTermKeyRequest) -> Res } /// See `Kdf::get_public_key`. -pub fn get_public_key(ctx: &mut RpcContext, req: &LongTermKeyRequest) -> Result { +pub fn get_public_key(ctx: &RpcContext, req: &LongTermKeyRequest) -> Result { // No authentication or authorization. // Absolutely anyone is allowed to query public long-term keys. @@ -126,7 +126,7 @@ pub fn get_public_key(ctx: &mut RpcContext, req: &LongTermKeyRequest) -> Result< /// See `Kdf::get_or_create_ephemeral_keys`. pub fn get_or_create_ephemeral_keys( - ctx: &mut RpcContext, + ctx: &RpcContext, req: &EphemeralKeyRequest, ) -> Result { authorize_private_key_generation(ctx, &req.runtime_id)?; @@ -138,7 +138,7 @@ pub fn get_or_create_ephemeral_keys( /// See `Kdf::get_public_ephemeral_key`. pub fn get_public_ephemeral_key( - ctx: &mut RpcContext, + ctx: &RpcContext, req: &EphemeralKeyRequest, ) -> Result { // No authentication or authorization. @@ -163,7 +163,7 @@ pub fn get_public_ephemeral_key( /// See `Kdf::replicate_master_secret`. pub fn replicate_master_secret( - ctx: &mut RpcContext, + ctx: &RpcContext, req: &ReplicateMasterSecretRequest, ) -> Result { authorize_secret_replication(ctx)?; @@ -181,7 +181,7 @@ pub fn replicate_master_secret( /// See `Kdf::replicate_ephemeral_secret`. pub fn replicate_ephemeral_secret( - ctx: &mut RpcContext, + ctx: &RpcContext, req: &ReplicateEphemeralSecretRequest, ) -> Result { authorize_secret_replication(ctx)?; @@ -193,7 +193,7 @@ pub fn replicate_ephemeral_secret( /// Generate a master secret and encrypt it using the key manager's REK keys. pub fn generate_master_secret( - ctx: &mut RpcContext, + ctx: &RpcContext, req: &GenerateMasterSecretRequest, ) -> Result { let kdf = Kdf::global(); @@ -228,7 +228,7 @@ pub fn generate_master_secret( /// Generate an ephemeral secret and encrypt it using the key manager's REK keys. pub fn generate_ephemeral_secret( - ctx: &mut RpcContext, + ctx: &RpcContext, req: &GenerateEphemeralSecretRequest, ) -> Result { let kdf = Kdf::global(); @@ -260,7 +260,7 @@ pub fn generate_ephemeral_secret( /// Encrypt a secret using the Deoxys-II MRAE algorithm and the key manager's REK keys. fn encrypt_secret( - ctx: &mut RpcContext, + ctx: &RpcContext, secret: Secret, checksum: Vec, additional_data: Vec, @@ -302,7 +302,7 @@ fn encrypt_secret( } /// Decrypt and store a proposal for the next master secret. -pub fn load_master_secret(ctx: &mut RpcContext, req: &LoadMasterSecretRequest) -> Result<()> { +pub fn load_master_secret(ctx: &RpcContext, req: &LoadMasterSecretRequest) -> Result<()> { let signed_secret = validate_signed_master_secret(ctx, &req.signed_secret)?; let secret = match decrypt_master_secret(ctx, &signed_secret)? { @@ -321,7 +321,7 @@ pub fn load_master_secret(ctx: &mut RpcContext, req: &LoadMasterSecretRequest) - /// Decrypt and store an ephemeral secret. If decryption fails, try to replicate the secret /// from another key manager enclave. -pub fn load_ephemeral_secret(ctx: &mut RpcContext, req: &LoadEphemeralSecretRequest) -> Result<()> { +pub fn load_ephemeral_secret(ctx: &RpcContext, req: &LoadEphemeralSecretRequest) -> Result<()> { let signed_secret = validate_signed_ephemeral_secret(ctx, &req.signed_secret)?; let secret = match decrypt_ephemeral_secret(ctx, &signed_secret)? { @@ -356,7 +356,7 @@ pub fn load_ephemeral_secret(ctx: &mut RpcContext, req: &LoadEphemeralSecretRequ /// Decrypt master secret with local REK key. fn decrypt_master_secret( - ctx: &mut RpcContext, + ctx: &RpcContext, secret: &EncryptedMasterSecret, ) -> Result> { let generation = secret.generation; @@ -390,7 +390,7 @@ fn decrypt_master_secret( /// Decrypt ephemeral secret with local REK key. fn decrypt_ephemeral_secret( - ctx: &mut RpcContext, + ctx: &RpcContext, secret: &EncryptedEphemeralSecret, ) -> Result> { let epoch = secret.epoch; @@ -422,7 +422,7 @@ fn decrypt_ephemeral_secret( } /// Key manager client for master and ephemeral secret replication. -fn key_manager_client_for_replication(ctx: &mut RpcContext) -> RemoteClient { +fn key_manager_client_for_replication(ctx: &RpcContext) -> RemoteClient { let rctx = runtime_context!(ctx, KmContext); let protocol = rctx.protocol.clone(); let runtime_id = rctx.runtime_id; @@ -442,7 +442,7 @@ fn key_manager_client_for_replication(ctx: &mut RpcContext) -> RemoteClient { /// Create init response and sign it with RAK. fn sign_init_response( - ctx: &mut RpcContext, + ctx: &RpcContext, state: State, policy_checksum: Vec, ) -> Result { diff --git a/runtime/src/enclave_rpc/dispatcher.rs b/runtime/src/enclave_rpc/dispatcher.rs index 193ffae918b..5fe432af42e 100644 --- a/runtime/src/enclave_rpc/dispatcher.rs +++ b/runtime/src/enclave_rpc/dispatcher.rs @@ -47,16 +47,16 @@ pub struct MethodDescriptor { /// Handler for a RPC method. pub trait MethodHandler { /// Invoke the method implementation and return a response. - fn handle(&self, ctx: &mut Context, request: &Rq) -> Result; + fn handle(&self, ctx: &Context, request: &Rq) -> Result; } impl MethodHandler for F where Rq: 'static, Rsp: 'static, - F: Fn(&mut Context, &Rq) -> Result + 'static, + F: Fn(&Context, &Rq) -> Result + 'static, { - fn handle(&self, ctx: &mut Context, request: &Rq) -> Result { + fn handle(&self, ctx: &Context, request: &Rq) -> Result { (*self)(ctx, request) } } @@ -67,7 +67,7 @@ pub trait MethodHandlerDispatch { fn get_descriptor(&self) -> &MethodDescriptor; /// Dispatch request. - fn dispatch(&self, ctx: &mut Context, request: Request) -> Result; + fn dispatch(&self, ctx: &Context, request: Request) -> Result; } struct MethodHandlerDispatchImpl { @@ -86,7 +86,7 @@ where &self.descriptor } - fn dispatch(&self, ctx: &mut Context, request: Request) -> Result { + fn dispatch(&self, ctx: &Context, request: Request) -> Result { let request = cbor::from_value(request.args)?; let response = self.handler.handle(ctx, &request)?; diff --git a/runtime/src/macros.rs b/runtime/src/macros.rs index 7e058d4cb00..8a2ff783eb9 100644 --- a/runtime/src/macros.rs +++ b/runtime/src/macros.rs @@ -13,7 +13,7 @@ macro_rules! runtime_context { ($ctx:ident, $type:ty) => { $ctx.runtime - .downcast_mut::<$type>() + .downcast_ref::<$type>() .expect("invalid runtime context") }; } From e32917651e91fd2d1953944b4892d521b58827d2 Mon Sep 17 00:00:00 2001 From: Peter Nose Date: Mon, 15 Jan 2024 10:08:19 +0100 Subject: [PATCH 2/5] go/worker/keymanager: Allow only insecure public key queries --- go/worker/keymanager/worker.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/go/worker/keymanager/worker.go b/go/worker/keymanager/worker.go index 3b037da3b79..6969d23e5eb 100644 --- a/go/worker/keymanager/worker.go +++ b/go/worker/keymanager/worker.go @@ -202,8 +202,6 @@ func (w *Worker) CallEnclave(ctx context.Context, data []byte, kind enclaverpc.K switch frame.UntrustedPlaintext { case "": // Anyone can connect. - case api.RPCMethodGetPublicKey, api.RPCMethodGetPublicEphemeralKey: - // Anyone can get public keys. default: if _, privatePeered := w.privatePeers[peerID]; !privatePeered { // Defer to access control to check the policy. From 91d76bc171592d45d5cf374054fdb409559a35b5 Mon Sep 17 00:00:00 2001 From: Peter Nose Date: Tue, 16 Jan 2024 12:24:59 +0100 Subject: [PATCH 3/5] go/worker/keymanager: Fetch hardware type from runtime registry --- go/worker/keymanager/worker.go | 40 ++++++++++++++-------------------- 1 file changed, 16 insertions(+), 24 deletions(-) diff --git a/go/worker/keymanager/worker.go b/go/worker/keymanager/worker.go index 6969d23e5eb..8590bcb0448 100644 --- a/go/worker/keymanager/worker.go +++ b/go/worker/keymanager/worker.go @@ -643,21 +643,12 @@ func (w *Worker) generateMasterSecret(runtimeID common.Namespace, height int64, return fmt.Errorf("failed to generate master secret: %w", err) } - // Fetch key manager runtime details. - kmRt, err := w.commonWorker.Consensus.Registry().GetRuntime(w.ctx, ®istry.GetRuntimeQuery{ - Height: consensus.HeightLatest, - ID: kmStatus.ID, - }) - if err != nil { - return err - } - - rak, err := w.runtimeAttestationKey(rtStatus, kmRt) + rak, err := w.runtimeAttestationKey(rtStatus) if err != nil { return err } - reks, err := w.runtimeEncryptionKeys(kmStatus, kmRt) + reks, err := w.runtimeEncryptionKeys(kmStatus) if err != nil { return err } @@ -720,21 +711,12 @@ func (w *Worker) generateEphemeralSecret(runtimeID common.Namespace, height int6 return fmt.Errorf("failed to generate ephemeral secret: %w", err) } - // Fetch key manager runtime details. - kmRt, err := w.commonWorker.Consensus.Registry().GetRuntime(w.ctx, ®istry.GetRuntimeQuery{ - Height: consensus.HeightLatest, - ID: kmStatus.ID, - }) - if err != nil { - return err - } - - rak, err := w.runtimeAttestationKey(rtStatus, kmRt) + rak, err := w.runtimeAttestationKey(rtStatus) if err != nil { return err } - reks, err := w.runtimeEncryptionKeys(kmStatus, kmRt) + reks, err := w.runtimeEncryptionKeys(kmStatus) if err != nil { return err } @@ -757,7 +739,12 @@ func (w *Worker) generateEphemeralSecret(runtimeID common.Namespace, height int6 return err } -func (w *Worker) runtimeAttestationKey(rtStatus *runtimeStatus, kmRt *registry.Runtime) (*signature.PublicKey, error) { +func (w *Worker) runtimeAttestationKey(rtStatus *runtimeStatus) (*signature.PublicKey, error) { + kmRt, err := w.runtime.RegistryDescriptor(w.ctx) + if err != nil { + return nil, err + } + var rak *signature.PublicKey switch kmRt.TEEHardware { case node.TEEHardwareInvalid: @@ -774,7 +761,12 @@ func (w *Worker) runtimeAttestationKey(rtStatus *runtimeStatus, kmRt *registry.R return rak, nil } -func (w *Worker) runtimeEncryptionKeys(kmStatus *api.Status, kmRt *registry.Runtime) (map[x25519.PublicKey]struct{}, error) { +func (w *Worker) runtimeEncryptionKeys(kmStatus *api.Status) (map[x25519.PublicKey]struct{}, error) { + kmRt, err := w.runtime.RegistryDescriptor(w.ctx) + if err != nil { + return nil, err + } + reks := make(map[x25519.PublicKey]struct{}) for _, id := range kmStatus.Nodes { var n *node.Node From c7ae3970df1c6a675b9510d4814fac7fa72e0e1c Mon Sep 17 00:00:00 2001 From: Peter Nose Date: Mon, 15 Jan 2024 15:24:04 +0100 Subject: [PATCH 4/5] go/worker/keymanager: Fix race condition when accessing runtime status --- go/worker/keymanager/status.go | 8 +------- go/worker/keymanager/worker.go | 10 ++++++++++ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/go/worker/keymanager/status.go b/go/worker/keymanager/status.go index 3d24d93143a..f7665ae9463 100644 --- a/go/worker/keymanager/status.go +++ b/go/worker/keymanager/status.go @@ -4,7 +4,6 @@ import ( "github.com/libp2p/go-libp2p/core/peer" "github.com/oasisprotocol/oasis-core/go/common" - "github.com/oasisprotocol/oasis-core/go/common/version" "github.com/oasisprotocol/oasis-core/go/worker/keymanager/api" ) @@ -61,15 +60,10 @@ func (w *Worker) GetStatus() (*api.Status, error) { pc = w.enclaveStatus.InitResponse.PolicyChecksum } - var aw *version.Version - if w.rtStatus != nil { - aw = &w.rtStatus.version - } - gs := w.globalStatus ws := api.WorkerStatus{ Status: ss, - ActiveVersion: aw, + ActiveVersion: w.activeVersion, MayGenerate: w.mayGenerate, RuntimeID: &w.runtimeID, ClientRuntimes: rts, diff --git a/go/worker/keymanager/worker.go b/go/worker/keymanager/worker.go index 8590bcb0448..8e85db657e8 100644 --- a/go/worker/keymanager/worker.go +++ b/go/worker/keymanager/worker.go @@ -94,6 +94,7 @@ type Worker struct { // nolint: maligned globalStatus *api.Status enclaveStatus *api.SignedInitResponse policy *api.SignedPolicySGX + activeVersion *version.Version masterSecretStats workerKeymanager.MasterSecretStats ephemeralSecretStats workerKeymanager.EphemeralSecretStats @@ -408,6 +409,13 @@ func (w *Worker) setStatus(status *api.Status) { w.globalStatus = status } +func (w *Worker) setVersion(v *version.Version) { + w.Lock() + defer w.Unlock() + + w.activeVersion = v +} + func (w *Worker) setLastGeneratedMasterSecretGeneration(generation uint64) { w.Lock() defer w.Unlock() @@ -1029,6 +1037,7 @@ func (w *Worker) handleRuntimeHostEvent(ev *host.Event) { default: return } + w.setVersion(&w.rtStatus.version) if w.kmStatus == nil { return @@ -1055,6 +1064,7 @@ func (w *Worker) handleRuntimeHostEvent(ev *host.Event) { case ev.FailedToStart != nil, ev.Stopped != nil: // Worker failed to start or was stopped -- we can no longer service requests. w.rtStatus = nil + w.setVersion(nil) w.roleProvider.SetUnavailable() default: // Unknown event. From 135bd82b4154b0a8a7afe579a88b2074c6524eba Mon Sep 17 00:00:00 2001 From: Peter Nose Date: Tue, 16 Jan 2024 11:03:05 +0100 Subject: [PATCH 5/5] go/worker/keymanager: Fix race condition when accessing enclave status --- .changelog/5529.bugfix.md | 1 + go/worker/keymanager/status.go | 7 +------ go/worker/keymanager/worker.go | 22 ++++++++++++++-------- 3 files changed, 16 insertions(+), 14 deletions(-) create mode 100644 .changelog/5529.bugfix.md diff --git a/.changelog/5529.bugfix.md b/.changelog/5529.bugfix.md new file mode 100644 index 00000000000..8f1722b4ea0 --- /dev/null +++ b/.changelog/5529.bugfix.md @@ -0,0 +1 @@ +go/worker/keymanager: Fix race conditions when accessing status fields diff --git a/go/worker/keymanager/status.go b/go/worker/keymanager/status.go index f7665ae9463..2cda41d318a 100644 --- a/go/worker/keymanager/status.go +++ b/go/worker/keymanager/status.go @@ -55,11 +55,6 @@ func (w *Worker) GetStatus() (*api.Status, error) { al = append(al, ral) } - var pc []byte - if w.enclaveStatus != nil { - pc = w.enclaveStatus.InitResponse.PolicyChecksum - } - gs := w.globalStatus ws := api.WorkerStatus{ Status: ss, @@ -70,7 +65,7 @@ func (w *Worker) GetStatus() (*api.Status, error) { AccessList: al, PrivatePeers: ps, Policy: w.policy, - PolicyChecksum: pc, + PolicyChecksum: w.policyChecksum, MasterSecrets: w.masterSecretStats, EphemeralSecrets: w.ephemeralSecretStats, } diff --git a/go/worker/keymanager/worker.go b/go/worker/keymanager/worker.go index 8e85db657e8..6caaf4c4eee 100644 --- a/go/worker/keymanager/worker.go +++ b/go/worker/keymanager/worker.go @@ -91,10 +91,10 @@ type Worker struct { // nolint: maligned roleProvider registration.RoleProvider backend api.Backend - globalStatus *api.Status - enclaveStatus *api.SignedInitResponse - policy *api.SignedPolicySGX - activeVersion *version.Version + globalStatus *api.Status + policy *api.SignedPolicySGX + policyChecksum []byte + activeVersion *version.Version masterSecretStats workerKeymanager.MasterSecretStats ephemeralSecretStats workerKeymanager.EphemeralSecretStats @@ -358,13 +358,13 @@ func (w *Worker) initEnclave(kmStatus *api.Status, rtStatus *runtimeStatus) (*ap // Update metrics. enclaveMasterSecretGenerationNumber.WithLabelValues(w.runtimeLabel).Set(float64(kmStatus.Generation)) - if w.enclaveStatus == nil || !bytes.Equal(w.enclaveStatus.InitResponse.PolicyChecksum, signedInitResp.InitResponse.PolicyChecksum) { + if !bytes.Equal(w.policyChecksum, signedInitResp.InitResponse.PolicyChecksum) { policyUpdateCount.WithLabelValues(w.runtimeLabel).Inc() } - // Cache the key manager enclave status and the currently active policy. - w.enclaveStatus = &signedInitResp + // Cache the currently active policy and its checksum. w.policy = kmStatus.Policy + w.policyChecksum = signedInitResp.InitResponse.PolicyChecksum return &signedInitResp, nil } @@ -1043,10 +1043,16 @@ func (w *Worker) handleRuntimeHostEvent(ev *host.Event) { return } + // Check whether the enclave has been initialized at least once. + // If true, preregistration is not required. + w.RLock() + initialized := w.policyChecksum != nil + w.RUnlock() + // Send a node preregistration, so that other nodes know to update their access // control. Without it, the enclave won't be able to replicate the master secrets // needed for initialization. - if w.enclaveStatus == nil { + if !initialized { rtStatus := w.rtStatus w.roleProvider.SetAvailableWithCallback(func(n *node.Node) error { rt := n.AddOrUpdateRuntime(w.runtime.ID(), rtStatus.version)