From def9ad88b03679399931916726650c7cd03193c2 Mon Sep 17 00:00:00 2001 From: kermieisinthehouse Date: Wed, 2 Feb 2022 15:16:22 -0800 Subject: [PATCH] Remove trusted proxies (#2229) --- graphql/documents/data/config.graphql | 1 - graphql/schema/types/config.graphql | 4 +- pkg/api/authentication.go | 5 -- pkg/api/resolver_mutation_configure.go | 4 -- pkg/api/resolver_query_configuration.go | 1 - pkg/manager/config/config.go | 7 --- pkg/manager/config/config_concurrency_test.go | 1 - pkg/session/authentication.go | 58 +++---------------- pkg/session/authentication_test.go | 40 +------------ .../components/Changelog/versions/v0130.md | 1 + .../Settings/SettingsSecurityPanel.tsx | 10 +--- ui/v2.5/src/locales/en-GB.json | 2 - 12 files changed, 14 insertions(+), 120 deletions(-) diff --git a/graphql/documents/data/config.graphql b/graphql/documents/data/config.graphql index 1e5040f09..d801a216b 100644 --- a/graphql/documents/data/config.graphql +++ b/graphql/documents/data/config.graphql @@ -25,7 +25,6 @@ fragment ConfigGeneralData on ConfigGeneralResult { username password maxSessionAge - trustedProxies logFile logOut logLevel diff --git a/graphql/schema/types/config.graphql b/graphql/schema/types/config.graphql index d4fe5aa31..ee40fd95d 100644 --- a/graphql/schema/types/config.graphql +++ b/graphql/schema/types/config.graphql @@ -76,7 +76,7 @@ input ConfigGeneralInput { """Maximum session cookie age""" maxSessionAge: Int """Comma separated list of proxies to allow traffic from""" - trustedProxies: [String!] + trustedProxies: [String!] @deprecated(reason: "no longer supported") """Name of the log file""" logFile: String """Whether to also output to stderr""" @@ -157,7 +157,7 @@ type ConfigGeneralResult { """Maximum session cookie age""" maxSessionAge: Int! """Comma separated list of proxies to allow traffic from""" - trustedProxies: [String!]! + trustedProxies: [String!] @deprecated(reason: "no longer supported") """Name of the log file""" logFile: String """Whether to also output to stderr""" diff --git a/pkg/api/authentication.go b/pkg/api/authentication.go index 17246badd..fd638d697 100644 --- a/pkg/api/authentication.go +++ b/pkg/api/authentication.go @@ -57,15 +57,10 @@ func authenticateHandler() func(http.Handler) http.Handler { if err := session.CheckAllowPublicWithoutAuth(c, r); err != nil { var externalAccess session.ExternalAccessError - var untrustedProxy session.UntrustedProxyError switch { case errors.As(err, &externalAccess): securityActivateTripwireAccessedFromInternetWithoutAuth(c, externalAccess, w) return - case errors.As(err, &untrustedProxy): - logger.Warnf("Rejected request from untrusted proxy: %v", net.IP(untrustedProxy)) - w.WriteHeader(http.StatusForbidden) - return default: logger.Errorf("Error checking external access security: %v", err) w.WriteHeader(http.StatusInternalServerError) diff --git a/pkg/api/resolver_mutation_configure.go b/pkg/api/resolver_mutation_configure.go index 48f074769..d9fa32225 100644 --- a/pkg/api/resolver_mutation_configure.go +++ b/pkg/api/resolver_mutation_configure.go @@ -197,10 +197,6 @@ func (r *mutationResolver) ConfigureGeneral(ctx context.Context, input models.Co c.Set(config.MaxSessionAge, *input.MaxSessionAge) } - if input.TrustedProxies != nil { - c.Set(config.TrustedProxies, input.TrustedProxies) - } - if input.LogFile != nil { c.Set(config.LogFile, input.LogFile) } diff --git a/pkg/api/resolver_query_configuration.go b/pkg/api/resolver_query_configuration.go index 771c32357..e79eccc87 100644 --- a/pkg/api/resolver_query_configuration.go +++ b/pkg/api/resolver_query_configuration.go @@ -86,7 +86,6 @@ func makeConfigGeneralResult() *models.ConfigGeneralResult { Username: config.GetUsername(), Password: config.GetPasswordHash(), MaxSessionAge: config.GetMaxSessionAge(), - TrustedProxies: config.GetTrustedProxies(), LogFile: &logFile, LogOut: config.GetLogOut(), LogLevel: config.GetLogLevel(), diff --git a/pkg/manager/config/config.go b/pkg/manager/config/config.go index bfe1b7003..c95628930 100644 --- a/pkg/manager/config/config.go +++ b/pkg/manager/config/config.go @@ -147,7 +147,6 @@ const ( FunscriptOffset = "funscript_offset" // Security - TrustedProxies = "trusted_proxies" dangerousAllowPublicWithoutAuth = "dangerous_allow_public_without_auth" dangerousAllowPublicWithoutAuthDefault = "false" SecurityTripwireAccessedFromPublicInternet = "security_tripwire_accessed_from_public_internet" @@ -1014,12 +1013,6 @@ func (i *Instance) GetDefaultGenerateSettings() *models.GenerateMetadataOptions return nil } -// GetTrustedProxies returns a comma separated list of ip addresses that should allow proxying. -// When empty, allow from any private network -func (i *Instance) GetTrustedProxies() []string { - return i.getStringSlice(TrustedProxies) -} - // GetDangerousAllowPublicWithoutAuth determines if the security feature is enabled. // See https://github.com/stashapp/stash/wiki/Authentication-Required-When-Accessing-Stash-From-the-Internet func (i *Instance) GetDangerousAllowPublicWithoutAuth() bool { diff --git a/pkg/manager/config/config_concurrency_test.go b/pkg/manager/config/config_concurrency_test.go index 4b940cb2e..6db550c73 100644 --- a/pkg/manager/config/config_concurrency_test.go +++ b/pkg/manager/config/config_concurrency_test.go @@ -99,7 +99,6 @@ func TestConcurrentConfigAccess(t *testing.T) { i.Set(DefaultIdentifySettings, i.GetDefaultIdentifySettings()) i.Set(DeleteGeneratedDefault, i.GetDeleteGeneratedDefault()) i.Set(DeleteFileDefault, i.GetDeleteFileDefault()) - i.Set(TrustedProxies, i.GetTrustedProxies()) i.Set(dangerousAllowPublicWithoutAuth, i.GetDangerousAllowPublicWithoutAuth()) i.Set(SecurityTripwireAccessedFromPublicInternet, i.GetSecurityTripwireAccessedFromPublicInternet()) i.Set(DisableDropdownCreatePerformer, i.GetDisableDropdownCreate().Performer) diff --git a/pkg/session/authentication.go b/pkg/session/authentication.go index ff617c774..503fe777d 100644 --- a/pkg/session/authentication.go +++ b/pkg/session/authentication.go @@ -16,12 +16,6 @@ func (e ExternalAccessError) Error() string { return fmt.Sprintf("stash accessed from external IP %s", net.IP(e).String()) } -type UntrustedProxyError net.IP - -func (e UntrustedProxyError) Error() string { - return fmt.Sprintf("untrusted proxy %s", net.IP(e).String()) -} - func CheckAllowPublicWithoutAuth(c *config.Instance, r *http.Request) error { if !c.HasCredentials() && !c.GetDangerousAllowPublicWithoutAuth() && !c.IsNewSystem() { requestIPString, _, err := net.SplitHostPort(r.RemoteAddr) @@ -42,43 +36,21 @@ func CheckAllowPublicWithoutAuth(c *config.Instance, r *http.Request) error { if r.Header.Get("X-FORWARDED-FOR") != "" { // Request was proxied - trustedProxies := c.GetTrustedProxies() proxyChain := strings.Split(r.Header.Get("X-FORWARDED-FOR"), ", ") - if len(trustedProxies) == 0 { - // validate proxies against local network only - if !isLocalIP(requestIP) { - return ExternalAccessError(requestIP) - } else { - // Safe to validate X-Forwarded-For - for i := range proxyChain { - ip := net.ParseIP(proxyChain[i]) - if !isLocalIP(ip) { - return ExternalAccessError(ip) - } - } - } + // validate proxies against local network only + if !isLocalIP(requestIP) { + return ExternalAccessError(requestIP) } else { - // validate proxies against trusted proxies list - if isIPTrustedProxy(requestIP, trustedProxies) { - // Safe to validate X-Forwarded-For - // validate backwards, as only the last one is not attacker-controlled - for i := len(proxyChain) - 1; i >= 0; i-- { - ip := net.ParseIP(proxyChain[i]) - if i == 0 { - // last entry is originating device, check if from the public internet - if !isLocalIP(ip) { - return ExternalAccessError(ip) - } - } else if !isIPTrustedProxy(ip, trustedProxies) { - return UntrustedProxyError(ip) - } + // Safe to validate X-Forwarded-For + for i := range proxyChain { + ip := net.ParseIP(proxyChain[i]) + if !isLocalIP(ip) { + return ExternalAccessError(ip) } - } else { - // Proxy not on safe proxy list - return UntrustedProxyError(requestIP) } } + } else if !isLocalIP(requestIP) { // request was not proxied return ExternalAccessError(requestIP) } @@ -104,18 +76,6 @@ func isLocalIP(requestIP net.IP) bool { return requestIP.IsPrivate() || requestIP.IsLoopback() || requestIP.IsLinkLocalUnicast() || cgNatAddrSpace.Contains(requestIP) } -func isIPTrustedProxy(ip net.IP, trustedProxies []string) bool { - if len(trustedProxies) == 0 { - return isLocalIP(ip) - } - for _, v := range trustedProxies { - if ip.Equal(net.ParseIP(v)) { - return true - } - } - return false -} - func LogExternalAccessError(err ExternalAccessError) { logger.Errorf("Stash has been accessed from the internet (public IP %s), without authentication. \n"+ "This is extremely dangerous! The whole world can see your stash page and browse your files! \n"+ diff --git a/pkg/session/authentication_test.go b/pkg/session/authentication_test.go index 1cf967c8f..6a660bc5c 100644 --- a/pkg/session/authentication_test.go +++ b/pkg/session/authentication_test.go @@ -66,7 +66,7 @@ func TestCheckAllowPublicWithoutAuth(t *testing.T) { } { - // X-FORWARDED-FOR without trusted proxy + // X-FORWARDED-FOR testCases := []struct { proxyChain string err error @@ -91,39 +91,6 @@ func TestCheckAllowPublicWithoutAuth(t *testing.T) { } } - { - // X-FORWARDED-FOR with trusted proxy - var trustedProxies = []string{"8.8.8.8", "4.4.4.4"} - c.Set(config.TrustedProxies, trustedProxies) - - testCases := []struct { - address string - proxyChain string - err error - }{ - {"192.168.1.1:8080", "192.168.1.1, 192.168.1.2, 100.64.0.1, 127.0.0.1", &UntrustedProxyError{}}, - {"8.8.8.8:8080", "192.168.1.2, 127.0.0.1", &UntrustedProxyError{}}, - {"8.8.8.8:8080", "193.168.1.1, 4.4.4.4", &ExternalAccessError{}}, - {"8.8.8.8:8080", "4.4.4.4", &ExternalAccessError{}}, - {"8.8.8.8:8080", "192.168.1.1, 4.4.4.4a", &UntrustedProxyError{}}, - {"8.8.8.8:8080", "192.168.1.1a, 4.4.4.4", &ExternalAccessError{}}, - {"8.8.8.8:8080", "192.168.1.1, 4.4.4.4", nil}, - {"8.8.8.8:8080", "192.168.1.1", nil}, - } - - header := make(http.Header) - - for i, tc := range testCases { - header.Set("X-FORWARDED-FOR", tc.proxyChain) - r := &http.Request{ - RemoteAddr: tc.address, - Header: header, - } - - doTest(i, r, tc.err) - } - } - { // test invalid request IPs invalidIPs := []string{"192.168.1.a:9999", "192.168.1.1"} @@ -134,11 +101,6 @@ func TestCheckAllowPublicWithoutAuth(t *testing.T) { } err := CheckAllowPublicWithoutAuth(c, r) - if errors.As(err, &UntrustedProxyError{}) || errors.As(err, &ExternalAccessError{}) { - t.Errorf("[%s]: unexpected error: %v", remoteAddr, err) - continue - } - if err == nil { t.Errorf("[%s]: expected error", remoteAddr) continue diff --git a/ui/v2.5/src/components/Changelog/versions/v0130.md b/ui/v2.5/src/components/Changelog/versions/v0130.md index 5dc331aaa..82c480ea1 100644 --- a/ui/v2.5/src/components/Changelog/versions/v0130.md +++ b/ui/v2.5/src/components/Changelog/versions/v0130.md @@ -8,6 +8,7 @@ * Show counts on list tabs in Performer, Studio and Tag pages. ([#2169](https://github.com/stashapp/stash/pull/2169)) ### 🐛 Bug fixes +* Removed trusted proxies setting. ([#2229](https://github.com/stashapp/stash/pull/2229)) * Allow Stash to be iframed. ([#2217](https://github.com/stashapp/stash/pull/2217)) * Resolve CDP hostname if necessary. ([#2174](https://github.com/stashapp/stash/pull/2174)) * Generate sprites for short video files. ([#2167](https://github.com/stashapp/stash/pull/2167)) diff --git a/ui/v2.5/src/components/Settings/SettingsSecurityPanel.tsx b/ui/v2.5/src/components/Settings/SettingsSecurityPanel.tsx index ee1cd3bbe..0af6813a9 100644 --- a/ui/v2.5/src/components/Settings/SettingsSecurityPanel.tsx +++ b/ui/v2.5/src/components/Settings/SettingsSecurityPanel.tsx @@ -1,5 +1,5 @@ import React from "react"; -import { ModalSetting, NumberSetting, StringListSetting } from "./Inputs"; +import { ModalSetting, NumberSetting } from "./Inputs"; import { SettingSection } from "./SettingSection"; import * as GQL from "src/core/generated-graphql"; import { Button, Form } from "react-bootstrap"; @@ -162,14 +162,6 @@ export const SettingsSecurityPanel: React.FC = () => { value={general.maxSessionAge ?? undefined} onChange={(v) => saveGeneral({ maxSessionAge: v })} /> - - saveGeneral({ trustedProxies: v })} - /> ); diff --git a/ui/v2.5/src/locales/en-GB.json b/ui/v2.5/src/locales/en-GB.json index 89816a8c1..0f0486425 100644 --- a/ui/v2.5/src/locales/en-GB.json +++ b/ui/v2.5/src/locales/en-GB.json @@ -221,8 +221,6 @@ "password": "Password", "password_desc": "Password to access Stash. Leave blank to disable user authentication", "stash-box_integration": "Stash-box integration", - "trusted_proxies": "Trusted proxies", - "trusted_proxies_desc": "List of proxies that are allowed to proxy traffic into stash. Leave empty to allow from private network.", "username": "Username", "username_desc": "Username to access Stash. Leave blank to disable user authentication" },