From 35df5f61fcd9973c6dcb3e922a87b9cfdac7ae8d Mon Sep 17 00:00:00 2001 From: Livio Spring Date: Tue, 3 Dec 2024 11:38:28 +0100 Subject: [PATCH] fix(saml): improve error handling (#8928) # Which Problems Are Solved There are multiple issues with the metadata and error handling of SAML: - When providing a SAML metadata for an IdP, which cannot be processed, the error will only be noticed once a user tries to use the IdP. - Parsing for metadata with any other encoding than UTF-8 fails. - Metadata containing an enclosing EntitiesDescriptor around EntityDescriptor cannot be parsed. - Metadata's `validUntil` value is always set to 48 hours, which causes issues on external providers, if processed from a manual down/upload. - If a SAML response cannot be parsed, only a generic "Authentication failed" error is returned, the cause is hidden to the user and also to actions. # How the Problems Are Solved - Return parsing errors after create / update and retrieval of an IdP in the API. - Prevent the creation and update of an IdP in case of a parsing failure. - Added decoders for encodings other than UTF-8 (including ASCII, windows and ISO, [currently supported](https://github.com/golang/text/blob/efd25daf282ae4d20d3625f1ccb4452fe40967ae/encoding/ianaindex/ianaindex.go#L156)) - Updated parsing to handle both `EntitiesDescriptor` and `EntityDescriptor` as root element - `validUntil` will automatically set to the certificate's expiration time - Unwrapped the hidden error to be returned. The Login UI will still only provide a mostly generic error, but action can now access the underlying error. # Additional Changes None # Additional Context reported by a customer (cherry picked from commit ffe95707769abde4ffffa7fde62fe957adf24ab1) --- internal/api/grpc/admin/idp_converter.go | 8 +- internal/api/grpc/management/idp_converter.go | 8 +- internal/command/instance_idp.go | 17 +- internal/command/instance_idp_test.go | 151 +++++++++++++++--- internal/command/org_idp.go | 17 +- internal/command/org_idp_test.go | 86 +++++++--- internal/idp/providers/saml/saml.go | 44 ++++- internal/idp/providers/saml/saml_test.go | 110 +++++++++++++ internal/idp/providers/saml/session.go | 3 +- internal/idp/providers/saml/session_test.go | 2 +- 10 files changed, 377 insertions(+), 69 deletions(-) diff --git a/internal/api/grpc/admin/idp_converter.go b/internal/api/grpc/admin/idp_converter.go index 2914f94b30..67e40a44ab 100644 --- a/internal/api/grpc/admin/idp_converter.go +++ b/internal/api/grpc/admin/idp_converter.go @@ -469,12 +469,12 @@ func updateAppleProviderToCommand(req *admin_pb.UpdateAppleProviderRequest) comm } } -func addSAMLProviderToCommand(req *admin_pb.AddSAMLProviderRequest) command.SAMLProvider { +func addSAMLProviderToCommand(req *admin_pb.AddSAMLProviderRequest) *command.SAMLProvider { var nameIDFormat *domain.SAMLNameIDFormat if req.NameIdFormat != nil { nameIDFormat = gu.Ptr(idp_grpc.SAMLNameIDFormatToDomain(req.GetNameIdFormat())) } - return command.SAMLProvider{ + return &command.SAMLProvider{ Name: req.Name, Metadata: req.GetMetadataXml(), MetadataURL: req.GetMetadataUrl(), @@ -486,12 +486,12 @@ func addSAMLProviderToCommand(req *admin_pb.AddSAMLProviderRequest) command.SAML } } -func updateSAMLProviderToCommand(req *admin_pb.UpdateSAMLProviderRequest) command.SAMLProvider { +func updateSAMLProviderToCommand(req *admin_pb.UpdateSAMLProviderRequest) *command.SAMLProvider { var nameIDFormat *domain.SAMLNameIDFormat if req.NameIdFormat != nil { nameIDFormat = gu.Ptr(idp_grpc.SAMLNameIDFormatToDomain(req.GetNameIdFormat())) } - return command.SAMLProvider{ + return &command.SAMLProvider{ Name: req.Name, Metadata: req.GetMetadataXml(), MetadataURL: req.GetMetadataUrl(), diff --git a/internal/api/grpc/management/idp_converter.go b/internal/api/grpc/management/idp_converter.go index 48d5a85a99..ef3914cc96 100644 --- a/internal/api/grpc/management/idp_converter.go +++ b/internal/api/grpc/management/idp_converter.go @@ -462,12 +462,12 @@ func updateAppleProviderToCommand(req *mgmt_pb.UpdateAppleProviderRequest) comma } } -func addSAMLProviderToCommand(req *mgmt_pb.AddSAMLProviderRequest) command.SAMLProvider { +func addSAMLProviderToCommand(req *mgmt_pb.AddSAMLProviderRequest) *command.SAMLProvider { var nameIDFormat *domain.SAMLNameIDFormat if req.NameIdFormat != nil { nameIDFormat = gu.Ptr(idp_grpc.SAMLNameIDFormatToDomain(req.GetNameIdFormat())) } - return command.SAMLProvider{ + return &command.SAMLProvider{ Name: req.Name, Metadata: req.GetMetadataXml(), MetadataURL: req.GetMetadataUrl(), @@ -479,12 +479,12 @@ func addSAMLProviderToCommand(req *mgmt_pb.AddSAMLProviderRequest) command.SAMLP } } -func updateSAMLProviderToCommand(req *mgmt_pb.UpdateSAMLProviderRequest) command.SAMLProvider { +func updateSAMLProviderToCommand(req *mgmt_pb.UpdateSAMLProviderRequest) *command.SAMLProvider { var nameIDFormat *domain.SAMLNameIDFormat if req.NameIdFormat != nil { nameIDFormat = gu.Ptr(idp_grpc.SAMLNameIDFormatToDomain(req.GetNameIdFormat())) } - return command.SAMLProvider{ + return &command.SAMLProvider{ Name: req.Name, Metadata: req.GetMetadataXml(), MetadataURL: req.GetMetadataUrl(), diff --git a/internal/command/instance_idp.go b/internal/command/instance_idp.go index c3940c007a..99ab506424 100644 --- a/internal/command/instance_idp.go +++ b/internal/command/instance_idp.go @@ -11,6 +11,7 @@ import ( "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" + "github.com/zitadel/zitadel/internal/idp/providers/saml" "github.com/zitadel/zitadel/internal/repository/instance" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -511,10 +512,10 @@ func (c *Commands) UpdateInstanceAppleProvider(ctx context.Context, id string, p return pushedEventsToObjectDetails(pushedEvents), nil } -func (c *Commands) AddInstanceSAMLProvider(ctx context.Context, provider SAMLProvider) (string, *domain.ObjectDetails, error) { +func (c *Commands) AddInstanceSAMLProvider(ctx context.Context, provider *SAMLProvider) (id string, details *domain.ObjectDetails, err error) { instanceID := authz.GetInstance(ctx).InstanceID() instanceAgg := instance.NewAggregate(instanceID) - id, err := c.idGenerator.Next() + id, err = c.idGenerator.Next() if err != nil { return "", nil, err } @@ -530,7 +531,7 @@ func (c *Commands) AddInstanceSAMLProvider(ctx context.Context, provider SAMLPro return id, pushedEventsToObjectDetails(pushedEvents), nil } -func (c *Commands) UpdateInstanceSAMLProvider(ctx context.Context, id string, provider SAMLProvider) (*domain.ObjectDetails, error) { +func (c *Commands) UpdateInstanceSAMLProvider(ctx context.Context, id string, provider *SAMLProvider) (details *domain.ObjectDetails, err error) { instanceID := authz.GetInstance(ctx).InstanceID() instanceAgg := instance.NewAggregate(instanceID) writeModel := NewSAMLInstanceIDPWriteModel(instanceID, id) @@ -1719,7 +1720,7 @@ func (c *Commands) prepareUpdateInstanceAppleProvider(a *instance.Aggregate, wri } } -func (c *Commands) prepareAddInstanceSAMLProvider(a *instance.Aggregate, writeModel *InstanceSAMLIDPWriteModel, provider SAMLProvider) preparation.Validation { +func (c *Commands) prepareAddInstanceSAMLProvider(a *instance.Aggregate, writeModel *InstanceSAMLIDPWriteModel, provider *SAMLProvider) preparation.Validation { return func() (preparation.CreateCommands, error) { if provider.Name = strings.TrimSpace(provider.Name); provider.Name == "" { return nil, zerrors.ThrowInvalidArgument(nil, "INST-o07zjotgnd", "Errors.Invalid.Argument") @@ -1734,6 +1735,9 @@ func (c *Commands) prepareAddInstanceSAMLProvider(a *instance.Aggregate, writeMo if len(provider.Metadata) == 0 { return nil, zerrors.ThrowInvalidArgument(nil, "INST-3bi3esi16t", "Errors.Invalid.Argument") } + if _, err := saml.ParseMetadata(provider.Metadata); err != nil { + return nil, zerrors.ThrowInvalidArgument(err, "INST-SF3rwhgh", "Errors.Project.App.SAMLMetadataFormat") + } return func(ctx context.Context, filter preparation.FilterToQueryReducer) ([]eventstore.Command, error) { events, err := filter(ctx, writeModel.Query()) if err != nil { @@ -1772,7 +1776,7 @@ func (c *Commands) prepareAddInstanceSAMLProvider(a *instance.Aggregate, writeMo } } -func (c *Commands) prepareUpdateInstanceSAMLProvider(a *instance.Aggregate, writeModel *InstanceSAMLIDPWriteModel, provider SAMLProvider) preparation.Validation { +func (c *Commands) prepareUpdateInstanceSAMLProvider(a *instance.Aggregate, writeModel *InstanceSAMLIDPWriteModel, provider *SAMLProvider) preparation.Validation { return func() (preparation.CreateCommands, error) { if writeModel.ID = strings.TrimSpace(writeModel.ID); writeModel.ID == "" { return nil, zerrors.ThrowInvalidArgument(nil, "INST-7o3rq1owpm", "Errors.Invalid.Argument") @@ -1790,6 +1794,9 @@ func (c *Commands) prepareUpdateInstanceSAMLProvider(a *instance.Aggregate, writ } provider.Metadata = data } + if _, err := saml.ParseMetadata(provider.Metadata); err != nil { + return nil, zerrors.ThrowInvalidArgument(err, "INST-dsfj3kl2", "Errors.Project.App.SAMLMetadataFormat") + } return func(ctx context.Context, filter preparation.FilterToQueryReducer) ([]eventstore.Command, error) { events, err := filter(ctx, writeModel.Query()) if err != nil { diff --git a/internal/command/instance_idp_test.go b/internal/command/instance_idp_test.go index c6181af9f1..22defda532 100644 --- a/internal/command/instance_idp_test.go +++ b/internal/command/instance_idp_test.go @@ -22,6 +22,73 @@ import ( "github.com/zitadel/zitadel/internal/zerrors" ) +var ( + validSAMLMetadata = []byte(` + + + + + + + + + + + + Tyw4csdpNNq0E7wi5FXWdVNkdPNg+cM6kK21VB2+iF0= + + + hWQSYmnBJENy/okk2qRDuHaZiyqpDsdV6BF9/T/LNjUh/8z4dV2NEZvkNhFEyQ+bqdj+NmRWvKqpg1dtgNJxQc32+IsLQvXNYyhMCtyG570/jaTOtm8daV4NKJyTV7SdwM6yfXgubz5YCRTyV13W2gBIFYppIRImIv5NDcjz+lEmWhnrkw8G2wRSFUY7VvkDn9rgsTzw/Pnsw6hlzpjGDYPMPx3ux3kjFVevdhFGNo+VC7t9ozruuGyH3yue9Re6FZoqa4oyWaPSOwei0ZH6UNqkX93Eo5Y49QKwaO8Rm+kWsOhdTqebVmCc+SpWbbrZbQj4nSLgWGlvCkZSivmH7ezr4Ol1ZkRetQ92UQ7xJS7E0y6uXAGvdgpDnyqHCOFfhTS6yqltHtc3m7JZex327xkv6e69uAEOSiv++sifVUIE0h/5u3hZLvwmTPrkoRVY4wgZ4ieb86QPvhw4UPeYapOhCBk5RfjoEFIeYwPUw5rtOlpTyeBJiKMpH1+mDAoa+8HQytZoMrnnY1s612vINtY7jU5igMwIk6MitQpRGibnBVBHRc2A6aE+XS333ganFK9hX6TzNkpHUb66NINDZ8Rgb1thn3MABArGlomtM5/enrAixWExZp70TSElor7SBdBW57H7OZCYUCobZuPRDLsCO6LLKeVrbdygWeRqr/o= + + + MIIFIjCCAwqgAwIBAgICA7YwDQYJKoZIhvcNAQELBQAwLDEQMA4GA1UEChMHWklUQURFTDEYMBYGA1UEAxMPWklUQURFTCBTQU1MIENBMB4XDTI0MTEyNzEwMjc0NFoXDTI1MTEyNzE2Mjc0NFowMjEQMA4GA1UEChMHWklUQURFTDEeMBwGA1UEAxMVWklUQURFTCBTQU1MIG1ldGFkYXRhMIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEApEpYT7EjbRBp0Hw7PGCiSgUoJtwd2nwZOhGy5WZVWvraAtHzW5ih2B6UwEShjwCmRJZeKYEN9JKJbpAy2EdL/l2rm/pArVNvSQu6sN4izz5p2rd9NfHAO3/EcvYdrelWLQj8WQx6LVM282Z4wbclp8Jz1y8Ow43352hGfFVc1x8gauoNl5MAy4kdbvs8UqihqcRmEyIOWl6UwTApb+XIRSRz0Yop99Fv9ALJwfUppsx+d4j9rlRDvrQJMJz7GC/19L9INTbY0HsVEiTltdAWHwREwrpwxNJQt42p3W/zpf1mjwXd3qNNDZAr1t2POPP4SXd598kabBZ3EMWGGxFw+NYYajyjG5EFOZw09FFJn2jIcovejvigfdqem5DGPECvHefqcqHkBPGukI3RaotXpAYyAGfnV7slVytSW484IX3KloAJLICbETbFGGsGQzIDw8rUqWyaOCOttw2fVNDyRFUMHrGe1PhJ9qA1If+KCWYD0iJqF03rIEhdrvNSdQNYkRa0DdtpacQLpzQtqsUioODqX0W3uzLceJEXLBbU0ZEk8mWZM/auwMo3ycPNXDVwrb6AkUKar+sqSumUuixw7da3KF1/mynh6M2Eo4NRB16oUiyN0EYrit/RRJjsTdH+71cj0V+8KqO88cBpmm+lO6x4RM5xpOf/EwwQHivxgRkCAwEAAaNIMEYwDgYDVR0PAQH/BAQDAgWgMBMGA1UdJQQMMAoGCCsGAQUFBwMCMB8GA1UdIwQYMBaAFIzl7uckcPWldirXeOFL3rH6K8FLMA0GCSqGSIb3DQEBCwUAA4ICAQBz+7R99uX1Us9T4BB2RK3RD9K8Q5foNmxJ8GbxpOQFL8IG1DE3FqBssciJkOsKY+1+Y6eow2TgmD9MxfCY444C8k8YDDjxIcs+4dEaWMUxA6NoEy378ciy0U1E6rpYLxWYTxXmsELyODWwTrRNIiWfbBD2m0w9HYbK6QvX6IYQqYoTOJJ3WJKsMCeQ8XhQsJYNINZEq8RsERY/aikOlTWN7ax4Mkr3bfnz1euXGClExCOM6ej4m2I33i4nyYBvvRkRRZRQCfkAQ+5WFVZoVXrQHNe/Oifit7tfLaDuybcjgkzzY3o0YbczzbdV69fVoj53VpR3QQOB+PCF/VJPUMtUFPEC05yH76g24KVBiM/Ws8GaERW1AxgupHSmvTY3GSiwDXQ2NzgDxUHfRHo8rxenJdEcPlGM0DstbUONDSFGLwvGDiidUVtqj1UB4yGL26bgtmwf61G4qsTn9PJMWdRmCeeOf7fmloRxTA0EEey3bulBBHim466tWHUhgOP+g1X0iE7CnwL8aJ//CCiQOAv1O6x5RLyxrmVTehPLr1T8qvnBmxpmuYU0kfbYpO3tMVe7VLabBx0cYh7izClZKHhgEj1w4aE9tIk7nqVAwvVocT3io8RrcKixlnBrFd7RYIuF3+RsYC/kYEgnZYKAig5u2TySgGmJ7nIS24FYW68WDg== + + + + + + + urn:oasis:names:tc:SAML:2.0:profiles:attribute:basic + + + + + + + + + urn:oasis:names:tc:SAML:2.0:nameid-format:persistent + + + http://localhost:8080/saml/v2/metadata IDP signing + + MIIFIjCCAwqgAwIBAgICA7QwDQYJKoZIhvcNAQELBQAwLDEQMA4GA1UEChMHWklUQURFTDEYMBYGA1UEAxMPWklUQURFTCBTQU1MIENBMB4XDTI0MTEyNzEwMjUwMloXDTI1MTEyNzE2MjUwMlowMjEQMA4GA1UEChMHWklUQURFTDEeMBwGA1UEAxMVWklUQURFTCBTQU1MIHJlc3BvbnNlMIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEA2lUgaI6AS/9xvM9DNSWK6Ho64LpK8UIioM26QfvAfeQ/I2pgX6SwWxEbd7qv+PkJzaFTjrXSlwOmWsJYma+UsdyFClaGFRyCgY8SWxPceandC8a+hQIDS/irLd9XF33RWp0b/09HjQl+n0HZ4teUFDUd2U1mUf3XCpn0+Ho316bmi6xSW6zaMy5RsbUl01hgWj2fgapAsGAHSBphwCE3Dz/9I/UfHWQw1k2/UTgjc9uIujcza6WgOxfsKluXYIOxwNKTfmzzOJMUwXz6GRgB2jhQI29MuKOZOITA7pXq5kZKf0lSRU8zKFTMJaK4zAHQ6f877Drr8XdAHemuXGZ2JdH/Dbdwarzy3YBMCWsAYlpeEvaVAdiSpyR7fAZktNuHd39Zg00Vlj2wdc44Vk5yVssW7pv5qnVZ7JTrXX2uBYFecLAXmplQ2ph1VdSXZLEDGgjiNA2T/fBj7G4/VjsuCBZFm1I0KCJp3HWEJx5dwwhSVc5wOJEzl7fMuPYMKWH/RM6P/7LnO1ulpdmiKPa4gHzdg3hDZn42NKcVt3UYf0phtxpWMrZp/DUEeizhckrC4ed6cfGtS3CUtJEqoycrCROJ5Hy+ONHl5Aqxt+JoPU+t/XATuctfPxQVcDr0itHzo2cjh/AVTU+IC7C0oQHSS9CC8Fp58UqbtYwFtSAd7ecCAwEAAaNIMEYwDgYDVR0PAQH/BAQDAgWgMBMGA1UdJQQMMAoGCCsGAQUFBwMCMB8GA1UdIwQYMBaAFIzl7uckcPWldirXeOFL3rH6K8FLMA0GCSqGSIb3DQEBCwUAA4ICAQAp+IGZScVIbRdCq5HPjlYBPOY7UbL8ZXnlMW/HLELV9GndnULuFhnuQTIdA5dquCsk8RI1fKsScEV1rqWvHZeSo5nVbvUaPJctoD/4GACqE6F8axs1AgSOvpJMyuycjSzSh6gDM1z37Fdqc/2IRqgi7SKdDsfJpi8XW8LtErpp4kyE1rEXopsXG2fe1UH25bZpXraUqYvp61rwVUCazAtV/U7ARG5AnT0mPqzUriIPrfL+v/+2ntV/BSc8/uCqYnHbwpIwjPURCaxo1Pmm6EEkm+V/Ss4ieNwwkD2bLLLST1LoVMim7Ebfy53PEKpsznKsGlVSu0YYKUsStWQVpwhKQw0bQLCJHdpvZtZSDgS9RbSMZz+aY/fpoNx6wDvmMgtdrb3pVXZ8vPKdq9YDrGfFqP60QdZ3CuSHXCM/zX4742GgImJ4KYAcTuF1+BkGf5JLAJOUZBkfCQ/kBT5wr8+EotLxASOC6717whLBYMEG6N8osEk+LDqoJRTLqkzirJsyOHWChKK47yGkdS3HBIZfo91QrJwKpfATYziBjEnqipkTu+6jFylBIkxKTPye4b3vgcodZP8LSNVXAsMGTPNPJxzPWQ37ba4zMnYZ5iUerlaox/SNsn68DT6RajIb1A1JDq+HNFc3hQP2bzk2y5pCax8zo5swjdklnm4clfB2Lw== + + + + + + + urn:oasis:names:tc:SAML:2.0:nameid-format:persistent + urn:oasis:names:tc:SAML:2.0:profiles:attribute:basic + + + + + + + + + http://localhost:8080/saml/v2/metadata IDP signing + + MIIFIjCCAwqgAwIBAgICA7QwDQYJKoZIhvcNAQELBQAwLDEQMA4GA1UEChMHWklUQURFTDEYMBYGA1UEAxMPWklUQURFTCBTQU1MIENBMB4XDTI0MTEyNzEwMjUwMloXDTI1MTEyNzE2MjUwMlowMjEQMA4GA1UEChMHWklUQURFTDEeMBwGA1UEAxMVWklUQURFTCBTQU1MIHJlc3BvbnNlMIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEA2lUgaI6AS/9xvM9DNSWK6Ho64LpK8UIioM26QfvAfeQ/I2pgX6SwWxEbd7qv+PkJzaFTjrXSlwOmWsJYma+UsdyFClaGFRyCgY8SWxPceandC8a+hQIDS/irLd9XF33RWp0b/09HjQl+n0HZ4teUFDUd2U1mUf3XCpn0+Ho316bmi6xSW6zaMy5RsbUl01hgWj2fgapAsGAHSBphwCE3Dz/9I/UfHWQw1k2/UTgjc9uIujcza6WgOxfsKluXYIOxwNKTfmzzOJMUwXz6GRgB2jhQI29MuKOZOITA7pXq5kZKf0lSRU8zKFTMJaK4zAHQ6f877Drr8XdAHemuXGZ2JdH/Dbdwarzy3YBMCWsAYlpeEvaVAdiSpyR7fAZktNuHd39Zg00Vlj2wdc44Vk5yVssW7pv5qnVZ7JTrXX2uBYFecLAXmplQ2ph1VdSXZLEDGgjiNA2T/fBj7G4/VjsuCBZFm1I0KCJp3HWEJx5dwwhSVc5wOJEzl7fMuPYMKWH/RM6P/7LnO1ulpdmiKPa4gHzdg3hDZn42NKcVt3UYf0phtxpWMrZp/DUEeizhckrC4ed6cfGtS3CUtJEqoycrCROJ5Hy+ONHl5Aqxt+JoPU+t/XATuctfPxQVcDr0itHzo2cjh/AVTU+IC7C0oQHSS9CC8Fp58UqbtYwFtSAd7ecCAwEAAaNIMEYwDgYDVR0PAQH/BAQDAgWgMBMGA1UdJQQMMAoGCCsGAQUFBwMCMB8GA1UdIwQYMBaAFIzl7uckcPWldirXeOFL3rH6K8FLMA0GCSqGSIb3DQEBCwUAA4ICAQAp+IGZScVIbRdCq5HPjlYBPOY7UbL8ZXnlMW/HLELV9GndnULuFhnuQTIdA5dquCsk8RI1fKsScEV1rqWvHZeSo5nVbvUaPJctoD/4GACqE6F8axs1AgSOvpJMyuycjSzSh6gDM1z37Fdqc/2IRqgi7SKdDsfJpi8XW8LtErpp4kyE1rEXopsXG2fe1UH25bZpXraUqYvp61rwVUCazAtV/U7ARG5AnT0mPqzUriIPrfL+v/+2ntV/BSc8/uCqYnHbwpIwjPURCaxo1Pmm6EEkm+V/Ss4ieNwwkD2bLLLST1LoVMim7Ebfy53PEKpsznKsGlVSu0YYKUsStWQVpwhKQw0bQLCJHdpvZtZSDgS9RbSMZz+aY/fpoNx6wDvmMgtdrb3pVXZ8vPKdq9YDrGfFqP60QdZ3CuSHXCM/zX4742GgImJ4KYAcTuF1+BkGf5JLAJOUZBkfCQ/kBT5wr8+EotLxASOC6717whLBYMEG6N8osEk+LDqoJRTLqkzirJsyOHWChKK47yGkdS3HBIZfo91QrJwKpfATYziBjEnqipkTu+6jFylBIkxKTPye4b3vgcodZP8LSNVXAsMGTPNPJxzPWQ37ba4zMnYZ5iUerlaox/SNsn68DT6RajIb1A1JDq+HNFc3hQP2bzk2y5pCax8zo5swjdklnm4clfB2Lw== + + + + +`) +) + func TestCommandSide_AddInstanceGenericOAuthIDP(t *testing.T) { type fields struct { eventstore func(*testing.T) *eventstore.Eventstore @@ -5180,7 +5247,7 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { } type args struct { ctx context.Context - provider SAMLProvider + provider *SAMLProvider } type res struct { id string @@ -5201,7 +5268,7 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { }, args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), - provider: SAMLProvider{}, + provider: &SAMLProvider{}, }, res{ err: func(err error) bool { @@ -5210,14 +5277,14 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { }, }, { - "invalid metadata", + "no metadata", fields{ eventstore: expectEventstore(), idGenerator: id_mock.NewIDGeneratorExpectIDs(t, "id1"), }, args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", }, }, @@ -5227,6 +5294,25 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { }, }, }, + { + "invalid metadata, error", + fields{ + eventstore: expectEventstore(), + idGenerator: id_mock.NewIDGeneratorExpectIDs(t, "id1"), + }, + args{ + ctx: authz.WithInstanceID(context.Background(), "instance1"), + provider: &SAMLProvider{ + Name: "name", + Metadata: []byte("metadata"), + }, + }, + res{ + err: func(err error) bool { + return errors.Is(err, zerrors.ThrowInvalidArgument(nil, "INST-SF3rwhgh", "Errors.Project.App.SAMLMetadataFormat")) + }, + }, + }, { name: "ok", fields: fields{ @@ -5236,7 +5322,7 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { instance.NewSAMLIDPAddedEvent(context.Background(), &instance.NewAggregate("instance1").Aggregate, "id1", "name", - []byte("metadata"), + validSAMLMetadata, &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -5258,9 +5344,9 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { }, args: args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, }, }, res: res{ @@ -5277,7 +5363,7 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { instance.NewSAMLIDPAddedEvent(context.Background(), &instance.NewAggregate("instance1").Aggregate, "id1", "name", - []byte("metadata"), + validSAMLMetadata, &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -5304,9 +5390,9 @@ func TestCommandSide_AddInstanceSAMLIDP(t *testing.T) { }, args: args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, Binding: "binding", WithSignedRequest: true, NameIDFormat: gu.Ptr(domain.SAMLNameIDFormatTransient), @@ -5356,7 +5442,7 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { type args struct { ctx context.Context id string - provider SAMLProvider + provider *SAMLProvider } type res struct { want *domain.ObjectDetails @@ -5375,7 +5461,7 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { }, args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), - provider: SAMLProvider{}, + provider: &SAMLProvider{}, }, res{ err: func(err error) bool { @@ -5391,7 +5477,7 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), id: "id1", - provider: SAMLProvider{}, + provider: &SAMLProvider{}, }, res{ err: func(err error) bool { @@ -5400,14 +5486,14 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { }, }, { - "invalid metadata", + "no metadata", fields{ eventstore: expectEventstore(), }, args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", }, }, @@ -5417,6 +5503,25 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { }, }, }, + { + "invalid metadata, error", + fields{ + eventstore: expectEventstore(), + }, + args{ + ctx: authz.WithInstanceID(context.Background(), "instance1"), + id: "id1", + provider: &SAMLProvider{ + Name: "name", + Metadata: []byte("metadata"), + }, + }, + res{ + err: func(err error) bool { + return errors.Is(err, zerrors.ThrowInvalidArgument(nil, "INST-dsfj3kl2", "Errors.Project.App.SAMLMetadataFormat")) + }, + }, + }, { name: "not found", fields: fields{ @@ -5427,9 +5532,9 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { args: args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, }, }, res: res{ @@ -5445,7 +5550,7 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { instance.NewSAMLIDPAddedEvent(context.Background(), &instance.NewAggregate("instance1").Aggregate, "id1", "name", - []byte("metadata"), + validSAMLMetadata, &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -5465,9 +5570,9 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { args: args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, }, }, res: res{ @@ -5505,7 +5610,7 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { "id1", []idp.SAMLIDPChanges{ idp.ChangeSAMLName("new name"), - idp.ChangeSAMLMetadata([]byte("new metadata")), + idp.ChangeSAMLMetadata(validSAMLMetadata), idp.ChangeSAMLBinding("new binding"), idp.ChangeSAMLWithSignedRequest(true), idp.ChangeSAMLNameIDFormat(gu.Ptr(domain.SAMLNameIDFormatTransient)), @@ -5527,9 +5632,9 @@ func TestCommandSide_UpdateInstanceGenericSAMLIDP(t *testing.T) { args: args{ ctx: authz.WithInstanceID(context.Background(), "instance1"), id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "new name", - Metadata: []byte("new metadata"), + Metadata: validSAMLMetadata, Binding: "new binding", WithSignedRequest: true, NameIDFormat: gu.Ptr(domain.SAMLNameIDFormatTransient), diff --git a/internal/command/org_idp.go b/internal/command/org_idp.go index 597783c78f..26690b3a66 100644 --- a/internal/command/org_idp.go +++ b/internal/command/org_idp.go @@ -10,6 +10,7 @@ import ( "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/eventstore" + "github.com/zitadel/zitadel/internal/idp/providers/saml" "github.com/zitadel/zitadel/internal/repository/org" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -446,9 +447,9 @@ func (c *Commands) UpdateOrgLDAPProvider(ctx context.Context, resourceOwner, id return pushedEventsToObjectDetails(pushedEvents), nil } -func (c *Commands) AddOrgSAMLProvider(ctx context.Context, resourceOwner string, provider SAMLProvider) (string, *domain.ObjectDetails, error) { +func (c *Commands) AddOrgSAMLProvider(ctx context.Context, resourceOwner string, provider *SAMLProvider) (id string, details *domain.ObjectDetails, err error) { orgAgg := org.NewAggregate(resourceOwner) - id, err := c.idGenerator.Next() + id, err = c.idGenerator.Next() if err != nil { return "", nil, err } @@ -464,7 +465,7 @@ func (c *Commands) AddOrgSAMLProvider(ctx context.Context, resourceOwner string, return id, pushedEventsToObjectDetails(pushedEvents), nil } -func (c *Commands) UpdateOrgSAMLProvider(ctx context.Context, resourceOwner, id string, provider SAMLProvider) (*domain.ObjectDetails, error) { +func (c *Commands) UpdateOrgSAMLProvider(ctx context.Context, resourceOwner, id string, provider *SAMLProvider) (details *domain.ObjectDetails, err error) { orgAgg := org.NewAggregate(resourceOwner) writeModel := NewSAMLOrgIDPWriteModel(resourceOwner, id) cmds, err := preparation.PrepareCommands(ctx, c.eventstore.Filter, c.prepareUpdateOrgSAMLProvider(orgAgg, writeModel, provider)) @@ -1703,7 +1704,7 @@ func (c *Commands) prepareUpdateOrgAppleProvider(a *org.Aggregate, writeModel *O } } -func (c *Commands) prepareAddOrgSAMLProvider(a *org.Aggregate, writeModel *OrgSAMLIDPWriteModel, provider SAMLProvider) preparation.Validation { +func (c *Commands) prepareAddOrgSAMLProvider(a *org.Aggregate, writeModel *OrgSAMLIDPWriteModel, provider *SAMLProvider) preparation.Validation { return func() (preparation.CreateCommands, error) { if provider.Name = strings.TrimSpace(provider.Name); provider.Name == "" { return nil, zerrors.ThrowInvalidArgument(nil, "ORG-957lr0f8u3", "Errors.Invalid.Argument") @@ -1718,6 +1719,9 @@ func (c *Commands) prepareAddOrgSAMLProvider(a *org.Aggregate, writeModel *OrgSA } provider.Metadata = data } + if _, err := saml.ParseMetadata(provider.Metadata); err != nil { + return nil, zerrors.ThrowInvalidArgument(err, "ORG-SF3rwhgh", "Errors.Project.App.SAMLMetadataFormat") + } return func(ctx context.Context, filter preparation.FilterToQueryReducer) ([]eventstore.Command, error) { events, err := filter(ctx, writeModel.Query()) if err != nil { @@ -1755,7 +1759,7 @@ func (c *Commands) prepareAddOrgSAMLProvider(a *org.Aggregate, writeModel *OrgSA } } -func (c *Commands) prepareUpdateOrgSAMLProvider(a *org.Aggregate, writeModel *OrgSAMLIDPWriteModel, provider SAMLProvider) preparation.Validation { +func (c *Commands) prepareUpdateOrgSAMLProvider(a *org.Aggregate, writeModel *OrgSAMLIDPWriteModel, provider *SAMLProvider) preparation.Validation { return func() (preparation.CreateCommands, error) { if writeModel.ID = strings.TrimSpace(writeModel.ID); writeModel.ID == "" { return nil, zerrors.ThrowInvalidArgument(nil, "ORG-wwdwdlaya0", "Errors.Invalid.Argument") @@ -1773,6 +1777,9 @@ func (c *Commands) prepareUpdateOrgSAMLProvider(a *org.Aggregate, writeModel *Or if provider.Metadata == nil { return nil, zerrors.ThrowInvalidArgument(nil, "ORG-j6spncd74m", "Errors.Invalid.Argument") } + if _, err := saml.ParseMetadata(provider.Metadata); err != nil { + return nil, zerrors.ThrowInvalidArgument(err, "ORG-SFqqh42", "Errors.Project.App.SAMLMetadataFormat") + } return func(ctx context.Context, filter preparation.FilterToQueryReducer) ([]eventstore.Command, error) { events, err := filter(ctx, writeModel.Query()) if err != nil { diff --git a/internal/command/org_idp_test.go b/internal/command/org_idp_test.go index b2b4632cf6..75321bb603 100644 --- a/internal/command/org_idp_test.go +++ b/internal/command/org_idp_test.go @@ -5348,7 +5348,7 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { type args struct { ctx context.Context resourceOwner string - provider SAMLProvider + provider *SAMLProvider } type res struct { id string @@ -5370,7 +5370,7 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { args{ ctx: context.Background(), resourceOwner: "org1", - provider: SAMLProvider{}, + provider: &SAMLProvider{}, }, res{ err: func(err error) bool { @@ -5379,7 +5379,7 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { }, }, { - "invalid metadata", + "no metadata", fields{ eventstore: expectEventstore(), idGenerator: id_mock.NewIDGeneratorExpectIDs(t, "id1"), @@ -5387,7 +5387,7 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { args{ ctx: context.Background(), resourceOwner: "org1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", }, }, @@ -5397,6 +5397,26 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { }, }, }, + { + "invalid metadata, fail on error", + fields{ + eventstore: expectEventstore(), + idGenerator: id_mock.NewIDGeneratorExpectIDs(t, "id1"), + }, + args{ + ctx: context.Background(), + resourceOwner: "org1", + provider: &SAMLProvider{ + Name: "name", + Metadata: []byte("metadata"), + }, + }, + res{ + err: func(err error) bool { + return errors.Is(err, zerrors.ThrowInvalidArgument(nil, "ORG-SF3rwhgh", "Errors.Project.App.SAMLMetadataFormat")) + }, + }, + }, { name: "ok", fields: fields{ @@ -5406,7 +5426,7 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { org.NewSAMLIDPAddedEvent(context.Background(), &org.NewAggregate("org1").Aggregate, "id1", "name", - []byte("metadata"), + validSAMLMetadata, &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -5428,9 +5448,9 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { args: args{ ctx: context.Background(), resourceOwner: "org1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, }, }, res: res{ @@ -5447,7 +5467,7 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { org.NewSAMLIDPAddedEvent(context.Background(), &org.NewAggregate("org1").Aggregate, "id1", "name", - []byte("metadata"), + validSAMLMetadata, &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -5475,9 +5495,9 @@ func TestCommandSide_AddOrgSAMLIDP(t *testing.T) { args: args{ ctx: context.Background(), resourceOwner: "org1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, Binding: "binding", WithSignedRequest: true, NameIDFormat: gu.Ptr(domain.SAMLNameIDFormatTransient), @@ -5528,7 +5548,7 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { ctx context.Context resourceOwner string id string - provider SAMLProvider + provider *SAMLProvider } type res struct { want *domain.ObjectDetails @@ -5548,7 +5568,7 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { args{ ctx: context.Background(), resourceOwner: "org1", - provider: SAMLProvider{}, + provider: &SAMLProvider{}, }, res{ err: func(err error) bool { @@ -5565,7 +5585,7 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { ctx: context.Background(), resourceOwner: "org1", id: "id1", - provider: SAMLProvider{}, + provider: &SAMLProvider{}, }, res{ err: func(err error) bool { @@ -5574,7 +5594,7 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { }, }, { - "invalid metadata", + "no metadata", fields{ eventstore: expectEventstore(), }, @@ -5582,7 +5602,7 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { ctx: context.Background(), resourceOwner: "org1", id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", }, }, @@ -5592,6 +5612,26 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { }, }, }, + { + "invalid metadata, error", + fields{ + eventstore: expectEventstore(), + }, + args{ + ctx: context.Background(), + resourceOwner: "org1", + id: "id1", + provider: &SAMLProvider{ + Name: "name", + Metadata: []byte("metadata"), + }, + }, + res{ + err: func(err error) bool { + return errors.Is(err, zerrors.ThrowInvalidArgument(nil, "ORG-SFqqh42", "Errors.Project.App.SAMLMetadataFormat")) + }, + }, + }, { name: "not found", fields: fields{ @@ -5603,9 +5643,9 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { ctx: context.Background(), resourceOwner: "org1", id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, }, }, res: res{ @@ -5623,7 +5663,7 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { org.NewSAMLIDPAddedEvent(context.Background(), &org.NewAggregate("org1").Aggregate, "id1", "name", - []byte("metadata"), + validSAMLMetadata, &crypto.CryptoValue{ CryptoType: crypto.TypeEncryption, Algorithm: "enc", @@ -5644,9 +5684,9 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { ctx: context.Background(), resourceOwner: "org1", id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "name", - Metadata: []byte("metadata"), + Metadata: validSAMLMetadata, }, }, res: res{ @@ -5684,7 +5724,7 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { "id1", []idp.SAMLIDPChanges{ idp.ChangeSAMLName("new name"), - idp.ChangeSAMLMetadata([]byte("new metadata")), + idp.ChangeSAMLMetadata(validSAMLMetadata), idp.ChangeSAMLBinding("new binding"), idp.ChangeSAMLWithSignedRequest(true), idp.ChangeSAMLNameIDFormat(gu.Ptr(domain.SAMLNameIDFormatTransient)), @@ -5707,9 +5747,9 @@ func TestCommandSide_UpdateOrgSAMLIDP(t *testing.T) { ctx: context.Background(), resourceOwner: "org1", id: "id1", - provider: SAMLProvider{ + provider: &SAMLProvider{ Name: "new name", - Metadata: []byte("new metadata"), + Metadata: validSAMLMetadata, Binding: "new binding", WithSignedRequest: true, NameIDFormat: gu.Ptr(domain.SAMLNameIDFormatTransient), diff --git a/internal/idp/providers/saml/saml.go b/internal/idp/providers/saml/saml.go index 702e1481e3..e0391bc099 100644 --- a/internal/idp/providers/saml/saml.go +++ b/internal/idp/providers/saml/saml.go @@ -1,15 +1,19 @@ package saml import ( + "bytes" "context" "crypto/rsa" "crypto/tls" "crypto/x509" "encoding/xml" + "io" "net/url" + "time" "github.com/crewjam/saml" "github.com/crewjam/saml/samlsp" + "golang.org/x/text/encoding/ianaindex" "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/idp" @@ -104,6 +108,41 @@ func WithEntityID(entityID string) ProviderOpts { } } +// ParseMetadata parses the metadata with the provided XML encoding and returns the EntityDescriptor +func ParseMetadata(metadata []byte) (*saml.EntityDescriptor, error) { + entityDescriptor := new(saml.EntityDescriptor) + reader := bytes.NewReader(metadata) + decoder := xml.NewDecoder(reader) + decoder.CharsetReader = func(charset string, reader io.Reader) (io.Reader, error) { + enc, err := ianaindex.IANA.Encoding(charset) + if err != nil { + return nil, err + } + return enc.NewDecoder().Reader(reader), nil + } + if err := decoder.Decode(entityDescriptor); err != nil { + if err.Error() == "expected element type but have " { + // reset reader to start of metadata so we can try to parse it as an EntitiesDescriptor + if _, err := reader.Seek(0, io.SeekStart); err != nil { + return nil, err + } + entities := &saml.EntitiesDescriptor{} + if err := decoder.Decode(entities); err != nil { + return nil, err + } + + for i, e := range entities.EntityDescriptors { + if len(e.IDPSSODescriptors) > 0 { + return &entities.EntityDescriptors[i], nil + } + } + return nil, zerrors.ThrowInternal(nil, "SAML-Ejoi3r2", "no entity found with IDPSSODescriptor") + } + return nil, err + } + return entityDescriptor, nil +} + func New( name string, rootURLStr string, @@ -112,8 +151,8 @@ func New( key []byte, options ...ProviderOpts, ) (*Provider, error) { - entityDescriptor := new(saml.EntityDescriptor) - if err := xml.Unmarshal(metadata, entityDescriptor); err != nil { + entityDescriptor, err := ParseMetadata(metadata) + if err != nil { return nil, err } keyPair, err := tls.X509KeyPair(certificate, key) @@ -180,6 +219,7 @@ func (p *Provider) GetSP() (*samlsp.Middleware, error) { if p.binding != "" { sp.Binding = p.binding } + sp.ServiceProvider.MetadataValidDuration = time.Until(sp.ServiceProvider.Certificate.NotAfter) return sp, nil } diff --git a/internal/idp/providers/saml/saml_test.go b/internal/idp/providers/saml/saml_test.go index dc8e8e2115..801ddd36fc 100644 --- a/internal/idp/providers/saml/saml_test.go +++ b/internal/idp/providers/saml/saml_test.go @@ -1,6 +1,7 @@ package saml import ( + "encoding/xml" "testing" "github.com/crewjam/saml" @@ -10,6 +11,7 @@ import ( "github.com/zitadel/zitadel/internal/domain" "github.com/zitadel/zitadel/internal/idp/providers/saml/requesttracker" + "github.com/zitadel/zitadel/internal/zerrors" ) func TestProvider_Options(t *testing.T) { @@ -170,3 +172,111 @@ func TestProvider_Options(t *testing.T) { }) } } + +func TestParseMetadata(t *testing.T) { + type args struct { + metadata []byte + } + tests := []struct { + name string + args args + want *saml.EntityDescriptor + wantErr error + }{ + { + "invalid", + args{ + metadata: []byte(``), + }, + nil, + xml.UnmarshalError("expected element type but have "), + }, + { + "valid entity descriptor", + args{ + metadata: []byte(``), + }, + &saml.EntityDescriptor{ + EntityID: "http://localhost:8000/metadata", + IDPSSODescriptors: []saml.IDPSSODescriptor{ + { + XMLName: xml.Name{ + Space: "urn:oasis:names:tc:SAML:2.0:metadata", + Local: "IDPSSODescriptor", + }, + SingleSignOnServices: []saml.Endpoint{ + { + Binding: saml.HTTPRedirectBinding, + Location: "http://localhost:8000/sso", + }, + }, + }, + }, + }, + nil, + }, + { + "valid entity descriptor, non utf-8", + args{ + metadata: []byte(``), + }, + &saml.EntityDescriptor{ + EntityID: "http://localhost:8000/metadata", + IDPSSODescriptors: []saml.IDPSSODescriptor{ + { + XMLName: xml.Name{ + Space: "urn:oasis:names:tc:SAML:2.0:metadata", + Local: "IDPSSODescriptor", + }, + SingleSignOnServices: []saml.Endpoint{ + { + Binding: saml.HTTPRedirectBinding, + Location: "http://localhost:8000/sso", + }, + }, + }, + }, + }, + nil, + }, + { + "entities descriptor without IDPSSODescriptor", + args{ + metadata: []byte(``), + }, + nil, + zerrors.ThrowInternal(nil, "SAML-Ejoi3r2", "no entity found with IDPSSODescriptor"), + }, + { + "valid entities descriptor", + args{ + metadata: []byte(``), + }, + &saml.EntityDescriptor{ + EntityID: "http://localhost:8000/metadata", + IDPSSODescriptors: []saml.IDPSSODescriptor{ + { + XMLName: xml.Name{ + Space: "urn:oasis:names:tc:SAML:2.0:metadata", + Local: "IDPSSODescriptor", + }, + SingleSignOnServices: []saml.Endpoint{ + { + Binding: saml.HTTPRedirectBinding, + Location: "http://localhost:8000/sso", + }, + }, + }, + }, + }, + nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ParseMetadata(tt.args.metadata) + assert.ErrorIs(t, err, tt.wantErr) + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/internal/idp/providers/saml/session.go b/internal/idp/providers/saml/session.go index 09de7163bb..49a04e49cb 100644 --- a/internal/idp/providers/saml/session.go +++ b/internal/idp/providers/saml/session.go @@ -9,7 +9,6 @@ import ( "github.com/crewjam/saml" "github.com/crewjam/saml/samlsp" - "github.com/zitadel/logging" "github.com/zitadel/zitadel/internal/idp" "github.com/zitadel/zitadel/internal/zerrors" @@ -71,7 +70,7 @@ func (s *Session) FetchUser(ctx context.Context) (user idp.User, err error) { if err != nil { invalidRespErr := new(saml.InvalidResponseError) if errors.As(err, &invalidRespErr) { - logging.WithError(invalidRespErr.PrivateErr).Info("invalid SAML response details") + return nil, zerrors.ThrowInvalidArgument(invalidRespErr.PrivateErr, "SAML-ajl3irfs", "Errors.Intent.ResponseInvalid") } return nil, zerrors.ThrowInvalidArgument(err, "SAML-nuo0vphhh9", "Errors.Intent.ResponseInvalid") } diff --git a/internal/idp/providers/saml/session_test.go b/internal/idp/providers/saml/session_test.go index 5ab8c7eaec..ea3e510d60 100644 --- a/internal/idp/providers/saml/session_test.go +++ b/internal/idp/providers/saml/session_test.go @@ -134,7 +134,7 @@ func TestSession_FetchUser(t *testing.T) { requestID: "id-b22c90db88bf01d82ffb0a7b6fe25ac9fcb2c679", }, want: want{ - err: zerrors.ThrowInvalidArgument(nil, "SAML-nuo0vphhh9", "Errors.Intent.ResponseInvalid"), + err: zerrors.ThrowInvalidArgument(nil, "SAML-ajl3irfs", "Errors.Intent.ResponseInvalid"), }, }, {