fix: prevent mirroring & migrations on unencrypted URL schemes (http://, git://) (#13490)

Requires that all outbound network connections made by migrations & mirroring be performed over encrypted communication channels.  As those communication channels (HTTPS and SSH) have their own built-in protections against DNS rebinding (TLS certificate validation and SSH host key validation), DNS rebinding attacks become ineffective as a means to redirect Forgejo to pull-from or push-to unexpected remotes with mirrors.

`[migrations].ALLOW_UNENCRYPTED` is a new setting which defaults to false, and can be set to `true` to restore access to unencrypted remotes.  The risks associated with it are documented in `app.example.ini` and will be transferred into the docs repo.

**Breaking:** Existing mirrors configured against `http://` and `git://` protocols will fail, with "unencrypted transfer protocol, remote cannot be validated".  It is recommended that those mirrors be changed to encrypted protocols.  `[migrations].ALLOW_UNENCRYPTED` may be set to `true` to revert to the previous behaviour, accepting the risks documented here.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/13490
Reviewed-by: Andreas Ahlenstorf <aahlenst@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

Author: mfenniak

custom/conf/app.example.ini

@@ -2559,6 +2559,13 @@ LEVEL = Info
 ;;
 ;; If set to true, completely ignores server certificate validation errors. This option is unsafe.
 ;SKIP_TLS_VERIFY = false
+;;
+;; Allow insecure connections for migrations, including http:// and git:// protocols. Credentials
+;; and data will be in plain-text. ALLOWED_DOMAINS, BLOCKED_DOMAINS, and ALLOW_LOCALNETWORKS
+;; protections may be bypassed by DNS rebinding attacks, allowing access to unprotected resources
+;; on your network. This may be appropriate for short-term usage during a Forgejo migration,
+;; but should be disabled in long-term production environments.
+;ALLOW_UNENCRYPTED = false
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

models/error.go

@@ -123,6 +123,7 @@ type ErrInvalidCloneAddr struct {
 	IsPermissionDenied bool
 	HasCredentials     bool
 	LocalPath          bool
+	Unencrypted        bool
 }
 
 // IsErrInvalidCloneAddr checks if an error is a ErrInvalidCloneAddr.
@@ -147,6 +148,9 @@ func (err *ErrInvalidCloneAddr) Error() string {
 	if err.HasCredentials {
 		return fmt.Sprintf("migration/cloning from '%s' is not allowed: the provided url contains credentials", err.Host)
 	}
+	if err.Unencrypted {
+		return fmt.Sprintf("migration/cloning from '%s' is not allowed: unencrypted transfer protocols are disabled because the authenticity of the remote cannot be validated", err.Host)
+	}
 
 	return fmt.Sprintf("migration/cloning from '%s' is not allowed", err.Host)
 }

modules/setting/migrations.go

@@ -11,6 +11,7 @@ var Migrations = struct {
 	BlockedDomains     string
 	AllowLocalNetworks bool
 	SkipTLSVerify      bool
+	AllowUnencrypted   bool
 }{
 	MaxAttempts:  3,
 	RetryBackoff: 3,
@@ -25,4 +26,5 @@ func loadMigrationsFrom(rootCfg ConfigProvider) {
 	Migrations.BlockedDomains = sec.Key("BLOCKED_DOMAINS").MustString("")
 	Migrations.AllowLocalNetworks = sec.Key("ALLOW_LOCALNETWORKS").MustBool(false)
 	Migrations.SkipTLSVerify = sec.Key("SKIP_TLS_VERIFY").MustBool(false)
+	Migrations.AllowUnencrypted = sec.Key("ALLOW_UNENCRYPTED").MustBool(false)
 }

services/migrations/allowlist/is_migrate_allowed.go

@@ -68,6 +68,10 @@ func isURLAllowed(remoteURL string, doer *user_model.User, isPushMirror bool) er
 		return &models.ErrInvalidCloneAddr{Host: u.Host, IsProtocolInvalid: true, IsPermissionDenied: true, IsURLError: true}
 	}
 
+	if (u.Scheme == "http" || u.Scheme == "git") && !setting.Migrations.AllowUnencrypted {
+		return &models.ErrInvalidCloneAddr{Host: remoteURL, Unencrypted: true}
+	}
+
 	hostName, _, err := net.SplitHostPort(u.Host)
 	if err != nil {
 		// u.Host can be "host" or "host:port"

services/migrations/allowlist/is_migrate_allowed_test.go

@@ -12,6 +12,7 @@ import (
 	"forgejo.org/models/unittest"
 	user_model "forgejo.org/models/user"
 	"forgejo.org/modules/setting"
+	"forgejo.org/modules/test"
 
 	"github.com/stretchr/testify/require"
 )
@@ -74,6 +75,38 @@ func TestMigrateWhiteBlocklist(t *testing.T) {
 	require.NoError(t, err)
 
 	setting.ImportLocalPaths = old
+
+	t.Run("require encrypted", func(t *testing.T) {
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, false)()
+
+		err = IsMigrateURLAllowed("http://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.ErrorContains(t, err, "unencrypted transfer protocol")
+
+		err = IsMigrateURLAllowed("git://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.ErrorContains(t, err, "unencrypted transfer protocol")
+
+		err = IsMigrateURLAllowed("https://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.NoError(t, err)
+
+		err = IsPushMirrorURLAllowed("ssh://user@10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.NoError(t, err)
+	})
+
+	t.Run("permit unencrypted", func(t *testing.T) {
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
+
+		err = IsMigrateURLAllowed("http://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.NoError(t, err)
+
+		err = IsMigrateURLAllowed("git://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.NoError(t, err)
+
+		err = IsMigrateURLAllowed("https://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.NoError(t, err)
+
+		err = IsPushMirrorURLAllowed("ssh://user@10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+		require.NoError(t, err)
+	})
 }
 
 func TestAllowBlockList(t *testing.T) {

tests/integration/api_push_mirror_test.go

@@ -45,6 +45,7 @@ func TestAPIPushMirror(t *testing.T) {
 
 func testAPIPushMirror(t *testing.T, u *url.URL) {
 	defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 	defer test.MockVariableValue(&setting.Mirror.Enabled, true)()
 	defer test.MockProtect(&mirror_service.AddPushMirrorRemote)()
 	defer test.MockProtect(&repo_model.DeletePushMirrors)()
@@ -149,6 +150,7 @@ func TestAPIPushMirrorBranchFilter(t *testing.T) {
 
 func testAPIPushMirrorBranchFilter(t *testing.T, u *url.URL) {
 	defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 	defer test.MockVariableValue(&setting.Mirror.Enabled, true)()
 	defer test.MockProtect(&mirror_service.AddPushMirrorRemote)()
 	defer test.MockProtect(&repo_model.DeletePushMirrors)()

tests/integration/dump_restore_test.go

@@ -20,6 +20,7 @@ import (
 	base "forgejo.org/modules/migration"
 	"forgejo.org/modules/setting"
 	"forgejo.org/modules/structs"
+	"forgejo.org/modules/test"
 	"forgejo.org/services/migrations"
 	migrations_allowlist "forgejo.org/services/migrations/allowlist"
 
@@ -30,6 +31,8 @@ import (
 
 func TestDumpRestore(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
+
 		AllowLocalNetworks := setting.Migrations.AllowLocalNetworks
 		setting.Migrations.AllowLocalNetworks = true
 		AppVer := setting.AppVer

tests/integration/migrate_test.go

@@ -62,6 +62,7 @@ func TestMigrateLocalPath(t *testing.T) {
 func TestMigrate(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.AppVer, "1.16.0")()
 		require.NoError(t, migrations_allowlist.Init())
 
@@ -117,6 +118,7 @@ func TestMigrate(t *testing.T) {
 func TestMigrateWithIssueComments(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.AppVer, "1.16.0")()
 		maxResponseItems := 10
 		numComments := 21
@@ -193,6 +195,7 @@ func TestMigrateWithIssueComments(t *testing.T) {
 func TestMigrateWithWiki(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.AppVer, "1.16.0")()
 		require.NoError(t, migrations_allowlist.Init())
 
@@ -249,6 +252,7 @@ func TestMigrateWithWiki(t *testing.T) {
 func TestMigrateWithReleases(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.AppVer, "1.16.0")()
 		require.NoError(t, migrations_allowlist.Init())
 

tests/integration/mirror_pull_test.go

@@ -46,6 +46,8 @@ import (
 )
 
 func TestMirrorPull(t *testing.T) {
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
+
 	t.Run("Basic", func(t *testing.T) {
 		defer tests.PrepareTestEnv(t)()
 
@@ -342,6 +344,8 @@ func TestMirrorPull(t *testing.T) {
 // Verifies that a pull mirror which was created while the remote address was permitted will fail to sync if the
 // AllowedDomains configuration later changes such that the remote URL is no longer permitted.
 func TestMirrorPullAddressCheck(t *testing.T) {
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
+
 	// Allow localhost as a migration domain so the mirror can initially be created from the local test server. Not
 	// using MockVariableValue due to need to undo `migrations_allowlist.Init()`.
 	prev := setting.Migrations.AllowedDomains
@@ -651,6 +655,8 @@ func TestPullMirrorRedactCredentials(t *testing.T) {
 }
 
 func TestMirrorPullLFS(t *testing.T) {
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
+
 	// Not using MockVariableValue due to need to undo `migrations_allowlist.Init()`
 	prev := setting.Migrations.AllowedDomains
 	setting.Migrations.AllowedDomains = "localhost"

tests/integration/mirror_push_test.go

@@ -89,6 +89,7 @@ func TestMirrorPush(t *testing.T) {
 
 func testMirrorPush(t *testing.T, u *url.URL) {
 	defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 
 	require.NoError(t, migrations_allowlist.Init())
 
@@ -388,6 +389,7 @@ func TestSSHPushMirror(t *testing.T) {
 func TestPushMirrorBranchFilterWebUI(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.Mirror.Enabled, true)()
 		require.NoError(t, migrations_allowlist.Init())
 
@@ -488,6 +490,7 @@ func TestPushMirrorBranchFilterWebUI(t *testing.T) {
 func TestPushMirrorBranchFilterIntegration(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.Mirror.Enabled, true)()
 		require.NoError(t, migrations_allowlist.Init())
 
@@ -577,6 +580,7 @@ func TestPushMirrorBranchFilterIntegration(t *testing.T) {
 func TestPushMirrorSettings(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.Mirror.Enabled, true)()
 		require.NoError(t, migrations_allowlist.Init())
 
@@ -648,6 +652,7 @@ func TestPushMirrorSettings(t *testing.T) {
 func TestPushMirrorBranchFilterSyncOperations(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.Mirror.Enabled, true)()
 		require.NoError(t, migrations_allowlist.Init())
 
@@ -882,6 +887,7 @@ func TestPushMirrorBranchFilterSyncOperations(t *testing.T) {
 func TestPushMirrorWebUIToAPIIntegration(t *testing.T) {
 	onApplicationRun(t, func(t *testing.T, u *url.URL) {
 		defer test.MockVariableValue(&setting.Migrations.AllowLocalNetworks, true)()
+		defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
 		defer test.MockVariableValue(&setting.Mirror.Enabled, true)()
 		require.NoError(t, migrations_allowlist.Init())
 
@@ -1097,6 +1103,8 @@ func TestPushMirrorWebUIToAPIIntegration(t *testing.T) {
 }
 
 func TestMirrorPushFailOnRedirect(t *testing.T) {
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
+
 	// Not using MockVariableValue due to need to undo `migrations_allowlist.Init()`
 	prev := setting.Migrations.AllowedDomains
 	setting.Migrations.AllowedDomains = "127.0.0.1"
@@ -1152,6 +1160,8 @@ func TestMirrorPushFailOnRedirect(t *testing.T) {
 // Verifies that a push mirror which was created while the remote address was permitted will fail to sync if the
 // AllowedDomains configuration later changes such that the remote URL is no longer permitted.
 func TestMirrorPushAddressCheck(t *testing.T) {
+	defer test.MockVariableValue(&setting.Migrations.AllowUnencrypted, true)()
+
 	// Not using MockVariableValue due to need to undo `migrations_allowlist.Init()`
 	prev := setting.Migrations.AllowedDomains
 	setting.Migrations.AllowedDomains = "127.0.0.1"