diff --git a/lib/services/local/saml_idp_service_provider.go b/lib/services/local/saml_idp_service_provider.go index f719044ec9b46..21a0ea4e8ec9b 100644 --- a/lib/services/local/saml_idp_service_provider.go +++ b/lib/services/local/saml_idp_service_provider.go @@ -126,7 +126,7 @@ func (s *SAMLIdPServiceProviderService) CreateSAMLIdPServiceProvider(ctx context // verify that entity descriptor parses ed, err := samlsp.ParseMetadata([]byte(sp.GetEntityDescriptor())) if err != nil { - return trace.BadParameter("invalid entity descriptor for SAML IdP Service provider %q: %v", sp.GetEntityID(), err) + return trace.BadParameter("invalid entity descriptor for SAML IdP Service Provider %q: %v", sp.GetEntityID(), err) } if ed.EntityID != sp.GetEntityID() { @@ -134,8 +134,8 @@ func (s *SAMLIdPServiceProviderService) CreateSAMLIdPServiceProvider(ctx context } // ensure any filtering related issues get logged - if err := services.FilterSAMLEntityDescriptor(ed); err != nil { - s.log.Warnf("Entity descriptor for SAML IdP service provider %q may be malformed: %v", sp.GetEntityID(), err) + if err := services.FilterSAMLEntityDescriptor(ed, false /* quiet */); err != nil { + s.log.Warnf("Entity descriptor for SAML IdP Service Provider %q contains unsupported ACS bindings: %v", sp.GetEntityID(), err) } // embed attribute mapping in entity descriptor @@ -167,7 +167,7 @@ func (s *SAMLIdPServiceProviderService) UpdateSAMLIdPServiceProvider(ctx context // verify that entity descriptor parses ed, err := samlsp.ParseMetadata([]byte(sp.GetEntityDescriptor())) if err != nil { - return trace.BadParameter("invalid entity descriptor for SAML IdP Service provider %q: %v", sp.GetEntityID(), err) + return trace.BadParameter("invalid entity descriptor for SAML IdP Service Provider %q: %v", sp.GetEntityID(), err) } if ed.EntityID != sp.GetEntityID() { @@ -175,8 +175,8 @@ func (s *SAMLIdPServiceProviderService) UpdateSAMLIdPServiceProvider(ctx context } // ensure any filtering related issues get logged - if err := services.FilterSAMLEntityDescriptor(ed); err != nil { - s.log.Warnf("Entity descriptor for SAML IdP service provider %q may be malformed: %v", sp.GetEntityID(), err) + if err := services.FilterSAMLEntityDescriptor(ed, false /* quiet */); err != nil { + s.log.Warnf("Entity descriptor for SAML IdP Service Provider %q contains unsupported ACS bindings: %v", sp.GetEntityID(), err) } // embed attribute mapping in entity descriptor @@ -290,12 +290,19 @@ func (s *SAMLIdPServiceProviderService) generateAndSetEntityDescriptor(sp types. AuthnNameIDFormat: saml.UnspecifiedNameIDFormat, } - entityDescriptor, err := xml.MarshalIndent(newServiceProvider.Metadata(), "", " ") + ed := newServiceProvider.Metadata() + // HTTPArtifactBinding is defined when entity descriptor is generated + // using crewjam/saml https://github.com/crewjam/saml/blob/main/service_provider.go#L228. + // But we do not support it, so filter it out below. + // Error and warnings are swallowed because the descriptor is Teleport generated and + // users have no control over sanitizing filtered binding. + services.FilterSAMLEntityDescriptor(ed, true /* quiet */) + edXMLBytes, err := xml.MarshalIndent(ed, "", " ") if err != nil { return trace.Wrap(err) } - sp.SetEntityDescriptor(string(entityDescriptor)) + sp.SetEntityDescriptor(string(edXMLBytes)) return nil } diff --git a/lib/services/local/saml_idp_service_provider_test.go b/lib/services/local/saml_idp_service_provider_test.go index 69bcc73dbb822..dbf0e88af378f 100644 --- a/lib/services/local/saml_idp_service_provider_test.go +++ b/lib/services/local/saml_idp_service_provider_test.go @@ -209,7 +209,7 @@ const testEntityDescriptor = ` ` func newSAMLSPMetadata(entityID, acsURL string) string { - return fmt.Sprintf(samlSPMetadata, entityID, acsURL, acsURL) + return fmt.Sprintf(samlSPMetadata, entityID, acsURL) } // samlSPMetadata mimics metadata generated by saml.ServiceProvider.Metadata() @@ -217,7 +217,6 @@ const samlSPMetadata = ` urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified - ` diff --git a/lib/services/saml_idp_service_provider.go b/lib/services/saml_idp_service_provider.go index cccd874eaf435..5a7fb1aba1dc1 100644 --- a/lib/services/saml_idp_service_provider.go +++ b/lib/services/saml_idp_service_provider.go @@ -150,13 +150,15 @@ func ValidateAssertionConsumerService(acs saml.IndexedEndpoint) error { // or are using a non-https endpoint. We perform filtering rather than outright rejection because it is generally // expected that a service provider will successfully support a given ACS so long as they have at least one // compatible binding. -func FilterSAMLEntityDescriptor(ed *saml.EntityDescriptor) error { +func FilterSAMLEntityDescriptor(ed *saml.EntityDescriptor, quiet bool) error { var originalCount int var filteredCount int for i := range ed.SPSSODescriptors { filtered := slices.DeleteFunc(ed.SPSSODescriptors[i].AssertionConsumerServices, func(acs saml.IndexedEndpoint) bool { if err := ValidateAssertionConsumerService(acs); err != nil { - log.Warnf("AssertionConsumerService binding for entity %q is invalid and will be ignored: %v", ed.EntityID, err) + if !quiet { + log.Warnf("AssertionConsumerService binding for entity %q is invalid and will be ignored: %v", ed.EntityID, err) + } return true } diff --git a/lib/services/saml_idp_service_provider_test.go b/lib/services/saml_idp_service_provider_test.go index bb45c704a082b..4983b377c45a5 100644 --- a/lib/services/saml_idp_service_provider_test.go +++ b/lib/services/saml_idp_service_provider_test.go @@ -97,16 +97,17 @@ func TestFilterSAMLEntityDescriptor(t *testing.T) { { eds: edBuilder(). ACS(saml.HTTPArtifactBinding, "https://example.com/acs"). + ACS("urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST-SimpleSign", "https://example.com/POST-SimpleSign"). ACS(saml.HTTPPostBinding, "https://example.com/acs"). Done(), ok: true, - before: 2, + before: 3, after: 1, name: "binding filtering", }, { eds: edBuilder(). - ACS(saml.HTTPArtifactBinding, "https://example.com/acs"). + ACS("urn:oasis:names:tc:SAML:2.0:bindings:PAOS", "https://example.com/ECP"). ACS(saml.HTTPPostBinding, "http://example.com/acs"). Done(), ok: false, @@ -123,7 +124,7 @@ func TestFilterSAMLEntityDescriptor(t *testing.T) { require.Equal(t, tt.before, getACSCount(ed)) - err = FilterSAMLEntityDescriptor(ed) + err = FilterSAMLEntityDescriptor(ed, false /* quiet */) if !tt.ok { require.Error(t, err) return diff --git a/tool/tctl/common/resource_command.go b/tool/tctl/common/resource_command.go index afa8b6ebfbc33..067465d3ba0b8 100644 --- a/tool/tctl/common/resource_command.go +++ b/tool/tctl/common/resource_command.go @@ -999,16 +999,17 @@ func (rc *ResourceCommand) createSAMLIdPServiceProvider(ctx context.Context, cli return trace.Wrap(err) } - // verify that entity descriptor parses - ed, err := samlsp.ParseMetadata([]byte(sp.GetEntityDescriptor())) - if err != nil { - return trace.BadParameter("invalid entity descriptor for SAML IdP Service provider %q: %v", sp.GetEntityID(), err) - } + if sp.GetEntityDescriptor() != "" { + // verify that entity descriptor parses + ed, err := samlsp.ParseMetadata([]byte(sp.GetEntityDescriptor())) + if err != nil { + return trace.BadParameter("invalid entity descriptor for SAML IdP Service Provider %q: %v", sp.GetEntityID(), err) + } - // try filtering the entity descriptor. if it can't be filtered down to a useable looking state, reject - // the creation attempt. - if err := services.FilterSAMLEntityDescriptor(ed); err != nil { - return trace.Wrap(err) + // issue warning about unsupported ACS bindings. + if err := services.FilterSAMLEntityDescriptor(ed, false /* quiet */); err != nil { + log.Warnf("Entity descriptor for SAML IdP service provider %q contains unsupported ACS bindings: %v", sp.GetEntityID(), err) + } } serviceProviderName := sp.GetName()