mirror of
https://github.com/zitadel/zitadel.git
synced 2025-08-12 03:57:32 +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
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