mirror of
https://github.com/zitadel/zitadel.git
synced 2025-08-12 19:47:34 +00:00
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](efd25daf28/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 commitffe9570776
)
This commit is contained in:
@@ -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 <EntityDescriptor> but have <EntitiesDescriptor>" {
|
||||
// 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
|
||||
}
|
||||
|
||||
|
Reference in New Issue
Block a user