From b3060291dd9197f7f2678d91392456275737bc0e Mon Sep 17 00:00:00 2001 From: Phan Trung Thanh Date: Thu, 5 Dec 2024 17:47:08 +0100 Subject: [PATCH 1/5] send redirect request instead of POST binding Signed-off-by: Phan Trung Thanh --- server/handlers.go | 20 ++------------------ 1 file changed, 2 insertions(+), 18 deletions(-) diff --git a/server/handlers.go b/server/handlers.go index 5954820caa..0560f2309c 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -287,24 +287,8 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { s.renderError(r, w, http.StatusInternalServerError, "Connector Login Error") return } - - // TODO(ericchiang): Don't inline this. - fmt.Fprintf(w, ` - - - - SAML login - - -
- - -
- - - `, action, value, authReq.ID) + escaped_saml_request := url.QueryEscape(value) + http.Redirect(w, r, action+"/saml?SAMLRequest="+escaped_saml_request+"&RelayState="+authReq.ID, http.StatusFound) default: s.renderError(r, w, http.StatusBadRequest, "Requested resource does not exist.") } From ce980cd8ad62023153b039879621090506321132 Mon Sep 17 00:00:00 2001 From: Phan Trung Thanh Date: Wed, 19 Mar 2025 18:19:41 +0100 Subject: [PATCH 2/5] temporary disable parsing attributeStatements Signed-off-by: Phan Trung Thanh --- connector/saml/saml.go | 137 +++++++++++++++++++++-------------------- 1 file changed, 71 insertions(+), 66 deletions(-) diff --git a/connector/saml/saml.go b/connector/saml/saml.go index bc8ef726ce..5ac66a01c2 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -21,7 +21,6 @@ import ( "github.com/russellhaering/goxmldsig/etreeutils" "github.com/dexidp/dex/connector" - "github.com/dexidp/dex/pkg/groups" ) const ( @@ -379,72 +378,78 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str return ident, fmt.Errorf("subject does not contain an NameID element") } - // After verifying the assertion, map data in the attribute statements to - // various user info. - attributes := assertion.AttributeStatement - if attributes == nil { - return ident, fmt.Errorf("response did not contain a AttributeStatement") - } - - // Log the actual attributes we got back from the server. This helps debug - // configuration errors on the server side, where the SAML server doesn't - // send us the correct attributes. - p.logger.Info("parsed and verified saml response attributes", "attributes", attributes) - - // Grab the email. - if ident.Email, _ = attributes.get(p.emailAttr); ident.Email == "" { - return ident, fmt.Errorf("no attribute with name %q: %s", p.emailAttr, attributes.names()) - } - // TODO(ericchiang): Does SAML have an email_verified equivalent? + ident.Email = assertion.Subject.NameID.Value + ident.Username = assertion.Subject.NameID.Value ident.EmailVerified = true - - // Grab the username. - if ident.Username, _ = attributes.get(p.usernameAttr); ident.Username == "" { - return ident, fmt.Errorf("no attribute with name %q: %s", p.usernameAttr, attributes.names()) - } - - if len(p.allowedGroups) == 0 && (!s.Groups || p.groupsAttr == "") { - // Groups not requested or not configured. We're done. - return ident, nil - } - - if len(p.allowedGroups) > 0 && (!s.Groups || p.groupsAttr == "") { - // allowedGroups set but no groups or groupsAttr. Disallowing. - return ident, fmt.Errorf("user not a member of allowed groups") - } - - // Grab the groups. - if p.groupsDelim != "" { - groupsStr, ok := attributes.get(p.groupsAttr) - if !ok { - return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) - } - // TODO(ericchiang): Do we need to further trim whitespace? - ident.Groups = strings.Split(groupsStr, p.groupsDelim) - } else { - groups, ok := attributes.all(p.groupsAttr) - if !ok { - return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) - } - ident.Groups = groups - } - - if len(p.allowedGroups) == 0 { - // No allowed groups set, just return the ident - return ident, nil - } - - // Look for membership in one of the allowed groups - groupMatches := groups.Filter(ident.Groups, p.allowedGroups) - - if len(groupMatches) == 0 { - // No group membership matches found, disallowing - return ident, fmt.Errorf("user not a member of allowed groups") - } - - if p.filterGroups { - ident.Groups = groupMatches - } + // p.logger.Info("Assertion: %+x", assertion) + // p.logger.Info("AttributeStatement: %+x", assertion.AttributeStatement) + + // // After verifying the assertion, map data in the attribute statements to + // // various user info. + // attributes := assertion.AttributeStatement + // if attributes == nil { + // return ident, fmt.Errorf("response did not contain a AttributeStatement") + // } + + // // Log the actual attributes we got back from the server. This helps debug + // // configuration errors on the server side, where the SAML server doesn't + // // send us the correct attributes. + // p.logger.Info("parsed and verified saml response attributes", "attributes", attributes) + + // // Grab the email. + // if ident.Email, _ = attributes.get(p.emailAttr); ident.Email == "" { + // return ident, fmt.Errorf("no attribute with name %q: %s", p.emailAttr, attributes.names()) + // } + // // TODO(ericchiang): Does SAML have an email_verified equivalent? + // ident.EmailVerified = true + + // // Grab the username. + // if ident.Username, _ = attributes.get(p.usernameAttr); ident.Username == "" { + // return ident, fmt.Errorf("no attribute with name %q: %s", p.usernameAttr, attributes.names()) + // } + + // if len(p.allowedGroups) == 0 && (!s.Groups || p.groupsAttr == "") { + // // Groups not requested or not configured. We're done. + // return ident, nil + // } + + // if len(p.allowedGroups) > 0 && (!s.Groups || p.groupsAttr == "") { + // // allowedGroups set but no groups or groupsAttr. Disallowing. + // return ident, fmt.Errorf("user not a member of allowed groups") + // } + + // // Grab the groups. + // if p.groupsDelim != "" { + // groupsStr, ok := attributes.get(p.groupsAttr) + // if !ok { + // return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) + // } + // // TODO(ericchiang): Do we need to further trim whitespace? + // ident.Groups = strings.Split(groupsStr, p.groupsDelim) + // } else { + // groups, ok := attributes.all(p.groupsAttr) + // if !ok { + // return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) + // } + // ident.Groups = groups + // } + + // if len(p.allowedGroups) == 0 { + // // No allowed groups set, just return the ident + // return ident, nil + // } + + // // Look for membership in one of the allowed groups + // groupMatches := groups.Filter(ident.Groups, p.allowedGroups) + + // if len(groupMatches) == 0 { + // // No group membership matches found, disallowing + // return ident, fmt.Errorf("user not a member of allowed groups") + // } + + // if p.filterGroups { + // ident.Groups = groupMatches + // } // Otherwise, we're good return ident, nil From 4842bbba2179b983c07fb261179a7ad5cd6f8fea Mon Sep 17 00:00:00 2001 From: Phan Trung Thanh Date: Mon, 24 Mar 2025 14:55:40 +0100 Subject: [PATCH 3/5] add config to select connector Signed-off-by: Phan Trung Thanh --- connector/connector.go | 2 +- connector/saml/saml.go | 164 ++++++++++++++++++++++------------------- server/handlers.go | 31 +++++++- 3 files changed, 119 insertions(+), 78 deletions(-) diff --git a/connector/connector.go b/connector/connector.go index d812390f0c..38a8d7e173 100644 --- a/connector/connector.go +++ b/connector/connector.go @@ -81,7 +81,7 @@ type SAMLConnector interface { // // POSTData should encode the provided request ID in the returned serialized // SAML request. - POSTData(s Scopes, requestID string) (ssoURL, samlRequest string, err error) + POSTData(s Scopes, requestID string) (ssoURL, samlRequest string, bindingType string, err error) // HandlePOST decodes, verifies, and maps attributes from the SAML response. // It passes the expected value of the "InResponseTo" response field, which diff --git a/connector/saml/saml.go b/connector/saml/saml.go index 5ac66a01c2..a310365da5 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -21,6 +21,12 @@ import ( "github.com/russellhaering/goxmldsig/etreeutils" "github.com/dexidp/dex/connector" + "github.com/dexidp/dex/pkg/groups" +) + +const ( + RedirectBinding = "redirect" + PostBinding = "post" ) const ( @@ -106,6 +112,11 @@ type Config struct { // urn:oasis:names:tc:SAML:2.0:nameid-format:persistent // NameIDPolicyFormat string `json:"nameIDPolicyFormat"` + + // Specify the type of binding used to send SAML request, only supported values are + // "post" and "redirect" + // If no value is specified, this value defaults to: "post" + BindingType string `json:"bindingType"` } type certStore struct { @@ -158,6 +169,7 @@ func (c *Config) openConnector(logger *slog.Logger) (*provider, error) { allowedGroups: c.AllowedGroups, filterGroups: c.FilterGroups, redirectURI: c.RedirectURI, + bindingType: c.BindingType, logger: logger, nameIDPolicyFormat: c.NameIDPolicyFormat, @@ -226,6 +238,13 @@ func (c *Config) openConnector(logger *slog.Logger) (*provider, error) { } p.validator = dsig.NewDefaultValidationContext(certStore{certs}) } + + if p.bindingType == "" { + p.bindingType = PostBinding + } + if p.bindingType != PostBinding && p.bindingType != RedirectBinding { + return nil, fmt.Errorf("bindingType must be either 'redirect' or 'post', current: %s", p.bindingType) + } return p, nil } @@ -251,10 +270,12 @@ type provider struct { nameIDPolicyFormat string + bindingType string + logger *slog.Logger } -func (p *provider) POSTData(s connector.Scopes, id string) (action, value string, err error) { +func (p *provider) POSTData(s connector.Scopes, id string) (action, value string, bindingType string, err error) { r := &authnRequest{ ProtocolBinding: bindingPOST, ID: id, @@ -274,12 +295,12 @@ func (p *provider) POSTData(s connector.Scopes, id string) (action, value string data, err := xml.MarshalIndent(r, "", " ") if err != nil { - return "", "", fmt.Errorf("marshal authn request: %v", err) + return "", "", "",fmt.Errorf("marshal authn request: %v", err) } // See: https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf // "3.5.4 Message Encoding" - return p.ssoURL, base64.StdEncoding.EncodeToString(data), nil + return p.ssoURL, base64.StdEncoding.EncodeToString(data), p.bindingType, nil } // HandlePOST interprets a request from a SAML provider attempting to verify a @@ -378,78 +399,73 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str return ident, fmt.Errorf("subject does not contain an NameID element") } - ident.Email = assertion.Subject.NameID.Value - ident.Username = assertion.Subject.NameID.Value + + // After verifying the assertion, map data in the attribute statements to + // various user info. + attributes := assertion.AttributeStatement + if attributes == nil { + return ident, fmt.Errorf("response did not contain a AttributeStatement") + } + + // Log the actual attributes we got back from the server. This helps debug + // configuration errors on the server side, where the SAML server doesn't + // send us the correct attributes. + p.logger.Info("parsed and verified saml response attributes", "attributes", attributes) + + // Grab the email. + if ident.Email, _ = attributes.get(p.emailAttr); ident.Email == "" { + return ident, fmt.Errorf("no attribute with name %q: %s", p.emailAttr, attributes.names()) + } + // TODO(ericchiang): Does SAML have an email_verified equivalent? ident.EmailVerified = true - // p.logger.Info("Assertion: %+x", assertion) - // p.logger.Info("AttributeStatement: %+x", assertion.AttributeStatement) - - // // After verifying the assertion, map data in the attribute statements to - // // various user info. - // attributes := assertion.AttributeStatement - // if attributes == nil { - // return ident, fmt.Errorf("response did not contain a AttributeStatement") - // } - - // // Log the actual attributes we got back from the server. This helps debug - // // configuration errors on the server side, where the SAML server doesn't - // // send us the correct attributes. - // p.logger.Info("parsed and verified saml response attributes", "attributes", attributes) - - // // Grab the email. - // if ident.Email, _ = attributes.get(p.emailAttr); ident.Email == "" { - // return ident, fmt.Errorf("no attribute with name %q: %s", p.emailAttr, attributes.names()) - // } - // // TODO(ericchiang): Does SAML have an email_verified equivalent? - // ident.EmailVerified = true - - // // Grab the username. - // if ident.Username, _ = attributes.get(p.usernameAttr); ident.Username == "" { - // return ident, fmt.Errorf("no attribute with name %q: %s", p.usernameAttr, attributes.names()) - // } - - // if len(p.allowedGroups) == 0 && (!s.Groups || p.groupsAttr == "") { - // // Groups not requested or not configured. We're done. - // return ident, nil - // } - - // if len(p.allowedGroups) > 0 && (!s.Groups || p.groupsAttr == "") { - // // allowedGroups set but no groups or groupsAttr. Disallowing. - // return ident, fmt.Errorf("user not a member of allowed groups") - // } - - // // Grab the groups. - // if p.groupsDelim != "" { - // groupsStr, ok := attributes.get(p.groupsAttr) - // if !ok { - // return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) - // } - // // TODO(ericchiang): Do we need to further trim whitespace? - // ident.Groups = strings.Split(groupsStr, p.groupsDelim) - // } else { - // groups, ok := attributes.all(p.groupsAttr) - // if !ok { - // return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) - // } - // ident.Groups = groups - // } - - // if len(p.allowedGroups) == 0 { - // // No allowed groups set, just return the ident - // return ident, nil - // } - - // // Look for membership in one of the allowed groups - // groupMatches := groups.Filter(ident.Groups, p.allowedGroups) - - // if len(groupMatches) == 0 { - // // No group membership matches found, disallowing - // return ident, fmt.Errorf("user not a member of allowed groups") - // } - - // if p.filterGroups { - // ident.Groups = groupMatches - // } + + // Grab the username. + if ident.Username, _ = attributes.get(p.usernameAttr); ident.Username == "" { + return ident, fmt.Errorf("no attribute with name %q: %s", p.usernameAttr, attributes.names()) + } + + if len(p.allowedGroups) == 0 && (!s.Groups || p.groupsAttr == "") { + // Groups not requested or not configured. We're done. + return ident, nil + } + + if len(p.allowedGroups) > 0 && (!s.Groups || p.groupsAttr == "") { + // allowedGroups set but no groups or groupsAttr. Disallowing. + return ident, fmt.Errorf("user not a member of allowed groups") + } + + // Grab the groups. + if p.groupsDelim != "" { + groupsStr, ok := attributes.get(p.groupsAttr) + if !ok { + return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) + } + // TODO(ericchiang): Do we need to further trim whitespace? + ident.Groups = strings.Split(groupsStr, p.groupsDelim) + } else { + groups, ok := attributes.all(p.groupsAttr) + if !ok { + return ident, fmt.Errorf("no attribute with name %q: %s", p.groupsAttr, attributes.names()) + } + ident.Groups = groups + } + + if len(p.allowedGroups) == 0 { + // No allowed groups set, just return the ident + return ident, nil + } + + // Look for membership in one of the allowed groups + groupMatches := groups.Filter(ident.Groups, p.allowedGroups) + + if len(groupMatches) == 0 { + // No group membership matches found, disallowing + return ident, fmt.Errorf("user not a member of allowed groups") + } + + if p.filterGroups { + ident.Groups = groupMatches + } // Otherwise, we're good return ident, nil diff --git a/server/handlers.go b/server/handlers.go index 0560f2309c..1f636fc10d 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -22,6 +22,7 @@ import ( "github.com/gorilla/mux" "github.com/dexidp/dex/connector" + "github.com/dexidp/dex/connector/saml" "github.com/dexidp/dex/server/internal" "github.com/dexidp/dex/storage" ) @@ -281,14 +282,38 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, loginURL.String(), http.StatusFound) case connector.SAMLConnector: - action, value, err := conn.POSTData(scopes, authReq.ID) + action, value, bindingType, err := conn.POSTData(scopes, authReq.ID) if err != nil { s.logger.ErrorContext(r.Context(), "creating SAML data", "err", err) s.renderError(r, w, http.StatusInternalServerError, "Connector Login Error") return } - escaped_saml_request := url.QueryEscape(value) - http.Redirect(w, r, action+"/saml?SAMLRequest="+escaped_saml_request+"&RelayState="+authReq.ID, http.StatusFound) + + if bindingType == saml.RedirectBinding { + escaped_saml_request := url.QueryEscape(value) + http.Redirect(w, r, action+"/saml?SAMLRequest="+escaped_saml_request+"&RelayState="+authReq.ID, http.StatusFound) + } else if bindingType == saml.PostBinding { + // TODO(ericchiang): Don't inline this. + fmt.Fprintf(w, ` + + + + SAML login + + +
+ + +
+ + + `, action, value, authReq.ID) + } else { + s.renderError(r, w, http.StatusInternalServerError, "Invalid binding type") + return + } default: s.renderError(r, w, http.StatusBadRequest, "Requested resource does not exist.") } From eade3aca923183179bdb1f4638a4c7abeb72cbc0 Mon Sep 17 00:00:00 2001 From: Phan Trung Thanh Date: Mon, 24 Mar 2025 15:40:17 +0100 Subject: [PATCH 4/5] remove extra line Signed-off-by: Phan Trung Thanh --- connector/saml/saml.go | 1 - 1 file changed, 1 deletion(-) diff --git a/connector/saml/saml.go b/connector/saml/saml.go index a310365da5..e866dab90f 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -399,7 +399,6 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str return ident, fmt.Errorf("subject does not contain an NameID element") } - // After verifying the assertion, map data in the attribute statements to // various user info. attributes := assertion.AttributeStatement From 1c2cdaceb872a73afd04d8b36676678d5a885adb Mon Sep 17 00:00:00 2001 From: Phan Trung Thanh Date: Mon, 24 Mar 2025 15:41:12 +0100 Subject: [PATCH 5/5] more verbose log Signed-off-by: Phan Trung Thanh --- server/handlers.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/handlers.go b/server/handlers.go index 1f636fc10d..b67a285fb9 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -311,7 +311,7 @@ func (s *Server) handleConnectorLogin(w http.ResponseWriter, r *http.Request) { `, action, value, authReq.ID) } else { - s.renderError(r, w, http.StatusInternalServerError, "Invalid binding type") + s.renderError(r, w, http.StatusInternalServerError, fmt.Sprintf("Invalid binding type: %s", bindingType)) return } default: