From 38df2602e81a8b190abb100b6183452d2147eb72 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Tue, 6 Oct 2020 20:20:19 -0400 Subject: [PATCH 1/2] Handle edge case when realm quota is first enabled If realm quota is enabled and then someone "takes" from the bucket before the modeler runs, then the realm gets the default quota (which is low). Worse, it gets the default TTL (60s), which also isn't correct. --- .../realmadmin/_form_abuse_prevention.html | 14 +++++----- pkg/config/admin_server_config.go | 2 +- pkg/config/server_config.go | 2 +- pkg/controller/modeler/modeler.go | 6 ++++- pkg/controller/realmadmin/settings.go | 27 +++++++++++++++++++ 5 files changed, 42 insertions(+), 9 deletions(-) diff --git a/cmd/server/assets/realmadmin/_form_abuse_prevention.html b/cmd/server/assets/realmadmin/_form_abuse_prevention.html index 735b31f30..1a5c1bfdc 100644 --- a/cmd/server/assets/realmadmin/_form_abuse_prevention.html +++ b/cmd/server/assets/realmadmin/_form_abuse_prevention.html @@ -31,14 +31,16 @@
- {{if gt $quotaRemaining $quotaLimit}} - If your remaining quota exceeds the maximum quota, it means a realm + {{if gt $quotaRemaining $quotaLimit}} + +
+ {{end}}
diff --git a/pkg/config/admin_server_config.go b/pkg/config/admin_server_config.go index 051a4b5c7..8c8a54e54 100644 --- a/pkg/config/admin_server_config.go +++ b/pkg/config/admin_server_config.go @@ -48,7 +48,7 @@ type AdminAPIServerConfig struct { CollisionRetryCount uint `env:"COLLISION_RETRY_COUNT,default=6"` AllowedSymptomAge time.Duration `env:"ALLOWED_PAST_SYMPTOM_DAYS,default=336h"` // 336h is 14 days. - EnforceRealmQuotas bool `env:"ENFORCE_REALM_QUOTAS, default=false"` + EnforceRealmQuotas bool `env:"ENFORCE_REALM_QUOTAS, default=true"` // For EN Express, the link will be // https://[realm-region].[ENX_REDIRECT_DOMAIN]/v?c=[longcode] diff --git a/pkg/config/server_config.go b/pkg/config/server_config.go index 3622250c6..ed54f78e4 100644 --- a/pkg/config/server_config.go +++ b/pkg/config/server_config.go @@ -77,7 +77,7 @@ type ServerConfig struct { ServerName string `env:"SERVER_NAME,default=Diagnosis Verification Server"` CollisionRetryCount uint `env:"COLLISION_RETRY_COUNT,default=6"` AllowedSymptomAge time.Duration `env:"ALLOWED_PAST_SYMPTOM_DAYS,default=336h"` // 336h is 14 days. - EnforceRealmQuotas bool `env:"ENFORCE_REALM_QUOTAS, default=false"` + EnforceRealmQuotas bool `env:"ENFORCE_REALM_QUOTAS, default=true"` AssetsPath string `env:"ASSETS_PATH,default=./cmd/server/assets"` diff --git a/pkg/controller/modeler/modeler.go b/pkg/controller/modeler/modeler.go index 7cff3e361..26ef833e1 100644 --- a/pkg/controller/modeler/modeler.go +++ b/pkg/controller/modeler/modeler.go @@ -35,6 +35,10 @@ import ( "go.uber.org/zap" ) +const ( + oneWeek = 7 * 24 * time.Hour +) + // Controller is a controller for the modeler service. type Controller struct { config *config.Modeler @@ -211,7 +215,7 @@ func (c *Controller) rebuildModel(ctx context.Context, id uint64) error { return fmt.Errorf("failed to digest realm id: %w", err) } key := fmt.Sprintf("realm:quota:%s", dig) - if err := c.limiter.Set(ctx, key, uint64(effective), 24*time.Hour); err != nil { + if err := c.limiter.Set(ctx, key, uint64(effective), oneWeek); err != nil { return fmt.Errorf("failed to update limit: %w", err) } diff --git a/pkg/controller/realmadmin/settings.go b/pkg/controller/realmadmin/settings.go index 708348ccb..2e88fe523 100644 --- a/pkg/controller/realmadmin/settings.go +++ b/pkg/controller/realmadmin/settings.go @@ -199,11 +199,38 @@ func (c *Controller) HandleSettings() http.Handler { } // Abuse prevention + var abusePreventionJustEnabled bool if form.AbusePrevention { + abusePreventionJustEnabled = !realm.AbusePreventionEnabled && form.AbusePreventionEnabled + realm.AbusePreventionEnabled = form.AbusePreventionEnabled realm.AbusePreventionLimitFactor = form.AbusePreventionLimitFactor } + // If abuse prevention was just enabled, create the initial bucket so + // enforcement works. We do this before actually saving the configuration on + // the realm to avoid a race where someone is issuing a code where abuse + // prevention has been enabled, but the quota has not been set. In that + // case, the quota would be the "default" quota for the limiter, which is + // not ideal or correct. + // + // Even if saving the realm fails, there's no harm in doing this early. It's + // an idempotent operation that TTLs out after a week anyway. + if abusePreventionJustEnabled { + dig, err := digest.HMACUint(realm.ID, c.config.RateLimit.HMACKey) + if err != nil { + controller.InternalError(w, r, c.h, err) + return + } + key := fmt.Sprintf("realm:quota:%s", dig) + limit := uint64(realm.AbusePreventionEffectiveLimit()) + ttl := 7 * 24 * time.Hour + if err := c.limiter.Set(ctx, key, limit, ttl); err != nil { + controller.InternalError(w, r, c.h, err) + return + } + } + // Save realm if err := c.db.SaveRealm(realm, currentUser); err != nil { flash.Error("Failed to update realm: %v", err) From 552df6d017f992a9380090f4528e74111a66e728 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Tue, 6 Oct 2020 21:31:33 -0400 Subject: [PATCH 2/2] Oops --- pkg/controller/modeler/modeler.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/pkg/controller/modeler/modeler.go b/pkg/controller/modeler/modeler.go index 26ef833e1..7cff3e361 100644 --- a/pkg/controller/modeler/modeler.go +++ b/pkg/controller/modeler/modeler.go @@ -35,10 +35,6 @@ import ( "go.uber.org/zap" ) -const ( - oneWeek = 7 * 24 * time.Hour -) - // Controller is a controller for the modeler service. type Controller struct { config *config.Modeler @@ -215,7 +211,7 @@ func (c *Controller) rebuildModel(ctx context.Context, id uint64) error { return fmt.Errorf("failed to digest realm id: %w", err) } key := fmt.Sprintf("realm:quota:%s", dig) - if err := c.limiter.Set(ctx, key, uint64(effective), oneWeek); err != nil { + if err := c.limiter.Set(ctx, key, uint64(effective), 24*time.Hour); err != nil { return fmt.Errorf("failed to update limit: %w", err) }