From 030e292f30bf13a2516768b7d904eb9012d07075 Mon Sep 17 00:00:00 2001 From: Victor Lyuboslavsky <2685025+getvictor@users.noreply.github.com> Date: Mon, 4 Aug 2025 19:08:43 +0200 Subject: [PATCH] Fixed an issue where SSO URLs with trailing slashes (#31548) Fixes #31545 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. ## Testing - [x] Added/updated automated tests - [x] QA'd all new/changed functionality manually ## Summary by CodeRabbit * **Bug Fixes** * Resolved issues with Single Sign-On (SSO) and Mobile Device Management (MDM) SSO authentication failures caused by trailing slashes in URLs, ensuring proper URL formatting and preventing authentication errors. * **Tests** * Added tests to verify correct handling of trailing slashes in SSO URLs and to ensure errors are properly returned for invalid SSO URL configurations. --- changes/31545-fix-sso-trailing-slash | 1 + ee/server/service/mdm.go | 8 +- server/mdm/apple/apple_mdm.go | 6 +- server/service/sessions.go | 32 ++++--- server/service/sessions_test.go | 131 ++++++++++++++++++++++++--- 5 files changed, 148 insertions(+), 30 deletions(-) create mode 100644 changes/31545-fix-sso-trailing-slash diff --git a/changes/31545-fix-sso-trailing-slash b/changes/31545-fix-sso-trailing-slash new file mode 100644 index 0000000000..05760186db --- /dev/null +++ b/changes/31545-fix-sso-trailing-slash @@ -0,0 +1 @@ +- Fixed an issue where SSO URLs with trailing slashes would cause authentication failures due to double slashes in the ACS URL. Both regular SSO and MDM SSO URLs now properly handle trailing slashes. diff --git a/ee/server/service/mdm.go b/ee/server/service/mdm.go index df90777ec9..f2c4bbd53d 100644 --- a/ee/server/service/mdm.go +++ b/ee/server/service/mdm.go @@ -710,7 +710,13 @@ func (svc *Service) InitiateMDMAppleSSO(ctx context.Context, initiator string) ( } serverURL := appConfig.MDMUrl() - acsURL := serverURL + svc.config.Server.URLPrefix + "/api/v1/fleet/mdm/sso/callback" + // Parse the URL and use JoinPath to avoid double slashes + parsedURL, err := url.Parse(serverURL) + if err != nil { + return "", 0, "", ctxerr.Wrap(ctx, err, "invalid MDM URL") + } + parsedURL = parsedURL.JoinPath(svc.config.Server.URLPrefix, "/api/v1/fleet/mdm/sso/callback") + acsURL := parsedURL.String() samlProvider, err := sso.SAMLProviderFromConfiguredMetadata(ctx, mdmSSOSettings.EntityID, diff --git a/server/mdm/apple/apple_mdm.go b/server/mdm/apple/apple_mdm.go index 276d06eaef..4664aaf7cf 100644 --- a/server/mdm/apple/apple_mdm.go +++ b/server/mdm/apple/apple_mdm.go @@ -171,7 +171,11 @@ func (d *DEPService) buildJSONProfile(ctx context.Context, setupAsstJSON json.Ra endUserAuthEnabled = team.Config.MDM.MacOSSetup.EnableEndUserAuthentication } if endUserAuthEnabled { - jsonProf.ConfigurationWebURL = appCfg.MDMUrl() + "/mdm/sso" + mdmSSOURL, err := commonmdm.ResolveURL(appCfg.MDMUrl(), "/mdm/sso", false) + if err != nil { + return nil, fmt.Errorf("resolve MDM SSO URL: %w", err) + } + jsonProf.ConfigurationWebURL = mdmSSOURL } } diff --git a/server/service/sessions.go b/server/service/sessions.go index 9a4ca571d5..8aa8c37bbc 100644 --- a/server/service/sessions.go +++ b/server/service/sessions.go @@ -446,7 +446,13 @@ func (svc *Service) InitiateSSO(ctx context.Context, redirectURL string) (sessio if appConfig.SSOSettings != nil && appConfig.SSOSettings.SSOServerURL != "" { ssoURL = appConfig.SSOSettings.SSOServerURL } - acsURL := ssoURL + svc.config.Server.URLPrefix + "/api/v1/fleet/sso/callback" + // Parse the URL and use JoinPath to avoid double slashes + parsedURL, err := url.Parse(ssoURL) + if err != nil { + return "", 0, "", ctxerr.Wrap(ctx, badRequest("invalid SSO URL: "+err.Error())) + } + parsedURL = parsedURL.JoinPath(svc.config.Server.URLPrefix, "/api/v1/fleet/sso/callback") + acsURL := parsedURL.String() // If entityID is not explicitly set, default to host name. // @@ -660,32 +666,30 @@ func (svc *Service) InitSSOCallback( if appConfig.SSOSettings != nil && appConfig.SSOSettings.SSOServerURL != "" { ssoURL = appConfig.SSOSettings.SSOServerURL } - acsURL, err := url.Parse(ssoURL + svc.config.Server.URLPrefix + "/api/v1/fleet/sso/callback") + // Parse the URL and use JoinPath to avoid double slashes + parsedURL, err := url.Parse(ssoURL) if err != nil { - return nil, "", ctxerr.Wrap(ctx, err, "failed to parse ACS URL") + return nil, "", ctxerr.Wrap(ctx, newSSOError(err, ssoOtherError), "invalid SSO URL") } + baseSSO := parsedURL.String() + + // Now construct the ACS URL + parsedURL = parsedURL.JoinPath(svc.config.Server.URLPrefix, "/api/v1/fleet/sso/callback") expectedAudiences := []string{ appConfig.SSOSettings.EntityID, - appConfig.ServerSettings.ServerURL, - appConfig.ServerSettings.ServerURL + svc.config.Server.URLPrefix + "/api/v1/fleet/sso/callback", // ACS with server URL - } - // Add SSO server URL to expected audiences if configured - if appConfig.SSOSettings != nil && appConfig.SSOSettings.SSOServerURL != "" { - expectedAudiences = append(expectedAudiences, - appConfig.SSOSettings.SSOServerURL, - appConfig.SSOSettings.SSOServerURL+svc.config.Server.URLPrefix+"/api/v1/fleet/sso/callback", // ACS with SSO server URL - ) + baseSSO, // Base SSO URL + parsedURL.String(), // Use the already-constructed ACS URL } samlProvider, requestID, redirectURL, err := sso.SAMLProviderFromSessionOrConfiguredMetadata( - ctx, sessionID, svc.ssoSessionStore, acsURL, appConfig.SSOSettings, expectedAudiences, + ctx, sessionID, svc.ssoSessionStore, parsedURL, appConfig.SSOSettings, expectedAudiences, ) if err != nil { return nil, "", ctxerr.Wrap(ctx, err, "failed to create provider from metadata") } // Parse and verify SAMLResponse (verifies fields, expected IDs and signature). - auth, err = sso.ParseAndVerifySAMLResponse(samlProvider, samlResponse, requestID, acsURL) + auth, err = sso.ParseAndVerifySAMLResponse(samlProvider, samlResponse, requestID, parsedURL) if err != nil { // We actually don't return 401 to clients and instead return an HTML page with /login?status=error, // but to be consistent we will return fleet.AuthFailedError which is used for unauthorized access. diff --git a/server/service/sessions_test.go b/server/service/sessions_test.go index f8de66e61c..e0163940ed 100644 --- a/server/service/sessions_test.go +++ b/server/service/sessions_test.go @@ -17,6 +17,23 @@ import ( "github.com/stretchr/testify/require" ) +// testSSOMetadata returns a valid SAML metadata XML for testing +func testSSOMetadata() string { + return ` + + + + + + MIIDXTCCAkWgAwIBAgIJALmVVuDWu4NYMA0GCSqGSIb3DQEBCwUAMEUxCzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQwHhcNMTYxMjMxMTQzNDQ3WhcNNDgwNjI1MTQzNDQ3WjBFMQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzUCFozgNb1h1M0jzNRSCjhOBnR+uVbVpaWfXYIR+AhWDdEe5ryY+CgavOg8bfLybyzFdehlYdDRgkedEB/GjG8aJw06l0qF4jDOAw0kEygWCu2mcH7XOxRt+YAH3TVHa/Hu1W3WjzkobqqqLQ8gkKWWM27fOgAZ6GieaJBN6VBSMMcPey3HWLBmc+TYJmv1dbaO2jHhKh8pfKw0W12VM8P1PIO8gv4Phu/uuJYieBWKixBEyy0lHjyixYFCR12xdh4CA47q958ZRGnnDUGFVE1QhgRacJCOZ9bd5t9mr8KLaVBYTCJo5ERE8jymab5dPqe5qKfJsCZiqWglbjUo9twIDAQABo1AwTjAdBgNVHQ4EFgQUxpuwcs/CYQOyui+r1G+3KxBNhxkwHwYDVR0jBBgwFoAUxpuwcs/CYQOyui+r1G+3KxBNhxkwDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAAiWUKs/2x/viNCKi3Y6blEuCtAGhzOOZ9EjrvJ8+COH3Rag3tVBWrcBZ3/uhhPq5gy9lqw4OkvEws99/5jFsX1FJ6MKBgqfuy7yh5s1YfM0ANHYczMmYpZeAcQf2CGAaVfwTTfSlzNLsF2lW/ly7yapFzlYSJLGoVE+OHEu8g5SlNACUEfkXw+5Eghh+KzlIN7R6Q7r2ixWNFBC/jWf7NKUfJyX8qIG5md1YUeT6GBW9Bm2/1/RiO24JTaYlfLdKK9TYb8sG5B+OLab2DImG99CJ25RkAcSobWNF5zD0O6lgOo3cEdB/ksCq3hmtlC/DlLZ/D8CJ+7VuZnS1rR2naQ== + + + + + +` +} + func TestSessionAuth(t *testing.T) { ds := new(mock.Store) svc, ctx := newTestService(t, ds, nil, nil) @@ -440,7 +457,7 @@ func TestGetSSOUser(t *testing.T) { func TestInitiateSSOWithSSOServerURL(t *testing.T) { ds := new(mock.Store) - pool := redistest.SetupRedis(t, t.Name(), false, false, false) + pool := redistest.NopRedis() svc, ctx := newTestServiceWithConfig(t, ds, config.TestConfig(), nil, nil, &TestServerOpts{ Pool: pool, @@ -457,19 +474,7 @@ func TestInitiateSSOWithSSOServerURL(t *testing.T) { SSOProviderSettings: fleet.SSOProviderSettings{ EntityID: "fleet", IDPName: "TestIDP", - Metadata: ` - - - - - - MIIDXTCCAkWgAwIBAgIJALmVVuDWu4NYMA0GCSqGSIb3DQEBCwUAMEUxCzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQwHhcNMTYxMjMxMTQzNDQ3WhcNNDgwNjI1MTQzNDQ3WjBFMQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UECgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzUCFozgNb1h1M0jzNRSCjhOBnR+uVbVpaWfXYIR+AhWDdEe5ryY+CgavOg8bfLybyzFdehlYdDRgkedEB/GjG8aJw06l0qF4jDOAw0kEygWCu2mcH7XOxRt+YAH3TVHa/Hu1W3WjzkobqqqLQ8gkKWWM27fOgAZ6GieaJBN6VBSMMcPey3HWLBmc+TYJmv1dbaO2jHhKh8pfKw0W12VM8P1PIO8gv4Phu/uuJYieBWKixBEyy0lHjyixYFCR12xdh4CA47q958ZRGnnDUGFVE1QhgRacJCOZ9bd5t9mr8KLaVBYTCJo5ERE8jymab5dPqe5qKfJsCZiqWglbjUo9twIDAQABo1AwTjAdBgNVHQ4EFgQUxpuwcs/CYQOyui+r1G+3KxBNhxkwHwYDVR0jBBgwFoAUxpuwcs/CYQOyui+r1G+3KxBNhxkwDAYDVR0TBAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAAiWUKs/2x/viNCKi3Y6blEuCtAGhzOOZ9EjrvJ8+COH3Rag3tVBWrcBZ3/uhhPq5gy9lqw4OkvEws99/5jFsX1FJ6MKBgqfuy7yh5s1YfM0ANHYczMmYpZeAcQf2CGAaVfwTTfSlzNLsF2lW/ly7yapFzlYSJLGoVE+OHEu8g5SlNACUEfkXw+5Eghh+KzlIN7R6Q7r2ixWNFBC/jWf7NKUfJyX8qIG5md1YUeT6GBW9Bm2/1/RiO24JTaYlfLdKK9TYb8sG5B+OLab2DImG99CJ25RkAcSobWNF5zD0O6lgOo3cEdB/ksCq3hmtlC/DlLZ/D8CJ+7VuZnS1rR2naQ== - - - - - -`, + Metadata: testSSOMetadata(), }, }, } @@ -488,3 +493,101 @@ func TestInitiateSSOWithSSOServerURL(t *testing.T) { // We can't directly test the ACS URL in the SAML request here since it's embedded in the XML, // but the integration test verifies this works correctly } + +func TestInitiateSSOWithTrailingSlash(t *testing.T) { + ds := new(mock.Store) + pool := redistest.NopRedis() + + svc, ctx := newTestServiceWithConfig(t, ds, config.TestConfig(), nil, nil, &TestServerOpts{ + Pool: pool, + }) + + testCases := []struct { + name string + serverURL string + ssoServerURL string + }{ + { + name: "server URL with trailing slash", + serverURL: "https://fleet.example.com/", + ssoServerURL: "", + }, + { + name: "SSO server URL with trailing slash", + serverURL: "https://fleet.example.com", + ssoServerURL: "https://admin.fleet.example.com/", + }, + { + name: "both URLs with trailing slash", + serverURL: "https://fleet.example.com/", + ssoServerURL: "https://admin.fleet.example.com/", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Mock app config + appConfig := &fleet.AppConfig{ + ServerSettings: fleet.ServerSettings{ + ServerURL: tc.serverURL, + }, + SSOSettings: &fleet.SSOSettings{ + EnableSSO: true, + SSOServerURL: tc.ssoServerURL, + SSOProviderSettings: fleet.SSOProviderSettings{ + EntityID: "fleet", + IDPName: "TestIDP", + Metadata: testSSOMetadata(), + }, + }, + } + + ds.AppConfigFunc = func(_ context.Context) (*fleet.AppConfig, error) { + return appConfig, nil + } + + // Test that InitiateSSO works + sessionID, _, idpURL, err := svc.InitiateSSO(ctx, "/dashboard") + require.NoError(t, err) + require.NotEmpty(t, sessionID) + require.NotEmpty(t, idpURL) + }) + } +} + +func TestInitiateSSOWithInvalidURL(t *testing.T) { + ds := new(mock.Store) + pool := redistest.NopRedis() + + svc, ctx := newTestServiceWithConfig(t, ds, config.TestConfig(), nil, nil, &TestServerOpts{ + Pool: pool, + }) + + // Mock app config with invalid URL + appConfig := &fleet.AppConfig{ + ServerSettings: fleet.ServerSettings{ + ServerURL: "not-a-valid-url://%%%", + }, + SSOSettings: &fleet.SSOSettings{ + EnableSSO: true, + SSOProviderSettings: fleet.SSOProviderSettings{ + EntityID: "fleet", + IDPName: "TestIDP", + Metadata: testSSOMetadata(), + }, + }, + } + + ds.AppConfigFunc = func(_ context.Context) (*fleet.AppConfig, error) { + return appConfig, nil + } + + // Test that invalid URL returns bad request error + _, _, _, err := svc.InitiateSSO(ctx, "/dashboard") + require.Error(t, err) + + // Verify it's a bad request error + var badReqErr *fleet.BadRequestError + require.ErrorAs(t, err, &badReqErr) + require.Contains(t, badReqErr.Message, "invalid SSO URL") +}