diff --git a/upload-server/server/local.go b/upload-server/server/local.go index 7db2740f9..5534fb3b9 100644 --- a/upload-server/server/local.go +++ b/upload-server/server/local.go @@ -33,10 +33,13 @@ func configureLocalHandlers(mux *http.ServeMux, limiter *middleware.APIRateLimit if !ok { return fmt.Errorf("SERVER_URL environment variable is required") } - _, err := url.Parse(envURL) + parsedURL, err := url.Parse(envURL) if err != nil { return fmt.Errorf("SERVER_URL environment variable is invalid: %w", err) } + if parsedURL.Scheme != "https" { + return fmt.Errorf("SERVER_URL environment variable must use https, got %q", parsedURL.Scheme) + } dir := defaultDir envDir, ok := os.LookupEnv("STORE_DIR") diff --git a/upload-server/server/local_test.go b/upload-server/server/local_test.go index c70091815..706eb1eee 100644 --- a/upload-server/server/local_test.go +++ b/upload-server/server/local_test.go @@ -15,7 +15,7 @@ import ( "github.com/netbirdio/netbird/upload-server/types" ) -const testSigningKey = "test-signing-key" +const testSigningKey = "test-signing-key-with-enough-length" func signedQuery(t *testing.T, objectKey string) string { t.Helper() @@ -24,12 +24,12 @@ func signedQuery(t *testing.T, objectKey string) string { } func Test_LocalHandlerGetUploadURL(t *testing.T) { - mockURL := "http://localhost:8080" + mockURL := "https://localhost:8080" t.Setenv("SERVER_URL", mockURL) t.Setenv("STORE_DIR", t.TempDir()) mux := http.NewServeMux() - err := configureLocalHandlers(mux, newRateLimiter()) + err := configureLocalHandlers(mux, newTestRateLimiter(t)) require.NoError(t, err) req := httptest.NewRequest(http.MethodGet, types.GetURLPath+"?id=test-file", nil) @@ -50,13 +50,13 @@ func Test_LocalHandlerGetUploadURL(t *testing.T) { func Test_LocalHandlePutRequest(t *testing.T) { mockDir := t.TempDir() - mockURL := "http://localhost:8080" + mockURL := "https://localhost:8080" t.Setenv("SERVER_URL", mockURL) t.Setenv("STORE_DIR", mockDir) t.Setenv(signingKeyVar, testSigningKey) mux := http.NewServeMux() - err := configureLocalHandlers(mux, newRateLimiter()) + err := configureLocalHandlers(mux, newTestRateLimiter(t)) require.NoError(t, err) fileContent := []byte("test file content") @@ -76,13 +76,13 @@ func Test_LocalHandlePutRequest(t *testing.T) { func Test_LocalHandlePutRequest_PathTraversal(t *testing.T) { mockDir := t.TempDir() - mockURL := "http://localhost:8080" + mockURL := "https://localhost:8080" t.Setenv("SERVER_URL", mockURL) t.Setenv("STORE_DIR", mockDir) t.Setenv(signingKeyVar, testSigningKey) mux := http.NewServeMux() - err := configureLocalHandlers(mux, newRateLimiter()) + err := configureLocalHandlers(mux, newTestRateLimiter(t)) require.NoError(t, err) fileContent := []byte("malicious content") @@ -101,11 +101,11 @@ func Test_LocalHandlePutRequest_PathTraversal(t *testing.T) { func Test_LocalHandlePutRequest_DirTraversal(t *testing.T) { mockDir := t.TempDir() - t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("SERVER_URL", "https://localhost:8080") t.Setenv("STORE_DIR", mockDir) t.Setenv(signingKeyVar, testSigningKey) - l := &local{url: "http://localhost:8080", dir: mockDir, signer: &signer{key: []byte(testSigningKey)}} + l := &local{url: "https://localhost:8080", dir: mockDir, signer: &signer{key: []byte(testSigningKey)}} body := bytes.NewReader([]byte("bad")) req := httptest.NewRequest(http.MethodPut, @@ -124,12 +124,12 @@ func Test_LocalHandlePutRequest_DirTraversal(t *testing.T) { func Test_LocalHandlePutRequest_DuplicateFile(t *testing.T) { mockDir := t.TempDir() - t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("SERVER_URL", "https://localhost:8080") t.Setenv("STORE_DIR", mockDir) t.Setenv(signingKeyVar, testSigningKey) mux := http.NewServeMux() - err := configureLocalHandlers(mux, newRateLimiter()) + err := configureLocalHandlers(mux, newTestRateLimiter(t)) require.NoError(t, err) req := httptest.NewRequest(http.MethodPut, @@ -151,12 +151,12 @@ func Test_LocalHandlePutRequest_DuplicateFile(t *testing.T) { func Test_LocalHandlePutRequest_BodyTooLarge(t *testing.T) { mockDir := t.TempDir() - t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("SERVER_URL", "https://localhost:8080") t.Setenv("STORE_DIR", mockDir) t.Setenv(signingKeyVar, testSigningKey) mux := http.NewServeMux() - err := configureLocalHandlers(mux, newRateLimiter()) + err := configureLocalHandlers(mux, newTestRateLimiter(t)) require.NoError(t, err) largeBody := make([]byte, maxUploadSize+1) @@ -170,3 +170,12 @@ func Test_LocalHandlePutRequest_BodyTooLarge(t *testing.T) { _, err = os.Stat(filepath.Join(mockDir, "dir", "big.txt")) require.True(t, os.IsNotExist(err)) } + +func Test_ConfigureLocalHandlersRejectsPlaintextURL(t *testing.T) { + t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("STORE_DIR", t.TempDir()) + t.Setenv(signingKeyVar, testSigningKey) + + err := configureLocalHandlers(http.NewServeMux(), newTestRateLimiter(t)) + require.Error(t, err) +} diff --git a/upload-server/server/ratelimit.go b/upload-server/server/ratelimit.go index f28d2e312..081cf89c1 100644 --- a/upload-server/server/ratelimit.go +++ b/upload-server/server/ratelimit.go @@ -11,16 +11,19 @@ import ( const defaultUploadBurst = 100 func newRateLimiter() *middleware.APIRateLimiter { - cfg, _ := middleware.RateLimiterConfigFromEnv() + cfg, enabled := middleware.RateLimiterConfigFromEnv() if os.Getenv(middleware.RateLimitingBurstEnv) == "" { cfg.Burst = defaultUploadBurst } - limiter := middleware.NewAPIRateLimiter(cfg) - if os.Getenv(middleware.RateLimitingEnabledEnv) == "false" { - limiter.SetEnabled(false) + // Rate limiting is enabled by default unless explicitly disabled + if os.Getenv(middleware.RateLimitingEnabledEnv) == "" { + enabled = true } + limiter := middleware.NewAPIRateLimiter(cfg) + limiter.SetEnabled(enabled) + log.Infof("Upload URL rate limiting: enabled=%t rate=%.0f/min burst=%d trusted_proxies=%q", limiter.Enabled(), cfg.RequestsPerMinute, cfg.Burst, os.Getenv(middleware.RateLimitingTrustedProxiesEnv)) diff --git a/upload-server/server/ratelimit_test.go b/upload-server/server/ratelimit_test.go index 3ca4f1e4f..fe14eb97d 100644 --- a/upload-server/server/ratelimit_test.go +++ b/upload-server/server/ratelimit_test.go @@ -11,6 +11,15 @@ import ( "github.com/netbirdio/netbird/upload-server/types" ) +func newTestRateLimiter(t *testing.T) *middleware.APIRateLimiter { + t.Helper() + + limiter := newRateLimiter() + t.Cleanup(limiter.Stop) + + return limiter +} + func getUploadURL(t *testing.T, mux *http.ServeMux) int { t.Helper() @@ -33,6 +42,7 @@ func Test_GetUploadURLIsRateLimited(t *testing.T) { } func Test_RateLimitingIsOnByDefault(t *testing.T) { + t.Setenv(middleware.RateLimitingEnabledEnv, "") t.Setenv(middleware.RateLimitingBurstEnv, "1") mux, _ := newLocalMux(t) @@ -48,3 +58,14 @@ func Test_RateLimitingCanBeDisabled(t *testing.T) { require.Equal(t, http.StatusOK, getUploadURL(t, mux)) require.Equal(t, http.StatusOK, getUploadURL(t, mux)) } + +func Test_ServerStopIsIdempotent(t *testing.T) { + t.Setenv("SERVER_URL", "https://localhost:8080") + t.Setenv("STORE_DIR", t.TempDir()) + t.Setenv(signingKeyVar, testSigningKey) + + srv := NewServer() + + require.NoError(t, srv.Stop()) + require.NotPanics(t, func() { _ = srv.Stop() }) +} diff --git a/upload-server/server/s3_test.go b/upload-server/server/s3_test.go index 7c732383e..4cea9e853 100644 --- a/upload-server/server/s3_test.go +++ b/upload-server/server/s3_test.go @@ -90,7 +90,7 @@ func Test_S3HandlerGetUploadURL(t *testing.T) { t.Setenv(bucketVar, bucketName) mux := http.NewServeMux() - err = configureS3Handlers(mux, newRateLimiter()) + err = configureS3Handlers(mux, newTestRateLimiter(t)) require.NoError(t, err) req := httptest.NewRequest(http.MethodGet, types.GetURLPath+"?id=test-file", nil) diff --git a/upload-server/server/signing.go b/upload-server/server/signing.go index 5d03b06b7..86ee785b2 100644 --- a/upload-server/server/signing.go +++ b/upload-server/server/signing.go @@ -22,6 +22,8 @@ const ( expiryParam = "exp" signatureParam = "sig" + + minSigningKeyLen = 32 ) type signer struct { @@ -33,6 +35,9 @@ func newSigner() (*signer, error) { if env == "" { return nil, fmt.Errorf("%s is set but empty", signingKeyVar) } + if len(env) < minSigningKeyLen { + return nil, fmt.Errorf("%s must be at least %d bytes", signingKeyVar, minSigningKeyLen) + } return &signer{key: []byte(env)}, nil } @@ -71,7 +76,7 @@ func (s *signer) verify(objectKey string, query url.Values, now time.Time) error if !hmac.Equal(got, s.signature(objectKey, exp)) { return fmt.Errorf("signature mismatch") } - if now.Unix() > exp { + if now.Unix() >= exp { return fmt.Errorf("upload URL expired") } diff --git a/upload-server/server/signing_test.go b/upload-server/server/signing_test.go index 2d33d2f9b..e8b4c21c6 100644 --- a/upload-server/server/signing_test.go +++ b/upload-server/server/signing_test.go @@ -8,6 +8,7 @@ import ( "net/url" "os" "path/filepath" + "strings" "testing" "time" @@ -20,12 +21,12 @@ func newLocalMux(t *testing.T) (*http.ServeMux, string) { t.Helper() mockDir := t.TempDir() - t.Setenv("SERVER_URL", "http://localhost:8080") + t.Setenv("SERVER_URL", "https://localhost:8080") t.Setenv("STORE_DIR", mockDir) t.Setenv(signingKeyVar, testSigningKey) mux := http.NewServeMux() - require.NoError(t, configureLocalHandlers(mux, newRateLimiter())) + require.NoError(t, configureLocalHandlers(mux, newTestRateLimiter(t))) return mux, mockDir } @@ -81,6 +82,10 @@ func Test_LocalHandlePutRequest_RejectsUnauthorized(t *testing.T) { name: "signature for a different object", query: signedQuery(t, "dir/other.txt"), }, + { + name: "signature expiring this second", + query: expired.sign("dir/file.txt", time.Now().Add(-signatureTTL)).Encode(), + }, { name: "expired signature", query: expired.sign("dir/file.txt", time.Now().Add(-signatureTTL-time.Minute)).Encode(), @@ -134,3 +139,10 @@ func Test_NewSignerRejectsEmptyKey(t *testing.T) { _, err := newSigner() require.Error(t, err) } + +func Test_NewSignerRejectsShortKey(t *testing.T) { + t.Setenv(signingKeyVar, strings.Repeat("a", minSigningKeyLen-1)) + + _, err := newSigner() + require.Error(t, err) +}