mirror of
https://github.com/fleetdm/fleet
synced 2026-05-23 17:08:53 +00:00
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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
parent
ae4ccb8e3f
commit
030e292f30
5 changed files with 148 additions and 30 deletions
1
changes/31545-fix-sso-trailing-slash
Normal file
1
changes/31545-fix-sso-trailing-slash
Normal file
|
|
@ -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.
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -17,6 +17,23 @@ import (
|
|||
"github.com/stretchr/testify/require"
|
||||
)
|
||||
|
||||
// testSSOMetadata returns a valid SAML metadata XML for testing
|
||||
func testSSOMetadata() string {
|
||||
return `<?xml version="1.0"?>
|
||||
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="test-idp">
|
||||
<md:IDPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
|
||||
<md:KeyDescriptor use="signing">
|
||||
<ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
|
||||
<ds:X509Data>
|
||||
<ds:X509Certificate>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==</ds:X509Certificate>
|
||||
</ds:X509Data>
|
||||
</ds:KeyInfo>
|
||||
</md:KeyDescriptor>
|
||||
<md:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://idp.example.com/sso"/>
|
||||
</md:IDPSSODescriptor>
|
||||
</md:EntityDescriptor>`
|
||||
}
|
||||
|
||||
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: `<?xml version="1.0"?>
|
||||
<md:EntityDescriptor xmlns:md="urn:oasis:names:tc:SAML:2.0:metadata" entityID="test-idp">
|
||||
<md:IDPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
|
||||
<md:KeyDescriptor use="signing">
|
||||
<ds:KeyInfo xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
|
||||
<ds:X509Data>
|
||||
<ds:X509Certificate>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==</ds:X509Certificate>
|
||||
</ds:X509Data>
|
||||
</ds:KeyInfo>
|
||||
</md:KeyDescriptor>
|
||||
<md:SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://idp.example.com/sso"/>
|
||||
</md:IDPSSODescriptor>
|
||||
</md:EntityDescriptor>`,
|
||||
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")
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue