From b1377d991a1d4fd71cf275d96da343ca558993b7 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Mon, 9 Aug 2021 14:35:24 +0100 Subject: [PATCH] Cross-signing signature handling (#1965) * Handle other signatures * Decorate key ID properly * Match by key IDs * Tweaks * Fixes * Fix /user/keys/query bug, review comments, update sytest-whitelist * Various wtweaks * Fix wiring for keyserver in API mode * Additional fixes --- cmd/dendrite-monolith-server/main.go | 5 + federationapi/routing/devices.go | 7 +- federationapi/routing/keys.go | 10 +- go.mod | 2 +- go.sum | 4 +- keyserver/api/api.go | 4 +- keyserver/internal/cross_signing.go | 186 +++++++++++++++------------ keyserver/internal/internal.go | 24 ++-- keyserver/inthttp/server.go | 4 +- sytest-whitelist | 1 + 10 files changed, 145 insertions(+), 102 deletions(-) diff --git a/cmd/dendrite-monolith-server/main.go b/cmd/dendrite-monolith-server/main.go index bb313e57..4d8e2ee1 100644 --- a/cmd/dendrite-monolith-server/main.go +++ b/cmd/dendrite-monolith-server/main.go @@ -120,6 +120,11 @@ func main() { Impl: userAPI, } } + // needs to be after the SetUserAPI call above + if base.UseHTTPAPIs { + keyserver.AddInternalRoutes(base.InternalAPIMux, keyAPI) + keyAPI = base.KeyServerHTTPClient() + } eduInputAPI := eduserver.NewInternalAPI( base, cache.New(), userAPI, diff --git a/federationapi/routing/devices.go b/federationapi/routing/devices.go index b397c546..4cd19996 100644 --- a/federationapi/routing/devices.go +++ b/federationapi/routing/devices.go @@ -38,12 +38,11 @@ func GetUserDevices( } sigReq := &keyapi.QuerySignaturesRequest{ - TargetIDs: map[string][]gomatrixserverlib.KeyID{}, + TargetIDs: map[string][]gomatrixserverlib.KeyID{ + userID: {}, + }, } sigRes := &keyapi.QuerySignaturesResponse{} - for _, dev := range res.Devices { - sigReq.TargetIDs[userID] = append(sigReq.TargetIDs[userID], gomatrixserverlib.KeyID(dev.DeviceID)) - } keyAPI.QuerySignatures(req.Context(), sigReq, sigRes) response := gomatrixserverlib.RespUserDevices{ diff --git a/federationapi/routing/keys.go b/federationapi/routing/keys.go index d73161e9..bba3272b 100644 --- a/federationapi/routing/keys.go +++ b/federationapi/routing/keys.go @@ -71,8 +71,14 @@ func QueryDeviceKeys( return util.JSONResponse{ Code: 200, JSON: struct { - DeviceKeys interface{} `json:"device_keys"` - }{queryRes.DeviceKeys}, + DeviceKeys interface{} `json:"device_keys"` + MasterKeys interface{} `json:"master_keys"` + SelfSigningKeys interface{} `json:"self_signing_keys"` + }{ + queryRes.DeviceKeys, + queryRes.MasterKeys, + queryRes.SelfSigningKeys, + }, } } diff --git a/go.mod b/go.mod index 39f6020c..e5d15756 100644 --- a/go.mod +++ b/go.mod @@ -31,7 +31,7 @@ require ( github.com/matrix-org/go-http-js-libp2p v0.0.0-20200518170932-783164aeeda4 github.com/matrix-org/go-sqlite3-js v0.0.0-20210709140738-b0d1ba599a6d github.com/matrix-org/gomatrix v0.0.0-20210324163249-be2af5ef2e16 - github.com/matrix-org/gomatrixserverlib v0.0.0-20210805104751-03e40fa2c0e1 + github.com/matrix-org/gomatrixserverlib v0.0.0-20210809130922-d9c3f400582b github.com/matrix-org/naffka v0.0.0-20210623111924-14ff508b58e0 github.com/matrix-org/pinecone v0.0.0-20210623102758-74f885644c1b github.com/matrix-org/util v0.0.0-20200807132607-55161520e1d4 diff --git a/go.sum b/go.sum index df5e2b85..d848988d 100644 --- a/go.sum +++ b/go.sum @@ -994,8 +994,8 @@ github.com/matrix-org/go-sqlite3-js v0.0.0-20210709140738-b0d1ba599a6d/go.mod h1 github.com/matrix-org/gomatrix v0.0.0-20190528120928-7df988a63f26/go.mod h1:3fxX6gUjWyI/2Bt7J1OLhpCzOfO/bB3AiX0cJtEKud0= github.com/matrix-org/gomatrix v0.0.0-20210324163249-be2af5ef2e16 h1:ZtO5uywdd5dLDCud4r0r55eP4j9FuUNpl60Gmntcop4= github.com/matrix-org/gomatrix v0.0.0-20210324163249-be2af5ef2e16/go.mod h1:/gBX06Kw0exX1HrwmoBibFA98yBk/jxKpGVeyQbff+s= -github.com/matrix-org/gomatrixserverlib v0.0.0-20210805104751-03e40fa2c0e1 h1:ExS9AmOWYSZ4DR7W7Eqm7glP/g/u/CTVLAu9blAnve4= -github.com/matrix-org/gomatrixserverlib v0.0.0-20210805104751-03e40fa2c0e1/go.mod h1:JsAzE1Ll3+gDWS9JSUHPJiiyAksvOOnGWF2nXdg4ZzU= +github.com/matrix-org/gomatrixserverlib v0.0.0-20210809130922-d9c3f400582b h1:8St1B8QmlvMLsOmGqW3++0akUs0250IAi+AGcr5faxw= +github.com/matrix-org/gomatrixserverlib v0.0.0-20210809130922-d9c3f400582b/go.mod h1:JsAzE1Ll3+gDWS9JSUHPJiiyAksvOOnGWF2nXdg4ZzU= github.com/matrix-org/naffka v0.0.0-20210623111924-14ff508b58e0 h1:HZCzy4oVzz55e+cOMiX/JtSF2UOY1evBl2raaE7ACcU= github.com/matrix-org/naffka v0.0.0-20210623111924-14ff508b58e0/go.mod h1:sjyPyRxKM5uw1nD2cJ6O2OxI6GOqyVBfNXqKjBZTBZE= github.com/matrix-org/pinecone v0.0.0-20210623102758-74f885644c1b h1:5X5vdWQ13xrNkJVqaJHPsrt7rKkMJH5iac0EtfOuxSg= diff --git a/keyserver/api/api.go b/keyserver/api/api.go index 520562e7..aa6df96f 100644 --- a/keyserver/api/api.go +++ b/keyserver/api/api.go @@ -160,7 +160,7 @@ type PerformClaimKeysResponse struct { type PerformUploadDeviceKeysRequest struct { gomatrixserverlib.CrossSigningKeys // The user that uploaded the key, should be populated by the clientapi. - UserID string `json:"user_id"` + UserID string } type PerformUploadDeviceKeysResponse struct { @@ -170,7 +170,7 @@ type PerformUploadDeviceKeysResponse struct { type PerformUploadDeviceSignaturesRequest struct { Signatures map[string]map[gomatrixserverlib.KeyID]gomatrixserverlib.CrossSigningForKeyOrDevice // The user that uploaded the sig, should be populated by the clientapi. - UserID string `json:"user_id"` + UserID string } type PerformUploadDeviceSignaturesResponse struct { diff --git a/keyserver/internal/cross_signing.go b/keyserver/internal/cross_signing.go index 141a4eb2..4009dd45 100644 --- a/keyserver/internal/cross_signing.go +++ b/keyserver/internal/cross_signing.go @@ -15,6 +15,7 @@ package internal import ( + "bytes" "context" "crypto/ed25519" "database/sql" @@ -78,8 +79,8 @@ func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.P } return } - hasMasterKey = true for _, keyData := range req.MasterKey.Keys { // iterates once, because sanityCheckKey requires one key + hasMasterKey = true masterKey = keyData } } @@ -116,46 +117,11 @@ func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.P masterKey, hasMasterKey = existingKeys[gomatrixserverlib.CrossSigningKeyPurposeMaster] } - // If the user isn't a local user and we haven't successfully found a key - // through any local means then ask over federation. - if !hasMasterKey { - _, host, err := gomatrixserverlib.SplitID('@', req.UserID) - if err != nil { - res.Error = &api.KeyError{ - Err: "Retrieving cross-signing keys from federation failed: " + err.Error(), - } - return - } - keys, err := a.FedClient.QueryKeys(ctx, host, map[string][]string{ - req.UserID: {}, - }) - if err != nil { - res.Error = &api.KeyError{ - Err: "Retrieving cross-signing keys from federation failed: " + err.Error(), - } - return - } - switch k := keys.MasterKeys[req.UserID].CrossSigningBody.(type) { - case *gomatrixserverlib.CrossSigningKey: - if err := sanityCheckKey(*k, req.UserID, gomatrixserverlib.CrossSigningKeyPurposeMaster); err != nil { - res.Error = &api.KeyError{ - Err: "Master key sanity check failed: " + err.Error(), - } - return - } - default: - res.Error = &api.KeyError{ - Err: "Unexpected type for master key retrieved from federation", - } - return - } - } - // If we still don't have a master key at this point then there's nothing else // we can do - we've checked both the request and the database. if !hasMasterKey { res.Error = &api.KeyError{ - Err: "No master key was found, either in the database or in the request!", + Err: "No master key was found either in the database or in the request!", IsMissingParam: true, } return @@ -176,6 +142,15 @@ func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.P if len(req.UserSigningKey.Keys) > 0 { toVerify[gomatrixserverlib.CrossSigningKeyPurposeUserSigning] = req.UserSigningKey } + + if len(toVerify) == 0 { + res.Error = &api.KeyError{ + Err: "No supplied keys available for verification", + IsMissingParam: true, + } + return + } + for purpose, key := range toVerify { // Collect together the key IDs we need to verify with. This will include // all of the key IDs specified in the signatures. @@ -212,6 +187,14 @@ func (a *KeyInternalAPI) PerformUploadDeviceKeys(ctx context.Context, req *api.P } } + if len(toStore) == 0 { + res.Error = &api.KeyError{ + Err: "No supplied keys passed verification", + IsMissingParam: true, + } + return + } + if err := a.DB.StoreCrossSigningKeysForUser(ctx, req.UserID, toStore); err != nil { res.Error = &api.KeyError{ Err: fmt.Sprintf("a.DB.StoreCrossSigningKeysForUser: %s", err), @@ -257,6 +240,8 @@ func (a *KeyInternalAPI) PerformUploadDeviceSignatures(ctx context.Context, req selfSignatures := map[string]map[gomatrixserverlib.KeyID]gomatrixserverlib.CrossSigningForKeyOrDevice{} otherSignatures := map[string]map[gomatrixserverlib.KeyID]gomatrixserverlib.CrossSigningForKeyOrDevice{} + // Sort signatures into two groups: one where people have signed their own + // keys and one where people have signed someone elses for userID, forUserID := range req.Signatures { for keyID, keyOrDevice := range forUserID { switch key := keyOrDevice.CrossSigningBody.(type) { @@ -335,20 +320,18 @@ func (a *KeyInternalAPI) processSelfSignatures( } for originKeyID, originSig := range forOriginUserID { - originDeviceKeyID := gomatrixserverlib.KeyID("ed25519:" + originKeyID) - var originKey gomatrixserverlib.DeviceKeys if err := json.Unmarshal(originDeviceKeys[string(originKeyID)], &originKey); err != nil { return fmt.Errorf("json.Unmarshal: %w", err) } - originSigningKey, ok := originKey.Keys[originDeviceKeyID] + originSigningKey, ok := originKey.Keys[originKeyID] if !ok { - return fmt.Errorf("missing origin signing key %q", originDeviceKeyID) + return fmt.Errorf("missing origin signing key %q", originKeyID) } originSigningKeyPublic := ed25519.PublicKey(originSigningKey) - if err := gomatrixserverlib.VerifyJSON(originUserID, originDeviceKeyID, originSigningKeyPublic, j); err != nil { + if err := gomatrixserverlib.VerifyJSON(originUserID, originKeyID, originSigningKeyPublic, j); err != nil { return fmt.Errorf("gomatrixserverlib.VerifyJSON: %w", err) } @@ -414,6 +397,56 @@ func (a *KeyInternalAPI) processOtherSignatures( // Here we will process: // * A user signing someone else's master keys using their user-signing keys + for targetUserID, forTargetUserID := range signatures { + for _, signature := range forTargetUserID { + switch sig := signature.CrossSigningBody.(type) { + case *gomatrixserverlib.CrossSigningKey: + // Find the local copy of the master key. We'll use this to be + // sure that the supplied stanza matches the key that we think it + // should be. + masterKey, ok := queryRes.MasterKeys[targetUserID] + if !ok { + return fmt.Errorf("failed to find master key for user %q", targetUserID) + } + + // For each key ID, write the signatures. Maybe there'll be more + // than one algorithm in the future so it's best not to focus on + // everything being ed25519:. + for targetKeyID, suppliedKeyData := range sig.Keys { + // The master key will be supplied in the request, but we should + // make sure that it matches what we think the master key should + // actually be. + localKeyData, lok := masterKey.Keys[targetKeyID] + if !lok { + return fmt.Errorf("uploaded master key %q for user %q doesn't match local copy", targetKeyID, targetUserID) + } else if !bytes.Equal(suppliedKeyData, localKeyData) { + return fmt.Errorf("uploaded master key %q for user %q doesn't match local copy", targetKeyID, targetUserID) + } + + // We only care about the signatures from the uploading user, so + // we will ignore anything that didn't originate from them. + userSigs, ok := sig.Signatures[userID] + if !ok { + return fmt.Errorf("there are no signatures on master key %q from uploading user %q", targetKeyID, userID) + } + + for originKeyID, originSig := range userSigs { + if err := a.DB.StoreCrossSigningSigsForTarget( + ctx, userID, originKeyID, targetUserID, targetKeyID, originSig, + ); err != nil { + return fmt.Errorf("a.DB.StoreCrossSigningKeysForTarget: %w", err) + } + } + } + + default: + // Users should only be signing another person's master key, + // so if we're here, it's probably because it's actually a + // gomatrixserverlib.DeviceKeys, which doesn't make sense. + } + } + } + return nil } @@ -435,7 +468,7 @@ func (a *KeyInternalAPI) crossSigningKeysFromDatabase( } sigMap, err := a.DB.CrossSigningSigsForTarget(ctx, userID, keyID) - if err != nil { + if err != nil && err != sql.ErrNoRows { logrus.WithError(err).Errorf("Failed to get cross-signing signatures for user %q key %q", userID, keyID) continue } @@ -480,44 +513,39 @@ func (a *KeyInternalAPI) crossSigningKeysFromDatabase( func (a *KeyInternalAPI) QuerySignatures(ctx context.Context, req *api.QuerySignaturesRequest, res *api.QuerySignaturesResponse) { for targetUserID, forTargetUser := range req.TargetIDs { + keyMap, err := a.DB.CrossSigningKeysForUser(ctx, targetUserID) + if err != nil && err != sql.ErrNoRows { + res.Error = &api.KeyError{ + Err: fmt.Sprintf("a.DB.CrossSigningKeysForUser: %s", err), + } + continue + } + + for targetPurpose, targetKey := range keyMap { + switch targetPurpose { + case gomatrixserverlib.CrossSigningKeyPurposeMaster: + if res.MasterKeys == nil { + res.MasterKeys = map[string]gomatrixserverlib.CrossSigningKey{} + } + res.MasterKeys[targetUserID] = targetKey + + case gomatrixserverlib.CrossSigningKeyPurposeSelfSigning: + if res.SelfSigningKeys == nil { + res.SelfSigningKeys = map[string]gomatrixserverlib.CrossSigningKey{} + } + res.SelfSigningKeys[targetUserID] = targetKey + + case gomatrixserverlib.CrossSigningKeyPurposeUserSigning: + if res.UserSigningKeys == nil { + res.UserSigningKeys = map[string]gomatrixserverlib.CrossSigningKey{} + } + res.UserSigningKeys[targetUserID] = targetKey + } + } + for _, targetKeyID := range forTargetUser { - keyMap, err := a.DB.CrossSigningKeysForUser(ctx, targetUserID) - if err != nil { - if err == sql.ErrNoRows { - continue - } - res.Error = &api.KeyError{ - Err: fmt.Sprintf("a.DB.CrossSigningKeysForUser: %s", err), - } - } - - for targetPurpose, targetKey := range keyMap { - switch targetPurpose { - case gomatrixserverlib.CrossSigningKeyPurposeMaster: - if res.MasterKeys == nil { - res.MasterKeys = map[string]gomatrixserverlib.CrossSigningKey{} - } - res.MasterKeys[targetUserID] = targetKey - - case gomatrixserverlib.CrossSigningKeyPurposeSelfSigning: - if res.SelfSigningKeys == nil { - res.SelfSigningKeys = map[string]gomatrixserverlib.CrossSigningKey{} - } - res.SelfSigningKeys[targetUserID] = targetKey - - case gomatrixserverlib.CrossSigningKeyPurposeUserSigning: - if res.UserSigningKeys == nil { - res.UserSigningKeys = map[string]gomatrixserverlib.CrossSigningKey{} - } - res.UserSigningKeys[targetUserID] = targetKey - } - } - sigMap, err := a.DB.CrossSigningSigsForTarget(ctx, targetUserID, targetKeyID) - if err != nil { - if err == sql.ErrNoRows { - continue - } + if err != nil && err != sql.ErrNoRows { res.Error = &api.KeyError{ Err: fmt.Sprintf("a.DB.CrossSigningSigsForTarget: %s", err), } diff --git a/keyserver/internal/internal.go b/keyserver/internal/internal.go index 28638c29..de269911 100644 --- a/keyserver/internal/internal.go +++ b/keyserver/internal/internal.go @@ -372,9 +372,15 @@ func (a *KeyInternalAPI) queryRemoteKeys( domains := map[string]struct{}{} for domain := range domainToDeviceKeys { + if domain == string(a.ThisServer) { + continue + } domains[domain] = struct{}{} } for domain := range domainToCrossSigningKeys { + if domain == string(a.ThisServer) { + continue + } domains[domain] = struct{}{} } wg.Add(len(domains)) @@ -406,17 +412,11 @@ func (a *KeyInternalAPI) queryRemoteKeys( } for userID, body := range result.MasterKeys { - switch b := body.CrossSigningBody.(type) { - case *gomatrixserverlib.CrossSigningKey: - res.MasterKeys[userID] = *b - } + res.MasterKeys[userID] = body } for userID, body := range result.SelfSigningKeys { - switch b := body.CrossSigningBody.(type) { - case *gomatrixserverlib.CrossSigningKey: - res.SelfSigningKeys[userID] = *b - } + res.SelfSigningKeys[userID] = body } // TODO: do we want to persist these somewhere now @@ -430,8 +430,12 @@ func (a *KeyInternalAPI) queryRemoteKeysOnServer( res *api.QueryKeysResponse, ) { defer wg.Done() - fedCtx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() + fedCtx := ctx + if timeout > 0 { + var cancel context.CancelFunc + fedCtx, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } // for users who we do not have any knowledge about, try to start doing device list updates for them // by hitting /users/devices - otherwise fallback to /keys/query which has nicer bulk properties but // lack a stream ID. diff --git a/keyserver/inthttp/server.go b/keyserver/inthttp/server.go index ac70e3e5..475544a5 100644 --- a/keyserver/inthttp/server.go +++ b/keyserver/inthttp/server.go @@ -62,7 +62,7 @@ func AddRoutes(internalAPIMux *mux.Router, s api.KeyInternalAPI) { httputil.MakeInternalAPI("performUploadDeviceKeys", func(req *http.Request) util.JSONResponse { request := api.PerformUploadDeviceKeysRequest{} response := api.PerformUploadDeviceKeysResponse{} - if err := json.NewDecoder(req.Body).Decode(&request.CrossSigningKeys); err != nil { + if err := json.NewDecoder(req.Body).Decode(&request); err != nil { return util.MessageResponse(http.StatusBadRequest, err.Error()) } s.PerformUploadDeviceKeys(req.Context(), &request, &response) @@ -73,7 +73,7 @@ func AddRoutes(internalAPIMux *mux.Router, s api.KeyInternalAPI) { httputil.MakeInternalAPI("performUploadDeviceSignatures", func(req *http.Request) util.JSONResponse { request := api.PerformUploadDeviceSignaturesRequest{} response := api.PerformUploadDeviceSignaturesResponse{} - if err := json.NewDecoder(req.Body).Decode(&request.Signatures); err != nil { + if err := json.NewDecoder(req.Body).Decode(&request); err != nil { return util.MessageResponse(http.StatusBadRequest, err.Error()) } s.PerformUploadDeviceSignatures(req.Context(), &request, &response) diff --git a/sytest-whitelist b/sytest-whitelist index 27109e60..d2f2a1c7 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -553,3 +553,4 @@ Deleted & recreated backups are empty Can upload self-signing keys Fails to upload self-signing keys with no auth Fails to upload self-signing key without master key +can fetch self-signing keys over federation