From bcd05a2e55e538827700dc5a6ee1ab55912b7283 Mon Sep 17 00:00:00 2001 From: Sacha Al Himdani Date: Fri, 24 Jul 2026 09:47:09 +0200 Subject: [PATCH] Reject empty SAML NameIDs on login Empty NameID values were stored as '' and occupied the unique saml_subject index, causing duplicate-key failures on later logins. Reject blank NameIDs during assertion validation, return a clear error when a NameID is already linked to another account, and stop returning internal errors from the SAML consume endpoint. Signed-off-by: Sacha Al Himdani --- pkg/coredata/identity.go | 24 ++++- pkg/iam/saml/errors.go | 22 ++++ pkg/iam/saml/service.go | 35 +++++-- pkg/iam/saml/service_test.go | 117 ++++++++++++++++++++++ pkg/server/api/connect/v1/saml_handler.go | 49 ++++++++- 5 files changed, 234 insertions(+), 13 deletions(-) create mode 100644 pkg/iam/saml/service_test.go diff --git a/pkg/coredata/identity.go b/pkg/coredata/identity.go index 27dd797f7..60ce0e648 100644 --- a/pkg/coredata/identity.go +++ b/pkg/coredata/identity.go @@ -52,6 +52,10 @@ type ( Identities []*Identity ) +var ( + ErrSAMLSubjectAlreadyExists = errors.New("saml subject already exists") +) + func (i Identity) CursorKey(orderBy IdentityOrderField) page.CursorKey { switch orderBy { case IdentityOrderFieldCreatedAt: @@ -247,8 +251,11 @@ VALUES ( _, err := conn.Exec(ctx, q, args) if err != nil { - if pgErr, ok := errors.AsType[*pgconn.PgError](err); ok { - if pgErr.Code == "23505" && strings.Contains(pgErr.ConstraintName, "email_address") { + if pgErr, ok := errors.AsType[*pgconn.PgError](err); ok && pgErr.Code == "23505" { + switch pgErr.ConstraintName { + case "idx_users_saml_subject": + return ErrSAMLSubjectAlreadyExists + case "usrmgr_users_email_address_key": return ErrResourceAlreadyExists } } @@ -288,6 +295,15 @@ WHERE result, err := conn.Exec(ctx, q, args) if err != nil { + if pgErr, ok := errors.AsType[*pgconn.PgError](err); ok && pgErr.Code == "23505" { + switch pgErr.ConstraintName { + case "idx_users_saml_subject": + return ErrSAMLSubjectAlreadyExists + case "usrmgr_users_email_address_key": + return ErrResourceAlreadyExists + } + } + return fmt.Errorf("cannot update identity: %w", err) } @@ -304,6 +320,10 @@ func (i *Identity) LoadBySAMLSubject( conn pg.Querier, samlSubject string, ) error { + if strings.TrimSpace(samlSubject) == "" { + return ErrResourceNotFound + } + q := ` SELECT id, diff --git a/pkg/iam/saml/errors.go b/pkg/iam/saml/errors.go index 99a9fc064..9d2d8a4bd 100644 --- a/pkg/iam/saml/errors.go +++ b/pkg/iam/saml/errors.go @@ -104,3 +104,25 @@ func NewUserInactiveError(profileID gid.GID) error { func (e ErrUserInactive) Error() string { return fmt.Sprintf("user %q is inactive", e.ProfileID) } + +type ErrSAMLSubjectAlreadyInUse struct { + AssertionID string +} + +func NewSAMLSubjectAlreadyInUseError(assertionID string) error { + return &ErrSAMLSubjectAlreadyInUse{AssertionID: assertionID} +} + +func (e ErrSAMLSubjectAlreadyInUse) Error() string { + return fmt.Sprintf("SAML NameID is already linked to another account (assertion %q)", e.AssertionID) +} + +type ErrSAMLSubjectRequired struct{} + +func NewSAMLSubjectRequiredError() error { + return &ErrSAMLSubjectRequired{} +} + +func (e ErrSAMLSubjectRequired) Error() string { + return "NameID value is required" +} diff --git a/pkg/iam/saml/service.go b/pkg/iam/saml/service.go index 66fd9fca8..7edac5323 100644 --- a/pkg/iam/saml/service.go +++ b/pkg/iam/saml/service.go @@ -257,14 +257,16 @@ func (s *Service) HandleAssertion( return NewEmailDomainMismatchError(email, config.EmailDomain) } + samlSubject := strings.TrimSpace(assertion.Subject.NameID.Value) + err = identity.LoadByEmail(ctx, tx, email) - if err == coredata.ErrResourceNotFound && !config.AutoSignupEnabled { + if errors.Is(err, coredata.ErrResourceNotFound) && !config.AutoSignupEnabled { return NewSAMLAutoSignupDisabledError(config.ID) - } else if err == coredata.ErrResourceNotFound && config.AutoSignupEnabled { + } else if errors.Is(err, coredata.ErrResourceNotFound) && config.AutoSignupEnabled { *identity = coredata.Identity{ ID: gid.New(gid.NilTenant, coredata.IdentityEntityType), EmailAddress: email, - SAMLSubject: &assertion.Subject.NameID.Value, + SAMLSubject: &samlSubject, FullName: fullname, HashedPassword: nil, EmailAddressVerified: true, @@ -272,8 +274,11 @@ func (s *Service) HandleAssertion( UpdatedAt: now, } - err := identity.Insert(ctx, tx) - if err != nil { + if err := identity.Insert(ctx, tx); err != nil { + if errors.Is(err, coredata.ErrSAMLSubjectAlreadyExists) { + return NewSAMLSubjectAlreadyInUseError(assertion.ID) + } + return fmt.Errorf("cannot insert identity: %w", err) } } else if err != nil { @@ -282,16 +287,18 @@ func (s *Service) HandleAssertion( identity.EmailAddress = email identity.FullName = fullname - // Identity can exist (e.g. provisioned via SCIM) but not have a SAML subject - if identity.SAMLSubject == nil { - identity.SAMLSubject = &assertion.Subject.NameID.Value + if !hasSAMLSubject(identity) { + identity.SAMLSubject = &samlSubject } identity.EmailAddressVerified = true identity.UpdatedAt = now - err = identity.Update(ctx, tx) - if err != nil { + if err = identity.Update(ctx, tx); err != nil { + if errors.Is(err, coredata.ErrSAMLSubjectAlreadyExists) { + return NewSAMLSubjectAlreadyInUseError(assertion.ID) + } + return fmt.Errorf("cannot update identity: %w", err) } } @@ -415,6 +422,10 @@ func (s *Service) validateAssertion(assertion *saml.Assertion, config *coredata. return fmt.Errorf("subject or NameID missing") } + if strings.TrimSpace(assertion.Subject.NameID.Value) == "" { + return NewSAMLSubjectRequiredError() + } + if assertion.Issuer.Value != config.IdPEntityID { return fmt.Errorf("assertion issuer %q does not match expected issuer %q", assertion.Issuer.Value, config.IdPEntityID) @@ -479,3 +490,7 @@ func (s *Service) baseServiceProvider() *saml.ServiceProvider { AllowIDPInitiated: true, } } + +func hasSAMLSubject(identity *coredata.Identity) bool { + return identity.SAMLSubject != nil && strings.TrimSpace(*identity.SAMLSubject) != "" +} diff --git a/pkg/iam/saml/service_test.go b/pkg/iam/saml/service_test.go new file mode 100644 index 000000000..f8f645a09 --- /dev/null +++ b/pkg/iam/saml/service_test.go @@ -0,0 +1,117 @@ +// Copyright (c) 2025-2026 Probo Inc . +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package saml + +import ( + "errors" + "testing" + "time" + + "github.com/crewjam/saml" + "go.probo.inc/probo/pkg/coredata" +) + +func TestValidateAssertionRejectsEmptySAMLSubject(t *testing.T) { + t.Parallel() + + s := &Service{} + config := &coredata.SAMLConfiguration{IdPEntityID: "https://idp.example"} + now := time.Now() + + tests := []struct { + name string + value string + }{ + {name: "empty", value: ""}, + {name: "whitespace", value: " "}, + } + + for _, tt := range tests { + t.Run( + tt.name, + func(t *testing.T) { + t.Parallel() + + err := s.validateAssertion( + &saml.Assertion{ + ID: "assertion-1", + Issuer: saml.Issuer{ + Value: config.IdPEntityID, + }, + Subject: &saml.Subject{ + NameID: &saml.NameID{Value: tt.value}, + }, + Conditions: &saml.Conditions{ + NotOnOrAfter: now.Add(time.Hour), + }, + }, + config, + now, + ) + if err == nil { + t.Fatal("expected error for empty SAML subject") + } + + if _, ok := errors.AsType[*ErrSAMLSubjectRequired](err); !ok { + t.Fatalf("expected *ErrSAMLSubjectRequired, got %T: %v", err, err) + } + + if got := err.Error(); got != "NameID value is required" { + t.Fatalf("unexpected error: %v", err) + } + }, + ) + } +} + +func TestHasSAMLSubject(t *testing.T) { + t.Parallel() + + empty := "" + whitespace := " " + value := "user@example.com" + + tests := []struct { + name string + subject *string + expected bool + }{ + {name: "nil", subject: nil, expected: false}, + {name: "empty", subject: &empty, expected: false}, + {name: "whitespace", subject: &whitespace, expected: false}, + {name: "present", subject: &value, expected: true}, + } + + for _, tt := range tests { + t.Run( + tt.name, + func(t *testing.T) { + t.Parallel() + + identity := &coredata.Identity{SAMLSubject: tt.subject} + + if got := hasSAMLSubject(identity); got != tt.expected { + t.Fatalf("hasSAMLSubject() = %v, want %v", got, tt.expected) + } + }, + ) + } +} diff --git a/pkg/server/api/connect/v1/saml_handler.go b/pkg/server/api/connect/v1/saml_handler.go index de4102327..05a06d91c 100644 --- a/pkg/server/api/connect/v1/saml_handler.go +++ b/pkg/server/api/connect/v1/saml_handler.go @@ -32,6 +32,7 @@ import ( "go.probo.inc/probo/pkg/baseurl" "go.probo.inc/probo/pkg/gid" "go.probo.inc/probo/pkg/iam" + "go.probo.inc/probo/pkg/iam/saml" "go.probo.inc/probo/pkg/saferedirect" "go.probo.inc/probo/pkg/securecookie" "go.probo.inc/probo/pkg/server/api/authn" @@ -59,6 +60,52 @@ func (h *SAMLHandler) renderInternalServerError(w http.ResponseWriter) { httpserver.RenderError(w, http.StatusInternalServerError, errors.New("internal server error")) } +func (h *SAMLHandler) renderAssertionError(w http.ResponseWriter, r *http.Request, err error) { + if isClientSAMLError(err) { + httpserver.RenderError(w, http.StatusUnauthorized, err) + return + } + + h.logger.ErrorCtx(r.Context(), "cannot handle SAML assertion", log.Error(err)) + httpserver.RenderError(w, http.StatusUnauthorized, errors.New("authentication failed")) +} + +func isClientSAMLError(err error) bool { + if _, ok := errors.AsType[*saml.ErrSAMLConfigurationNotFound](err); ok { + return true + } + + if _, ok := errors.AsType[*saml.ErrSAMLDisabled](err); ok { + return true + } + + if _, ok := errors.AsType[*saml.ErrInvalidAssertion](err); ok { + return true + } + + if _, ok := errors.AsType[*saml.ErrReplayAttackDetected](err); ok { + return true + } + + if _, ok := errors.AsType[*saml.ErrEmailDomainMismatch](err); ok { + return true + } + + if _, ok := errors.AsType[*saml.ErrSAMLAutoSignupDisabled](err); ok { + return true + } + + if _, ok := errors.AsType[*saml.ErrUserInactive](err); ok { + return true + } + + if _, ok := errors.AsType[*saml.ErrSAMLSubjectAlreadyInUse](err); ok { + return true + } + + return false +} + func (h *SAMLHandler) MetadataHandler(w http.ResponseWriter, r *http.Request) { metadataXML, err := h.iam.SAMLService.GenerateSpMetadata() if err != nil { @@ -101,7 +148,7 @@ func (h *SAMLHandler) ConsumeHandler(w http.ResponseWriter, r *http.Request) { user, membership, err := h.iam.SAMLService.HandleAssertion(ctx, samlResponse, configID) if err != nil { - httpserver.RenderError(w, http.StatusUnauthorized, err) + h.renderAssertionError(w, r, err) return }