From 91c53bbd2128378bf7fb31e83d5160f6000a37f3 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Thu, 10 Sep 2020 13:54:27 -0400 Subject: [PATCH] Switch to APIKey+IP rate limiting, HMAC values --- cmd/adminapi/main.go | 4 +- cmd/apiserver/main.go | 4 +- cmd/server/main.go | 4 +- docs/development.md | 7 +++ docs/production.md | 25 +++++++++ pkg/ratelimit/config.go | 5 ++ pkg/ratelimit/limitware/middleware.go | 81 ++++++++++++--------------- terraform/redis.tf | 22 ++++++++ terraform/service_admin_apiserver.tf | 6 ++ terraform/service_apiserver.tf | 6 ++ terraform/service_cleanup.tf | 6 ++ terraform/service_e2e_runner.tf | 6 ++ terraform/service_server.tf | 6 ++ terraform/services.tf | 1 + 14 files changed, 133 insertions(+), 50 deletions(-) diff --git a/cmd/adminapi/main.go b/cmd/adminapi/main.go index 22a2b8036..679473c85 100644 --- a/cmd/adminapi/main.go +++ b/cmd/adminapi/main.go @@ -87,7 +87,7 @@ func realMain(ctx context.Context) error { // Setup cacher cacher, err := cache.CacherFor(ctx, &config.Cache, cache.MultiKeyFunc( cache.HMACKeyFunc(sha1.New, config.Cache.HMACKey), - cache.PrefixKeyFunc("adminapi:"), + cache.PrefixKeyFunc("adminapi:cache:"), )) if err != nil { return fmt.Errorf("failed to create cacher: %w", err) @@ -115,7 +115,7 @@ func realMain(ctx context.Context) error { defer limiterStore.Close() httplimiter, err := limitware.NewMiddleware(ctx, limiterStore, - limitware.APIKeyFunc(ctx, "adminapi", db), + limitware.APIKeyFunc(ctx, "adminapi:ratelimit:", config.RateLimit.HMACKey), limitware.AllowOnError(false)) if err != nil { return fmt.Errorf("failed to create limiter middleware: %w", err) diff --git a/cmd/apiserver/main.go b/cmd/apiserver/main.go index 2837a1b56..b81a0c16e 100644 --- a/cmd/apiserver/main.go +++ b/cmd/apiserver/main.go @@ -90,7 +90,7 @@ func realMain(ctx context.Context) error { // Setup cacher cacher, err := cache.CacherFor(ctx, &config.Cache, cache.MultiKeyFunc( cache.HMACKeyFunc(sha1.New, config.Cache.HMACKey), - cache.PrefixKeyFunc("apiserver:"), + cache.PrefixKeyFunc("apiserver:cache:"), )) if err != nil { return fmt.Errorf("failed to create cacher: %w", err) @@ -128,7 +128,7 @@ func realMain(ctx context.Context) error { defer limiterStore.Close() httplimiter, err := limitware.NewMiddleware(ctx, limiterStore, - limitware.APIKeyFunc(ctx, "apiserver", db), + limitware.APIKeyFunc(ctx, "apiserver:ratelimit:", config.RateLimit.HMACKey), limitware.AllowOnError(false)) if err != nil { return fmt.Errorf("failed to create limiter middleware: %w", err) diff --git a/cmd/server/main.go b/cmd/server/main.go index 597f727c6..dcf57d7e9 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -106,7 +106,7 @@ func realMain(ctx context.Context) error { // Setup cacher cacher, err := cache.CacherFor(ctx, &config.Cache, cache.MultiKeyFunc( cache.HMACKeyFunc(sha1.New, config.Cache.HMACKey), - cache.PrefixKeyFunc("server:"), + cache.PrefixKeyFunc("server:cache:"), )) if err != nil { return fmt.Errorf("failed to create cacher: %w", err) @@ -161,7 +161,7 @@ func realMain(ctx context.Context) error { defer limiterStore.Close() httplimiter, err := limitware.NewMiddleware(ctx, limiterStore, - limitware.UserEmailKeyFunc(ctx, "server"), + limitware.UserIDKeyFunc(ctx, "server:ratelimit:", config.RateLimit.HMACKey), limitware.AllowOnError(false)) if err != nil { return fmt.Errorf("failed to create limiter middleware: %w", err) diff --git a/docs/development.md b/docs/development.md index 788afe073..2e537d491 100644 --- a/docs/development.md +++ b/docs/development.md @@ -98,6 +98,13 @@ represent best practices. export CACHE_TYPE="IN_MEMORY" export CACHE_HMAC_KEY="/wC2dki5Z+To9iFwUamINtHIMOH/dME7e5Gy+9h3WTDBhqeeSYkqduZRjcZWwG3kPMdiWAdBxxop5wB+BHTBnSlfVVmy8qKVNv+Wf5ywgxV7SbB8bjNQBHSpn7aC5RxR6nkEsZ2w2fUhTJwD9q+MDo6TQvf+8OXEPrV1SXWNHrs=" + # Configure rate limiter. Create your own values with: + # + # openssl rand -base64 128 + # + export RATE_LIMIT_TYPE="MEMORY" + export RATE_LIMIT_HMAC_KEY="/wC2dki5Z+To9iFwUamINtHIMOH/dME7e5Gy+9h3WTDBhqeeSYkqduZRjcZWwG3kPMdiWAdBxxop5wB+BHTBnSlfVVmy8qKVNv+Wf5ywgxV7SbB8bjNQBHSpn7aC5RxR6nkEsZ2w2fUhTJwD9q+MDo6TQvf+8OXEPrV1SXWNHrs=" + # Configure certificate key management. The CERTIFICATE_SIGNING_KEY should # be the value output in the previous step. export CERTIFICATE_KEY_MANAGER="FILESYSTEM" diff --git a/docs/production.md b/docs/production.md index f098b8540..35d895ce2 100644 --- a/docs/production.md +++ b/docs/production.md @@ -326,4 +326,29 @@ small and are automatically re-built on demand, so occasional rotation is likely fine for this system. +### Rate limit HMAC keys + +**Recommended frequency:** 90 days, on breach + +This key is used as the HMAC key to named values in the rate limit. For example, +API keys and IP addresses are rate limited. We do not want the rate limiter to +have those values in plaintext, so the values are HMACed before being written +(and HMACed on lookup). This prevents a server operator with access to the rate +limiter (e.g. Redis) from seeing plaintext data about the system. The data is +hashed instead of encrypted because we only need a deterministic value to +lookup. + +To generate a new key: + +```sh +openssl rand -base64 128 | tr -d "\n" +``` + +Use this value as of the `RATE_LIMIT_HMAC_KEY` environment variable: + +```sh +RATE_LIMIT_HMAC_KEY="43+ViAkv7uHYKjsXhU468NGBZrtlJWtZqTORIiY8V6OMsLAZ+XmUF5He/wIhRlislnteTmChNi+BHveSgkxky81tpZSw45HKdK+XW3X5P7H6092I0u7H31C0NaInrxNxIRAbSw0NxSIKNbfKwucDu1Y36XjJC0pi0wlJHxkdGes=" +``` + + [gcp-kms]: https://cloud.google.com/kms diff --git a/pkg/ratelimit/config.go b/pkg/ratelimit/config.go index eecf4db93..482bba360 100644 --- a/pkg/ratelimit/config.go +++ b/pkg/ratelimit/config.go @@ -22,6 +22,7 @@ import ( "github.com/google/exposure-notifications-verification-server/pkg/redis" redigo "github.com/opencensus-integrations/redigo/redis" + "github.com/sethvargo/go-envconfig" "github.com/sethvargo/go-limiter" "github.com/sethvargo/go-limiter/memorystore" "github.com/sethvargo/go-limiter/noopstore" @@ -45,6 +46,10 @@ type Config struct { Tokens uint64 `env:"RATE_LIMIT_TOKENS, default=60"` Interval time.Duration `env:"RATE_LIMIT_INTERVAL, default=1m"` + // HMACKey is the key to use when calculating the HMAC of keys before saving + // them in the rate limiter. + HMACKey envconfig.Base64Bytes `env:"RATE_LIMIT_HMAC_KEY, required"` + // Redis configuration Redis redis.Config `env:",prefix=RATE_LIMIT_"` } diff --git a/pkg/ratelimit/limitware/middleware.go b/pkg/ratelimit/limitware/middleware.go index 6c2d6205f..84adb0539 100644 --- a/pkg/ratelimit/limitware/middleware.go +++ b/pkg/ratelimit/limitware/middleware.go @@ -3,16 +3,15 @@ package limitware import ( "context" + "crypto/hmac" "crypto/sha1" "fmt" - "hash" "net/http" "strconv" "strings" "time" "github.com/google/exposure-notifications-verification-server/pkg/controller" - "github.com/google/exposure-notifications-verification-server/pkg/database" "github.com/google/exposure-notifications-verification-server/pkg/observability" "github.com/google/exposure-notifications-server/pkg/logging" @@ -197,60 +196,46 @@ func (m *Middleware) Handle(next http.Handler) http.Handler { } // APIKeyFunc returns a default key function for ratelimiting on our API key -// header. It falls back to rate limiting by the client ip. -func APIKeyFunc(ctx context.Context, scope string, db *database.Database) httplimit.KeyFunc { +// header. Since APIKeys are assumed to be "public" at some point, they are +// rate limited by [api-key,ip]. +func APIKeyFunc(ctx context.Context, scope string, hmacKey []byte) httplimit.KeyFunc { logger := logging.FromContext(ctx).Named(scope + ".ratelimit") - ipAddrLimit := IPAddressKeyFunc(ctx, scope) + ipAddrLimit := IPAddressKeyFunc(ctx, scope, hmacKey) return func(r *http.Request) (string, error) { // Procss the API key v := r.Header.Get("x-api-key") if v != "" { - // v2 API keys encode the realm - _, realmID, err := db.VerifyAPIKeySignature(v) - if err == nil { - logger.Debugw("limiting by api key v2 realm", "realm", realmID) - dig, err := digest(sha1.New, strconv.FormatUint(uint64(realmID), 10)) - if err != nil { - return "", fmt.Errorf("failed to digest realm id: %w", err) - } - return fmt.Sprintf("ratelimit:%s:realm:%x", scope, dig), nil - } - - // v1 API keys do not, fallback to the database - app, err := db.FindAuthorizedAppByAPIKey(v) - if err == nil { - logger.Debugw("limiting by api key v1 realm", "realm", app.RealmID) - dig, err := digest(sha1.New, strconv.FormatUint(uint64(app.RealmID), 10)) - if err != nil { - return "", fmt.Errorf("failed to digest realm id: %w", err) - } - return fmt.Sprintf("ratelimit:%s:realm:%x", scope, dig), nil + logger.Debugw("limiting by apikey") + dig, err := digest(fmt.Sprintf("%s:%s", v, remoteIP(r)), hmacKey) + if err != nil { + return "", fmt.Errorf("failed to digest api key: %w", err) } + return fmt.Sprintf("%sapikey:%s", scope, dig), nil } return ipAddrLimit(r) } } -// UserEmailKeyFunc pulls the user out of the request context and uses that to +// UserIDKeyFunc pulls the user out of the request context and uses that to // ratelimit. It falls back to rate limiting by the client ip. -func UserEmailKeyFunc(ctx context.Context, scope string) httplimit.KeyFunc { +func UserIDKeyFunc(ctx context.Context, scope string, hmacKey []byte) httplimit.KeyFunc { logger := logging.FromContext(ctx).Named(scope + ".ratelimit") - ipAddrLimit := IPAddressKeyFunc(ctx, scope) + ipAddrLimit := IPAddressKeyFunc(ctx, scope, hmacKey) return func(r *http.Request) (string, error) { ctx := r.Context() // See if a user exists on the context user := controller.UserFromContext(ctx) - if user != nil && user.Email != "" { + if user != nil { logger.Debugw("limiting by user", "user", user.ID) - dig, err := digest(sha1.New, strconv.FormatUint(uint64(user.ID), 10)) + dig, err := digest(fmt.Sprintf("%d", user.ID), hmacKey) if err != nil { return "", fmt.Errorf("failed to digest user id: %w", err) } - return fmt.Sprintf("ratelimit:%s:user:%x", scope, dig), nil + return fmt.Sprintf("%suser:%x", scope, dig), nil } return ipAddrLimit(r) @@ -258,33 +243,41 @@ func UserEmailKeyFunc(ctx context.Context, scope string) httplimit.KeyFunc { } // IPAddressKeyFunc uses the client IP to rate limit. -func IPAddressKeyFunc(ctx context.Context, scope string) httplimit.KeyFunc { +func IPAddressKeyFunc(ctx context.Context, scope string, hmacKey []byte) httplimit.KeyFunc { logger := logging.FromContext(ctx).Named(scope + ".ratelimit") return func(r *http.Request) (string, error) { // Get the remote addr - ip := r.RemoteAddr - - // Check if x-forwarded-for exists, the load balancer sets this, and the - // first entry is the real client IP - xff := r.Header.Get("x-forwarded-for") - if xff != "" { - ip = strings.Split(xff, ",")[0] - } + ip := remoteIP(r) logger.Debugw("limiting by ip", "ip", ip) - dig, err := digest(sha1.New, ip) + dig, err := digest(ip, hmacKey) if err != nil { return "", fmt.Errorf("failed to digest ip: %w", err) } - return fmt.Sprintf("ratelimit:%s:ip:%x", scope, dig), nil + return fmt.Sprintf("%sip:%x", scope, dig), nil } } +// remoteIP returns the "real" remote IP. +func remoteIP(r *http.Request) string { + // Get the remote addr + ip := r.RemoteAddr + + // Check if x-forwarded-for exists, the load balancer sets this, and the + // first entry is the real client IP + xff := r.Header.Get("x-forwarded-for") + if xff != "" { + ip = strings.Split(xff, ",")[0] + } + + return ip +} + // digest returns the digest of given string as a hex-encoded string, and any // errors that occur while hashing. -func digest(hasher func() hash.Hash, in string) (string, error) { - h := hasher() +func digest(in string, key []byte) (string, error) { + h := hmac.New(sha1.New, key) n, err := h.Write([]byte(in)) if err != nil { return "", err diff --git a/terraform/redis.tf b/terraform/redis.tf index 85f65269c..f6c74aef1 100644 --- a/terraform/redis.tf +++ b/terraform/redis.tf @@ -52,6 +52,28 @@ resource "google_secret_manager_secret_version" "cache-hmac-key" { secret_data = random_id.cache-hmac-key.b64_std } +# Create secret for the HMAC ratelimit keys +resource "random_id" "ratelimit-hmac-key" { + byte_length = 128 +} + +resource "google_secret_manager_secret" "ratelimit-hmac-key" { + secret_id = "ratelimit-hmac-key" + + replication { + automatic = true + } + + depends_on = [ + google_project_service.services["secretmanager.googleapis.com"], + ] +} + +resource "google_secret_manager_secret_version" "ratelimit-hmac-key" { + secret = google_secret_manager_secret.ratelimit-hmac-key.id + secret_data = random_id.ratelimit-hmac-key.b64_std +} + output "redis_host" { value = google_redis_instance.cache.host } diff --git a/terraform/service_admin_apiserver.tf b/terraform/service_admin_apiserver.tf index 1f5ce947d..088300a81 100644 --- a/terraform/service_admin_apiserver.tf +++ b/terraform/service_admin_apiserver.tf @@ -85,6 +85,12 @@ resource "google_secret_manager_secret_iam_member" "adminapi-cache-hmac-key" { member = "serviceAccount:${google_service_account.adminapi.email}" } +resource "google_secret_manager_secret_iam_member" "adminapi-ratelimit-hmac-key" { + secret_id = google_secret_manager_secret.ratelimit-hmac-key.id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.adminapi.email}" +} + resource "google_cloud_run_service" "adminapi" { name = "adminapi" location = var.region diff --git a/terraform/service_apiserver.tf b/terraform/service_apiserver.tf index e0b405c20..7d4b55b76 100644 --- a/terraform/service_apiserver.tf +++ b/terraform/service_apiserver.tf @@ -91,6 +91,12 @@ resource "google_secret_manager_secret_iam_member" "apiserver-cache-hmac-key" { member = "serviceAccount:${google_service_account.apiserver.email}" } +resource "google_secret_manager_secret_iam_member" "apiserver-ratelimit-hmac-key" { + secret_id = google_secret_manager_secret.ratelimit-hmac-key.id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.apiserver.email}" +} + resource "google_cloud_run_service" "apiserver" { name = "apiserver" location = var.region diff --git a/terraform/service_cleanup.tf b/terraform/service_cleanup.tf index 2be4bfca2..2f568f8a5 100644 --- a/terraform/service_cleanup.tf +++ b/terraform/service_cleanup.tf @@ -85,6 +85,12 @@ resource "google_secret_manager_secret_iam_member" "cleanup-cache-hmac-key" { member = "serviceAccount:${google_service_account.cleanup.email}" } +resource "google_secret_manager_secret_iam_member" "cleanup-ratelimit-hmac-key" { + secret_id = google_secret_manager_secret.ratelimit-hmac-key.id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.cleanup.email}" +} + resource "google_cloud_run_service" "cleanup" { name = "cleanup" location = var.region diff --git a/terraform/service_e2e_runner.tf b/terraform/service_e2e_runner.tf index dc490a92c..83cffafe2 100644 --- a/terraform/service_e2e_runner.tf +++ b/terraform/service_e2e_runner.tf @@ -85,6 +85,12 @@ resource "google_secret_manager_secret_iam_member" "e2e-runner-cache-hmac-key" { member = "serviceAccount:${google_service_account.e2e-runner.email}" } +resource "google_secret_manager_secret_iam_member" "e2e-runner-ratelimit-hmac-key" { + secret_id = google_secret_manager_secret.ratelimit-hmac-key.id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.e2e-runner.email}" +} + resource "google_cloud_run_service" "e2e-runner" { name = "e2e-runner" location = var.region diff --git a/terraform/service_server.tf b/terraform/service_server.tf index e5792232c..aa3c7a7f5 100644 --- a/terraform/service_server.tf +++ b/terraform/service_server.tf @@ -122,6 +122,12 @@ resource "google_secret_manager_secret_iam_member" "server-cache-hmac-key" { member = "serviceAccount:${google_service_account.server.email}" } +resource "google_secret_manager_secret_iam_member" "server-ratelimit-hmac-key" { + secret_id = google_secret_manager_secret.ratelimit-hmac-key.id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.server.email}" +} + resource "google_cloud_run_service" "server" { name = "server" location = var.region diff --git a/terraform/services.tf b/terraform/services.tf index 5e432951d..aa5190e41 100644 --- a/terraform/services.tf +++ b/terraform/services.tf @@ -60,6 +60,7 @@ locals { } rate_limit_config = { + RATE_LIMIT_HMAC_KEY = "secret://${google_secret_manager_secret_version.ratelimit-hmac-key.id}" RATE_LIMIT_TYPE = "REDIS" RATE_LIMIT_TOKENS = "60" RATE_LIMIT_INTERVAL = "1m"