Merge pull request dexidp#885 from Calpicow/saml_issuer_fix

Add ssoIssuer to fix Response issuer checking
mmrath · Apr 6, 2017 · 1d935a5 · 1d935a5
2 parents 9ab4bb7 + f285128
commit 1d935a5
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 20 deletions.
diff --git a/Documentation/saml-connector.md b/Documentation/saml-connector.md
@@ -30,17 +30,23 @@ connectors:
     # CA to use when validating the SAML response.
     ca: /path/to/ca.pem
 
-    # CA's can also be provided inline as a base64'd blob. 
+    # CA's can also be provided inline as a base64'd blob.
     #
     # caData: ( RAW base64'd PEM encoded CA )
 
     # To skip signature validation, uncomment the following field. This should
     # only be used during testing and may be removed in the future.
-    # 
-    # insucreSkipSignatureValidation: true
+    #
+    # insecureSkipSignatureValidation: true
+
+    # Optional: Issuer value for AuthnRequest
+    entityIssuer: https://dex.example.com/callback
+
+    # Optional: Issuer value for SAML Response
+    ssoIssuer: https://saml.example.com/sso
 
     # Dex's callback URL. Must match the "Destination" attribute of all responses
-    # exactly.  
+    # exactly.
     redirectURI: https://dex.example.com/callback
 
     # Name of attributes in the returned assertions to map to ID token claims.

diff --git a/connector/saml/saml.go b/connector/saml/saml.go
@@ -81,8 +81,9 @@ type Config struct {
 	//
 	// https://www.oasis-open.org/committees/download.php/35391/sstc-saml-metadata-errata-2.0-wd-04-diff.pdf
 
-	Issuer string `json:"issuer"`
-	SSOURL string `json:"ssoURL"`
+	EntityIssuer string `json:"entityIssuer"`
+	SSOIssuer    string `json:"ssoIssuer"`
+	SSOURL       string `json:"ssoURL"`
 
 	// X509 CA file or raw data to verify XML signatures.
 	CA     string `json:"ca"`
@@ -154,7 +155,8 @@ func (c *Config) openConnector(logger logrus.FieldLogger) (*provider, error) {
 	}
 
 	p := &provider{
-		issuer:       c.Issuer,
+		entityIssuer: c.EntityIssuer,
+		ssoIssuer:    c.SSOIssuer,
 		ssoURL:       c.SSOURL,
 		now:          time.Now,
 		usernameAttr: c.UsernameAttr,
@@ -217,8 +219,9 @@ func (c *Config) openConnector(logger logrus.FieldLogger) (*provider, error) {
 }
 
 type provider struct {
-	issuer string
-	ssoURL string
+	entityIssuer string
+	ssoIssuer    string
+	ssoURL       string
 
 	now func() time.Time
 
@@ -251,10 +254,10 @@ func (p *provider) POSTData(s connector.Scopes, id string) (action, value string
 		},
 		AssertionConsumerServiceURL: p.redirectURI,
 	}
-	if p.issuer != "" {
+	if p.entityIssuer != "" {
 		// Issuer for the request is optional. For example, okta always ignores
 		// this value.
-		r.Issuer = &issuer{Issuer: p.issuer}
+		r.Issuer = &issuer{Issuer: p.entityIssuer}
 	}
 
 	data, err := xml.MarshalIndent(r, "", "  ")
@@ -287,8 +290,8 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
 	}
 
 	if rootElementSigned {
-		if p.issuer != "" && resp.Issuer != nil && resp.Issuer.Issuer != p.issuer {
-			return ident, fmt.Errorf("expected Issuer value %s, got %s", p.issuer, resp.Issuer.Issuer)
+		if p.ssoIssuer != "" && resp.Issuer != nil && resp.Issuer.Issuer != p.ssoIssuer {
+			return ident, fmt.Errorf("expected Issuer value %s, got %s", p.entityIssuer, resp.Issuer.Issuer)
 		}
 
 		// Verify InResponseTo value matches the expected ID associated with

diff --git a/connector/saml/saml_test.go b/connector/saml/saml_test.go
@@ -278,14 +278,14 @@ func (r responseTest) run(t *testing.T) {
 }
 
 const (
-	defaultIssuer      = "http://www.okta.com/exk91cb99lKkKSYoy0h7"
+	defaultSSOIssuer   = "http://www.okta.com/exk91cb99lKkKSYoy0h7"
 	defaultRedirectURI = "http://localhost:5556/dex/callback"
 
 	// Response ID embedded in our testdata.
 	testDataResponseID = "_fd1b3ef9-ec09-44a7-a66b-0d39c250f6a0"
 )
 
-// Depricated: Use testing framework established above.
+// Deprecated: Use testing framework established above.
 func runVerify(t *testing.T, ca string, resp string, shouldSucceed bool) {
 	cert, err := loadCert(ca)
 	if err != nil {
@@ -311,18 +311,18 @@ func runVerify(t *testing.T, ca string, resp string, shouldSucceed bool) {
 	}
 }
 
-// Depricated: Use testing framework established above.
-func newProvider(issuer string, redirectURI string) *provider {
-	if issuer == "" {
-		issuer = defaultIssuer
+// Deprecated: Use testing framework established above.
+func newProvider(ssoIssuer string, redirectURI string) *provider {
+	if ssoIssuer == "" {
+		ssoIssuer = defaultSSOIssuer
 	}
 	if redirectURI == "" {
 		redirectURI = defaultRedirectURI
 	}
 	now, _ := time.Parse(time.RFC3339, "2017-01-24T20:48:41Z")
 	timeFunc := func() time.Time { return now }
 	return &provider{
-		issuer:       issuer,
+		ssoIssuer:    ssoIssuer,
 		ssoURL:       "http://idp.org/saml/sso",
 		now:          timeFunc,
 		usernameAttr: "user",