Skip to content

Revert "Added deny list for checking external user submitted urls (#39947) #40445

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ksykulev merged 1 commit into from
Feb 24, 2026
Merged

Revert "Added deny list for checking external user submitted urls (#39947) #40445

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion changes/14284-external-deny-list
View file
Open in desktop

This file was deleted.

5 changes: 1 addition & 4 deletions cmd/fleetctl/integrationtest/gitops/gitops_enterprise_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -522,10 +522,6 @@ settings:
// At the same time, GitOps uploads Apple profiles that use the newly configured CAs.
func (s *enterpriseIntegrationGitopsTestSuite) TestCAIntegrations() {
t := s.T()

dev_mode.IsEnabled = true
t.Cleanup(func() { dev_mode.IsEnabled = false })

user := s.createGitOpsUser(t)
fleetctlConfig := s.createFleetctlConfig(t, user)

Expand Down Expand Up @@ -3505,6 +3501,7 @@ team_settings:
} else {
require.NoError(t, err)
}

})
}
}
72 changes: 39 additions & 33 deletions ee/server/service/certificate_authorities.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import (
"context"
"errors"
"fmt"
"net/url"
"regexp"
"strings"

"github.com/fleetdm/fleet/v4/pkg/fleethttp"
"github.com/fleetdm/fleet/v4/server/authz"
"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
"github.com/fleetdm/fleet/v4/server/fleet"
Expand Down Expand Up @@ -208,8 +208,8 @@ func (svc *Service) validatePayload(p *fleet.CertificateAuthorityPayload, errPre
}

func (svc *Service) validateDigicert(ctx context.Context, digicertCA *fleet.DigiCertCA, errPrefix string) error {
if err := fleethttp.CheckURLForSSRF(ctx, digicertCA.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sDigiCert URL is invalid: %v", errPrefix, err))
if err := validateURL(digicertCA.URL, "DigiCert", errPrefix); err != nil {
return err
}
if digicertCA.APIToken == "" || digicertCA.APIToken == fleet.MaskedPassword {
return fleet.NewInvalidArgumentError("api_token", fmt.Sprintf("%sInvalid API token. Please correct and try again.", errPrefix))
Expand Down Expand Up @@ -321,8 +321,8 @@ func (svc *Service) validateHydrant(ctx context.Context, hydrantCA *fleet.Hydran
if err := validateCAName(hydrantCA.Name, errPrefix); err != nil {
return err
}
if err := fleethttp.CheckURLForSSRF(ctx, hydrantCA.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sHydrant URL is invalid: %v", errPrefix, err))
if err := validateURL(hydrantCA.URL, "Hydrant", errPrefix); err != nil {
return err
}
if hydrantCA.ClientID == "" {
return fleet.NewInvalidArgumentError("client_id", fmt.Sprintf("%sInvalid Hydrant Client ID. Please correct and try again.", errPrefix))
Expand All @@ -346,8 +346,8 @@ func (svc *Service) validateEST(ctx context.Context, estProxyCA *fleet.ESTProxyC
if err := validateCAName(estProxyCA.Name, errPrefix); err != nil {
return err
}
if err := fleethttp.CheckURLForSSRF(ctx, estProxyCA.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sEST URL is invalid: %v", errPrefix, err))
if err := validateURL(estProxyCA.URL, "EST", errPrefix); err != nil {
return err
}
if estProxyCA.Username == "" {
return fleet.NewInvalidArgumentError("username", fmt.Sprintf("%sInvalid EST Username. Please correct and try again.", errPrefix))
Expand All @@ -361,12 +361,18 @@ func (svc *Service) validateEST(ctx context.Context, estProxyCA *fleet.ESTProxyC
return nil
}

func (svc *Service) validateNDESSCEPProxy(ctx context.Context, ndesSCEP *fleet.NDESSCEPProxyCA, errPrefix string) error {
if err := fleethttp.CheckURLForSSRF(ctx, ndesSCEP.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sNDES SCEP URL is invalid: %v", errPrefix, err))
func validateURL(caURL, displayType, errPrefix string) error {
if u, err := url.ParseRequestURI(caURL); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sInvalid %s URL. Please correct and try again.", errPrefix, displayType))
} else if u.Scheme != "https" && u.Scheme != "http" {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%s%s URL scheme must be https or http", errPrefix, displayType))
}
if err := fleethttp.CheckURLForSSRF(ctx, ndesSCEP.AdminURL, nil); err != nil {
return fleet.NewInvalidArgumentError("admin_url", fmt.Sprintf("%sNDES SCEP admin URL is invalid: %v", errPrefix, err))
return nil
}

func (svc *Service) validateNDESSCEPProxy(ctx context.Context, ndesSCEP *fleet.NDESSCEPProxyCA, errPrefix string) error {
if err := validateURL(ndesSCEP.URL, "NDES SCEP", errPrefix); err != nil {
return err
}
if err := svc.scepConfigService.ValidateSCEPURL(ctx, ndesSCEP.URL); err != nil {
level.Error(svc.logger).Log("msg", "Failed to validate NDES SCEP URL", "err", err)
Expand All @@ -390,8 +396,8 @@ func (svc *Service) validateCustomSCEPProxy(ctx context.Context, customSCEP *fle
if err := validateCAName(customSCEP.Name, errPrefix); err != nil {
return err
}
if err := fleethttp.CheckURLForSSRF(ctx, customSCEP.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sCustom SCEP Proxy URL is invalid: %v", errPrefix, err))
if err := validateURL(customSCEP.URL, "SCEP", errPrefix); err != nil {
return err
}
if customSCEP.Challenge == "" || customSCEP.Challenge == fleet.MaskedPassword {
return fleet.NewInvalidArgumentError("challenge", fmt.Sprintf("%sCustom SCEP Proxy challenge cannot be empty", errPrefix))
Expand All @@ -407,8 +413,8 @@ func (svc *Service) validateSmallstepSCEPProxy(ctx context.Context, smallstepSCE
if err := validateCAName(smallstepSCEP.Name, errPrefix); err != nil {
return err
}
if err := fleethttp.CheckURLForSSRF(ctx, smallstepSCEP.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sSmallstep SCEP URL is invalid: %v", errPrefix, err))
if err := validateURL(smallstepSCEP.URL, "Smallstep SCEP", errPrefix); err != nil {
return err
}
if smallstepSCEP.Username == "" {
return fleet.NewInvalidArgumentError("username", fmt.Sprintf("%sSmallstep username cannot be empty", errPrefix))
Expand Down Expand Up @@ -1251,9 +1257,10 @@ func (svc *Service) validateDigicertUpdate(ctx context.Context, digicert *fleet.
}
}
if digicert.URL != nil {
if err := fleethttp.CheckURLForSSRF(ctx, *digicert.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sDigiCert URL is invalid: %v", errPrefix, err))
if err := validateURL(*digicert.URL, "DigiCert", errPrefix); err != nil {
return err
}

// We want to generate a DigiCertCA struct with all required fields to verify the new URL.
// If URL or APIToken are not being updated we use the existing values from oldCA
digicertCA := fleet.DigiCertCA{
Expand Down Expand Up @@ -1331,9 +1338,10 @@ func (svc *Service) validateHydrantUpdate(ctx context.Context, hydrant *fleet.Hy
}
}
if hydrant.URL != nil {
if err := fleethttp.CheckURLForSSRF(ctx, *hydrant.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sHydrant URL is invalid: %v", errPrefix, err))
if err := validateURL(*hydrant.URL, "Hydrant", errPrefix); err != nil {
return err
}

hydrantCAToVerify := fleet.ESTProxyCA{ // The hydrant service for verification only requires the URL.
URL: *hydrant.URL,
}
Expand Down Expand Up @@ -1362,9 +1370,10 @@ func (svc *Service) validateCustomESTUpdate(ctx context.Context, estUpdate *flee
}
}
if estUpdate.URL != nil {
if err := fleethttp.CheckURLForSSRF(ctx, *estUpdate.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sEST URL is invalid: %v", errPrefix, err))
if err := validateURL(*estUpdate.URL, "EST", errPrefix); err != nil {
return err
}

hydrantCAToVerify := fleet.ESTProxyCA{ // The EST service for verification only requires the URL.
URL: *estUpdate.URL,
}
Expand All @@ -1390,8 +1399,8 @@ func (svc *Service) validateNDESSCEPProxyUpdate(ctx context.Context, ndesSCEP *f
// some methods in this fuction require the NDESSCEPProxyCA type so we convert the ndes update payload here

if ndesSCEP.URL != nil {
if err := fleethttp.CheckURLForSSRF(ctx, *ndesSCEP.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sNDES SCEP URL is invalid: %v", errPrefix, err))
if err := validateURL(*ndesSCEP.URL, "NDES SCEP", errPrefix); err != nil {
return err
}
if err := svc.scepConfigService.ValidateSCEPURL(ctx, *ndesSCEP.URL); err != nil {
level.Error(svc.logger).Log("msg", "Failed to validate NDES SCEP URL", "err", err)
Expand All @@ -1405,9 +1414,6 @@ func (svc *Service) validateNDESSCEPProxyUpdate(ctx context.Context, ndesSCEP *f
}
}

if err := fleethttp.CheckURLForSSRF(ctx, *ndesSCEP.AdminURL, nil); err != nil {
return fleet.NewInvalidArgumentError("admin_url", fmt.Sprintf("%sNDES SCEP admin URL is invalid: %v", errPrefix, err))
}
// We want to generate a NDESSCEPProxyCA struct with all required fields to verify the admin URL.
// If URL, Username or Password are not being updated we use the existing values from oldCA
NDESProxy := fleet.NDESSCEPProxyCA{
Expand Down Expand Up @@ -1451,8 +1457,8 @@ func (svc *Service) validateCustomSCEPProxyUpdate(ctx context.Context, customSCE
}
}
if customSCEP.URL != nil {
if err := fleethttp.CheckURLForSSRF(ctx, *customSCEP.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sCustom SCEP Proxy URL is invalid: %v", errPrefix, err))
if err := validateURL(*customSCEP.URL, "SCEP", errPrefix); err != nil {
return err
}
if err := svc.scepConfigService.ValidateSCEPURL(ctx, *customSCEP.URL); err != nil {
level.Error(svc.logger).Log("msg", "Failed to validate custom SCEP URL", "err", err)
Expand All @@ -1475,8 +1481,8 @@ func (svc *Service) validateSmallstepSCEPProxyUpdate(ctx context.Context, smalls
}
}
if smallstep.URL != nil {
if err := fleethttp.CheckURLForSSRF(ctx, *smallstep.URL, nil); err != nil {
return fleet.NewInvalidArgumentError("url", fmt.Sprintf("%sSmallstep SCEP URL is invalid: %v", errPrefix, err))
if err := validateURL(*smallstep.URL, "SCEP", errPrefix); err != nil {
return err
}
if err := svc.scepConfigService.ValidateSCEPURL(ctx, *smallstep.URL); err != nil {
level.Error(svc.logger).Log("msg", "Failed to validate Smallstep SCEP URL", "err", err)
Expand All @@ -1500,8 +1506,8 @@ func (svc *Service) validateSmallstepSCEPProxyUpdate(ctx context.Context, smalls

// Additional validation if url was updated
if smallstep.ChallengeURL != nil {
if err := fleethttp.CheckURLForSSRF(ctx, *smallstep.ChallengeURL, nil); err != nil {
return fleet.NewInvalidArgumentError("challenge_url", fmt.Sprintf("%sChallenge URL is invalid: %v", errPrefix, err))
if err := validateURL(*smallstep.ChallengeURL, "Challenge", errPrefix); err != nil {
return err
}
smallstepSCEPProxy.ChallengeURL = *smallstep.ChallengeURL
}
Expand Down
31 changes: 12 additions & 19 deletions ee/server/service/certificate_authorities_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"github.com/fleetdm/fleet/v4/ee/server/service/est"
"github.com/fleetdm/fleet/v4/server/authz"
"github.com/fleetdm/fleet/v4/server/contexts/viewer"
"github.com/fleetdm/fleet/v4/server/dev_mode"
"github.com/fleetdm/fleet/v4/server/fleet"
"github.com/fleetdm/fleet/v4/server/mock"
scep_mock "github.com/fleetdm/fleet/v4/server/mock/scep"
Expand Down Expand Up @@ -145,10 +144,6 @@ func setupMockCAServers(t *testing.T) (digicertServer, hydrantServer *httptest.S
}

func TestCreatingCertificateAuthorities(t *testing.T) {
// Enable dev mode so CheckURLForSSRF skips the private-IP blocklist for the duration of this test.
dev_mode.IsEnabled = true
t.Cleanup(func() { dev_mode.IsEnabled = false })

digicertServer, hydrantServer := setupMockCAServers(t)
digicertURL := digicertServer.URL
hydrantURL := hydrantServer.URL
Expand Down Expand Up @@ -539,7 +534,7 @@ func TestCreatingCertificateAuthorities(t *testing.T) {
}

createdCA, err := svc.NewCertificateAuthority(ctx, createDigicertRequest)
require.ErrorContains(t, err, "DigiCert URL is invalid")
require.ErrorContains(t, err, "Invalid DigiCert URL")
require.Len(t, createdCAs, 0)
require.Nil(t, createdCA)
})
Expand Down Expand Up @@ -680,7 +675,7 @@ func TestCreatingCertificateAuthorities(t *testing.T) {
}

createdCA, err := svc.NewCertificateAuthority(ctx, createHydrantRequest)
require.ErrorContains(t, err, "Hydrant URL is invalid")
require.ErrorContains(t, err, "Invalid Hydrant URL.")
require.Len(t, createdCAs, 0)
require.Nil(t, createdCA)
})
Expand Down Expand Up @@ -768,7 +763,7 @@ func TestCreatingCertificateAuthorities(t *testing.T) {
}

createdCA, err := svc.NewCertificateAuthority(ctx, createCustomSCEPRequest)
require.ErrorContains(t, err, "Custom SCEP Proxy URL is invalid")
require.ErrorContains(t, err, "Invalid SCEP URL.")
require.Len(t, createdCAs, 0)
require.Nil(t, createdCA)
})
Expand Down Expand Up @@ -824,7 +819,7 @@ func TestCreatingCertificateAuthorities(t *testing.T) {
}

createdCA, err := svc.NewCertificateAuthority(ctx, createNDESSCEPRequest)
require.ErrorContains(t, err, "NDES SCEP URL is invalid")
require.ErrorContains(t, err, "Invalid NDES SCEP URL.")
require.Len(t, createdCAs, 0)
require.Nil(t, createdCA)
})
Expand Down Expand Up @@ -984,7 +979,7 @@ func TestCreatingCertificateAuthorities(t *testing.T) {
}

createdCA, err := svc.NewCertificateAuthority(ctx, createSmallstepRequest)
require.ErrorContains(t, err, "Smallstep SCEP URL is invalid")
require.ErrorContains(t, err, "Invalid Smallstep SCEP URL.")
require.Len(t, createdCAs, 0)
require.Nil(t, createdCA)
})
Expand Down Expand Up @@ -1097,9 +1092,7 @@ func TestCreatingCertificateAuthorities(t *testing.T) {
}

func TestUpdatingCertificateAuthorities(t *testing.T) {
// Enable dev mode so CheckURLForSSRF skips the private-IP blocklist for the duration of this test.
dev_mode.IsEnabled = true
t.Cleanup(func() { dev_mode.IsEnabled = false })
t.Parallel()

digicertServer, hydrantServer := setupMockCAServers(t)
digicertURL := digicertServer.URL
Expand Down Expand Up @@ -1367,7 +1360,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) {
}

err := svc.UpdateCertificateAuthority(ctx, digicertID, payload)
require.ErrorContains(t, err, "DigiCert URL is invalid")
require.EqualError(t, err, "validation failed: url Couldn't edit certificate authority. Invalid DigiCert URL. Please correct and try again.")
})

t.Run("Bad URL Path", func(t *testing.T) {
Expand Down Expand Up @@ -1509,7 +1502,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) {
}

err := svc.UpdateCertificateAuthority(ctx, hydrantID, payload)
require.ErrorContains(t, err, "Hydrant URL is invalid")
require.EqualError(t, err, "validation failed: url Couldn't edit certificate authority. Invalid Hydrant URL. Please correct and try again.")
})

t.Run("Bad URL", func(t *testing.T) {
Expand Down Expand Up @@ -1595,7 +1588,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) {
}

err := svc.UpdateCertificateAuthority(ctx, scepID, payload)
require.ErrorContains(t, err, "Custom SCEP Proxy URL is invalid")
require.EqualError(t, err, "validation failed: url Couldn't edit certificate authority. Invalid SCEP URL. Please correct and try again.")
})

t.Run("Requires challenge when updating URL", func(t *testing.T) {
Expand Down Expand Up @@ -1658,7 +1651,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) {
}

err := svc.UpdateCertificateAuthority(ctx, ndesID, payload)
require.ErrorContains(t, err, "NDES SCEP URL is invalid")
require.EqualError(t, err, "validation failed: url Couldn't edit certificate authority. Invalid NDES SCEP URL. Please correct and try again.")
})

t.Run("Bad SCEP URL", func(t *testing.T) {
Expand Down Expand Up @@ -1817,7 +1810,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) {
}

err := svc.UpdateCertificateAuthority(ctx, smallstepID, payload)
require.ErrorContains(t, err, "Smallstep SCEP URL is invalid")
require.EqualError(t, err, "validation failed: url Couldn't edit certificate authority. Invalid SCEP URL. Please correct and try again.")
})

t.Run("Invalid Challenge URL format", func(t *testing.T) {
Expand All @@ -1833,7 +1826,7 @@ func TestUpdatingCertificateAuthorities(t *testing.T) {
}

err := svc.UpdateCertificateAuthority(ctx, smallstepID, payload)
require.ErrorContains(t, err, "Challenge URL is invalid")
require.EqualError(t, err, "validation failed: url Couldn't edit certificate authority. Invalid Challenge URL. Please correct and try again.")
})

t.Run("Bad Smallstep SCEP URL", func(t *testing.T) {
Expand Down
8 changes: 3 additions & 5 deletions ee/server/service/scep_proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,7 @@ func (s *SCEPConfigService) GetNDESSCEPChallenge(ctx context.Context, proxy flee
// Get the challenge from NDES
client := fleethttp.NewClient(fleethttp.WithTimeout(*s.Timeout))
client.Transport = ntlmssp.Negotiator{
RoundTripper: fleethttp.NewSSRFProtectedTransport(),
RoundTripper: fleethttp.NewTransport(),
}
req, err := http.NewRequest(http.MethodGet, adminURL, http.NoBody)
if err != nil {
Expand Down Expand Up @@ -586,10 +586,8 @@ func (s *SCEPConfigService) ValidateSmallstepChallengeURL(ctx context.Context, c
}

func (s *SCEPConfigService) GetSmallstepSCEPChallenge(ctx context.Context, ca fleet.SmallstepSCEPProxyCA) (string, error) {
client := fleethttp.NewClient(
fleethttp.WithTimeout(30*time.Second),
fleethttp.WithTransport(fleethttp.NewSSRFProtectedTransport()),
)
// Get the challenge from Smallstep
client := fleethttp.NewClient(fleethttp.WithTimeout(30 * time.Second))
var reqBody bytes.Buffer
if err := json.NewEncoder(&reqBody).Encode(fleet.SmallstepChallengeRequestBody{
Webhook: fleet.SmallstepChallengeWebhook{
Expand Down
Loading
Loading