Tweak AS registration check and AS component HTTP clients (#1785)

* Tweak AS registration check

* Check appservice usernames using correct function

* Update sytest-whitelist

* Use gomatrixserverlib.Client since that allows us to disable TLS validation using the config

* Add appservice-specific client and ability to control TLS validation for appservices only

* Set timeout on appservice client

* Review comments

* Remove dead code

* Enforce LoginTypeApplicationService after all

* Check correct auth type field

9 files changed, 64 insertions, 47 deletions

diff --git a/appservice/appservice.go b/appservice/appservice.go
index d783c7eb..f608e8e7 100644
--- a/appservice/appservice.go
+++ b/appservice/appservice.go
@@ -16,9 +16,7 @@ package appservice
 
 import (
 	"context"
-	"net/http"
 	"sync"
-	"time"
 
 	"github.com/gorilla/mux"
 	appserviceAPI "github.com/matrix-org/dendrite/appservice/api"
@@ -48,6 +46,7 @@ func NewInternalAPI(
 	userAPI userapi.UserInternalAPI,
 	rsAPI roomserverAPI.RoomserverInternalAPI,
 ) appserviceAPI.AppServiceQueryAPI {
+	client := base.CreateAppserviceClient()
 	consumer, _ := kafka.SetupConsumerProducer(&base.Cfg.Global.Kafka)
 
 	// Create a connection to the appservice postgres DB
@@ -79,10 +78,8 @@ func NewInternalAPI(
 	// Create appserivce query API with an HTTP client that will be used for all
 	// outbound and inbound requests (inbound only for the internal API)
 	appserviceQueryAPI := &query.AppServiceQueryAPI{
-		HTTPClient: &http.Client{
-			Timeout: time.Second * 30,
-		},
-		Cfg: base.Cfg,
+		HTTPClient: client,
+		Cfg:        base.Cfg,
 	}
 
 	// Only consume if we actually have ASes to track, else we'll just chew cycles needlessly.
@@ -98,7 +95,7 @@ func NewInternalAPI(
 	}
 
 	// Create application service transaction workers
-	if err := workers.SetupTransactionWorkers(appserviceDB, workerStates); err != nil {
+	if err := workers.SetupTransactionWorkers(client, appserviceDB, workerStates); err != nil {
 		logrus.WithError(err).Panicf("failed to start app service transaction workers")
 	}
 	return appserviceQueryAPI
diff --git a/appservice/query/query.go b/appservice/query/query.go
index 7e5ac475..b4c33528 100644
--- a/appservice/query/query.go
+++ b/appservice/query/query.go
@@ -20,10 +20,10 @@ import (
 	"context"
 	"net/http"
 	"net/url"
-	"time"
 
 	"github.com/matrix-org/dendrite/appservice/api"
 	"github.com/matrix-org/dendrite/setup/config"
+	"github.com/matrix-org/gomatrixserverlib"
 	opentracing "github.com/opentracing/opentracing-go"
 	log "github.com/sirupsen/logrus"
 )
@@ -33,7 +33,7 @@ const userIDExistsPath = "/users/"
 
 // AppServiceQueryAPI is an implementation of api.AppServiceQueryAPI
 type AppServiceQueryAPI struct {
-	HTTPClient *http.Client
+	HTTPClient *gomatrixserverlib.Client
 	Cfg        *config.Dendrite
 }
 
@@ -47,11 +47,6 @@ func (a *AppServiceQueryAPI) RoomAliasExists(
 	span, ctx := opentracing.StartSpanFromContext(ctx, "ApplicationServiceRoomAlias")
 	defer span.Finish()
 
-	// Create an HTTP client if one does not already exist
-	if a.HTTPClient == nil {
-		a.HTTPClient = makeHTTPClient()
-	}
-
 	// Determine which application service should handle this request
 	for _, appservice := range a.Cfg.Derived.ApplicationServices {
 		if appservice.URL != "" && appservice.IsInterestedInRoomAlias(request.Alias) {
@@ -68,7 +63,7 @@ func (a *AppServiceQueryAPI) RoomAliasExists(
 			}
 			req = req.WithContext(ctx)
 
-			resp, err := a.HTTPClient.Do(req)
+			resp, err := a.HTTPClient.DoHTTPRequest(ctx, req)
 			if resp != nil {
 				defer func() {
 					err = resp.Body.Close()
@@ -115,11 +110,6 @@ func (a *AppServiceQueryAPI) UserIDExists(
 	span, ctx := opentracing.StartSpanFromContext(ctx, "ApplicationServiceUserID")
 	defer span.Finish()
 
-	// Create an HTTP client if one does not already exist
-	if a.HTTPClient == nil {
-		a.HTTPClient = makeHTTPClient()
-	}
-
 	// Determine which application service should handle this request
 	for _, appservice := range a.Cfg.Derived.ApplicationServices {
 		if appservice.URL != "" && appservice.IsInterestedInUserID(request.UserID) {
@@ -134,7 +124,7 @@ func (a *AppServiceQueryAPI) UserIDExists(
 			if err != nil {
 				return err
 			}
-			resp, err := a.HTTPClient.Do(req.WithContext(ctx))
+			resp, err := a.HTTPClient.DoHTTPRequest(ctx, req)
 			if resp != nil {
 				defer func() {
 					err = resp.Body.Close()
@@ -169,10 +159,3 @@ func (a *AppServiceQueryAPI) UserIDExists(
 	response.UserIDExists = false
 	return nil
 }
-
-// makeHTTPClient creates an HTTP client with certain options that will be used for all query requests to application services
-func makeHTTPClient() *http.Client {
-	return &http.Client{
-		Timeout: time.Second * 30,
-	}
-}
diff --git a/appservice/workers/transaction_scheduler.go b/appservice/workers/transaction_scheduler.go
index 45748c21..47d447c2 100644
--- a/appservice/workers/transaction_scheduler.go
+++ b/appservice/workers/transaction_scheduler.go
@@ -34,8 +34,6 @@ import (
 var (
 	// Maximum size of events sent in each transaction.
 	transactionBatchSize = 50
-	// Timeout for sending a single transaction to an application service.
-	transactionTimeout = time.Second * 60
 )
 
 // SetupTransactionWorkers spawns a separate goroutine for each application
@@ -44,6 +42,7 @@ var (
 // size), then send that off to the AS's /transactions/{txnID} endpoint. It also
 // handles exponentially backing off in case the AS isn't currently available.
 func SetupTransactionWorkers(
+	client *gomatrixserverlib.Client,
 	appserviceDB storage.Database,
 	workerStates []types.ApplicationServiceWorkerState,
 ) error {
@@ -51,7 +50,7 @@ func SetupTransactionWorkers(
 	for _, workerState := range workerStates {
 		// Don't create a worker if this AS doesn't want to receive events
 		if workerState.AppService.URL != "" {
-			go worker(appserviceDB, workerState)
+			go worker(client, appserviceDB, workerState)
 		}
 	}
 	return nil
@@ -59,17 +58,12 @@ func SetupTransactionWorkers(
 
 // worker is a goroutine that sends any queued events to the application service
 // it is given.
-func worker(db storage.Database, ws types.ApplicationServiceWorkerState) {
+func worker(client *gomatrixserverlib.Client, db storage.Database, ws types.ApplicationServiceWorkerState) {
 	log.WithFields(log.Fields{
 		"appservice": ws.AppService.ID,
 	}).Info("Starting application service")
 	ctx := context.Background()
 
-	// Create a HTTP client for sending requests to app services
-	client := &http.Client{
-		Timeout: transactionTimeout,
-	}
-
 	// Initial check for any leftover events to send from last time
 	eventCount, err := db.CountEventsWithAppServiceID(ctx, ws.AppService.ID)
 	if err != nil {
@@ -206,7 +200,7 @@ func createTransaction(
 // send sends events to an application service. Returns an error if an OK was not
 // received back from the application service or the request timed out.
 func send(
-	client *http.Client,
+	client *gomatrixserverlib.Client,
 	appservice config.ApplicationService,
 	txnID int,
 	transaction []byte,
@@ -219,7 +213,7 @@ func send(
 		return err
 	}
 	req.Header.Set("Content-Type", "application/json")
-	resp, err := client.Do(req)
+	resp, err := client.DoHTTPRequest(context.TODO(), req)
 	if err != nil {
 		return err
 	}
diff --git a/clientapi/routing/register.go b/clientapi/routing/register.go
index 614e19d5..7d5ddbea 100644
--- a/clientapi/routing/register.go
+++ b/clientapi/routing/register.go
@@ -496,11 +496,20 @@ func Register(
 		r.Username = strconv.FormatInt(id, 10)
 	}
 
+	// Is this an appservice registration? It will be if the access
+	// token is supplied
+	accessToken, accessTokenErr := auth.ExtractAccessToken(req)
+
 	// Squash username to all lowercase letters
 	r.Username = strings.ToLower(r.Username)
-
-	if resErr = validateUsername(r.Username); resErr != nil {
-		return *resErr
+	if r.Type == authtypes.LoginTypeApplicationService && accessTokenErr == nil {
+		if resErr = validateApplicationServiceUsername(r.Username); resErr != nil {
+			return *resErr
+		}
+	} else {
+		if resErr = validateUsername(r.Username); resErr != nil {
+			return *resErr
+		}
 	}
 	if resErr = validatePassword(r.Password); resErr != nil {
 		return *resErr
@@ -513,7 +522,7 @@ func Register(
 		"session_id": r.Auth.Session,
 	}).Info("Processing registration request")
 
-	return handleRegistrationFlow(req, r, sessionID, cfg, userAPI)
+	return handleRegistrationFlow(req, r, sessionID, cfg, userAPI, accessToken, accessTokenErr)
 }
 
 func handleGuestRegistration(
@@ -579,6 +588,8 @@ func handleRegistrationFlow(
 	sessionID string,
 	cfg *config.ClientAPI,
 	userAPI userapi.UserInternalAPI,
+	accessToken string,
+	accessTokenErr error,
 ) util.JSONResponse {
 	// TODO: Shared secret registration (create new user scripts)
 	// TODO: Enable registration config flag
@@ -588,12 +599,12 @@ func handleRegistrationFlow(
 	// TODO: Handle mapping registrationRequest parameters into session parameters
 
 	// TODO: email / msisdn auth types.
-	accessToken, accessTokenErr := auth.ExtractAccessToken(req)
 
 	// Appservices are special and are not affected by disabled
-	// registration or user exclusivity.
-	if r.Auth.Type == authtypes.LoginTypeApplicationService ||
-		(r.Auth.Type == "" && accessTokenErr == nil) {
+	// registration or user exclusivity. We'll go onto the appservice
+	// registration flow if a valid access token was provided or if
+	// the login type specifically requests it.
+	if r.Type == authtypes.LoginTypeApplicationService && accessTokenErr == nil {
 		return handleApplicationServiceRegistration(
 			accessToken, accessTokenErr, req, r, cfg, userAPI,
 		)
diff --git a/cmd/generate-config/main.go b/cmd/generate-config/main.go
index fa0da10c..9ef0f0b4 100644
--- a/cmd/generate-config/main.go
+++ b/cmd/generate-config/main.go
@@ -61,6 +61,7 @@ func main() {
 	}
 
 	if *defaultsForCI {
+		cfg.AppServiceAPI.DisableTLSValidation = true
 		cfg.ClientAPI.RateLimiting.Enabled = false
 		cfg.FederationSender.DisableTLSValidation = true
 		cfg.MSCs.MSCs = []string{"msc2836", "msc2946", "msc2444", "msc2753"}
diff --git a/dendrite-config.yaml b/dendrite-config.yaml
index a3d1065d..bf604c9d 100644
--- a/dendrite-config.yaml
+++ b/dendrite-config.yaml
@@ -125,6 +125,11 @@ app_service_api:
     max_idle_conns: 2
     conn_max_lifetime: -1
 
+  # Disable the validation of TLS certificates of appservices. This is
+  # not recommended in production since it may allow appservice traffic
+  # to be sent to an unverified endpoint.
+  disable_tls_validation: false
+
   # Appservice configuration files to load into this homeserver.
   config_files: []
 
diff --git a/setup/base.go b/setup/base.go
index e9aa2a45..f8a45409 100644
--- a/setup/base.go
+++ b/setup/base.go
@@ -290,6 +290,22 @@ func (b *BaseDendrite) CreateClient() *gomatrixserverlib.Client {
 	return client
 }
 
+// CreateAppserviceClient creates a new client for application services.
+// It has a specific timeout and obeys TLS validation from the appservice
+// config rather than the federation config.
+func (b *BaseDendrite) CreateAppserviceClient() *gomatrixserverlib.Client {
+	opts := []gomatrixserverlib.ClientOption{
+		gomatrixserverlib.WithSkipVerify(b.Cfg.AppServiceAPI.DisableTLSValidation),
+		gomatrixserverlib.WithTimeout(time.Second * 60),
+	}
+	if b.Cfg.Global.DNSCache.Enabled {
+		opts = append(opts, gomatrixserverlib.WithDNSCache(b.DNSCache))
+	}
+	client := gomatrixserverlib.NewClient(opts...)
+	client.SetUserAgent(fmt.Sprintf("Dendrite/%s", internal.VersionString()))
+	return client
+}
+
 // CreateFederationClient creates a new federation client. Should only be called
 // once per component.
 func (b *BaseDendrite) CreateFederationClient() *gomatrixserverlib.FederationClient {
diff --git a/setup/config/config_appservice.go b/setup/config/config_appservice.go
index a042691d..a6f77abf 100644
--- a/setup/config/config_appservice.go
+++ b/setup/config/config_appservice.go
@@ -33,6 +33,10 @@ type AppServiceAPI struct {
 
 	Database DatabaseOptions `yaml:"database"`
 
+	// DisableTLSValidation disables the validation of X.509 TLS certs
+	// on appservice endpoints. This is not recommended in production!
+	DisableTLSValidation bool `yaml:"disable_tls_validation"`
+
 	ConfigFiles []string `yaml:"config_files"`
 }
 
diff --git a/sytest-whitelist b/sytest-whitelist
index 1e4442a0..7ccbaad1 100644
--- a/sytest-whitelist
+++ b/sytest-whitelist
@@ -510,3 +510,9 @@ Can pass a JSON filter as a query parameter
 Local room members can get room messages
 Remote room members can get room messages
 Guest users can send messages to guest_access rooms if joined
+AS can create a user
+AS can create a user with an underscore
+AS can create a user with inhibit_login
+AS can set avatar for ghosted users
+AS can set displayname for ghosted users
+Ghost user must register before joining room