From 362e0798a4f0c6816e9b1c885aff1e089be45259 Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Thu, 6 Apr 2017 17:11:28 -0700 Subject: [PATCH] connector/saml: clean up SAML verification logic and comments --- connector/saml/saml.go | 264 ++++++++++++++++++++---------------- connector/saml/saml_test.go | 197 +++++++-------------------- connector/saml/types.go | 3 +- 3 files changed, 195 insertions(+), 269 deletions(-) diff --git a/connector/saml/saml.go b/connector/saml/saml.go index 226eef31..fad270c6 100644 --- a/connector/saml/saml.go +++ b/connector/saml/saml.go @@ -2,10 +2,8 @@ package saml import ( - "crypto/rand" "crypto/x509" "encoding/base64" - "encoding/hex" "encoding/pem" "encoding/xml" "errors" @@ -270,12 +268,22 @@ func (p *provider) POSTData(s connector.Scopes, id string) (action, value string return p.ssoURL, base64.StdEncoding.EncodeToString(data), nil } +// HandlePOST interprets a request from a SAML provider attempting to verify a +// user's identity. +// +// The steps taken are: +// +// * Verify signature on XML document (or verify sig on assertion elements). +// * Verify various parts of the Assertion element. Conditions, audience, etc. +// * Map the Assertion's attribute elements to user info. +// func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo string) (ident connector.Identity, err error) { rawResp, err := base64.StdEncoding.DecodeString(samlResponse) if err != nil { return ident, fmt.Errorf("decode response: %v", err) } + // Root element is allowed to not be signed if the Assertion element is. rootElementSigned := true if p.validator != nil { rawResp, rootElementSigned, err = verifyResponseSig(p.validator, rawResp) @@ -289,6 +297,8 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str return ident, fmt.Errorf("unmarshal response: %v", err) } + // If the root element isn't signed, there's no reason to inspect these + // elements. They're not verified. if rootElementSigned { if p.ssoIssuer != "" && resp.Issuer != nil && resp.Issuer.Issuer != p.ssoIssuer { return ident, fmt.Errorf("expected Issuer value %s, got %s", p.ssoIssuer, resp.Issuer.Issuer) @@ -303,10 +313,14 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str // Destination is optional. if resp.Destination != "" && resp.Destination != p.redirectURI { return ident, fmt.Errorf("expected destination %q got %q", p.redirectURI, resp.Destination) - } - if err = p.validateStatus(&resp); err != nil { + // Status is a required element. + if resp.Status == nil { + return ident, fmt.Errorf("Response did not contain a Status element") + } + + if err = p.validateStatus(resp.Status); err != nil { return ident, err } } @@ -315,16 +329,25 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str if assertion == nil { return ident, fmt.Errorf("response did not contain an assertion") } + + // Subject is usually optional, but we need it for the user ID, so complain + // if it's not present. subject := assertion.Subject if subject == nil { return ident, fmt.Errorf("response did not contain a subject") } - if err = p.validateConditions(assertion); err != nil { + // Validate that the response is to the request we originally sent. + if err = p.validateSubject(subject, inResponseTo); err != nil { return ident, err } - if err = p.validateSubjectConfirmation(subject); err != nil { - return ident, err + + // Conditions element is optional, but must be validated if present. + if assertion.Conditions != nil { + // Validate that dex is the intended audience of this response. + if err = p.validateConditions(assertion.Conditions); err != nil { + return ident, err + } } switch { @@ -336,53 +359,57 @@ 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") } + // 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 s.Groups && p.groupsAttr != "" { - 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 !s.Groups || p.groupsAttr == "" { + // Groups not requested or not configured. We're done. + return ident, nil } + // 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 + } return ident, nil } -// Validate that the StatusCode of the Response is success. -// Otherwise return a human readable message to the end user -func (p *provider) validateStatus(resp *response) error { - // Status is mandatory in the Response type - status := resp.Status - if status == nil { - return fmt.Errorf("response did not contain a Status") - } +// validateStatus verifies that the response has a good status code or +// formats a human readble error based on the bad status. +func (p *provider) validateStatus(status *status) error { // StatusCode is mandatory in the Status type statusCode := status.StatusCode if statusCode == nil { return fmt.Errorf("response did not contain a StatusCode") } + if statusCode.Value != statusCodeSuccess { parts := strings.Split(statusCode.Value, ":") lastPart := parts[len(parts)-1] @@ -396,96 +423,107 @@ func (p *provider) validateStatus(resp *response) error { return nil } -// Multiple subject SubjectConfirmation can be in the assertion -// and at least one SubjectConfirmation must be valid. +// validateSubject ensures the response is to the request we expect. +// // This is described in the spec "Profiles for the OASIS Security // Assertion Markup Language" in section 3.3 Bearer. // see https://www.oasis-open.org/committees/download.php/35389/sstc-saml-profiles-errata-2.0-wd-06-diff.pdf -func (p *provider) validateSubjectConfirmation(subject *subject) error { - validSubjectConfirmation := false - subjectConfirmations := subject.SubjectConfirmations - if subjectConfirmations != nil && len(subjectConfirmations) > 0 { - for _, subjectConfirmation := range subjectConfirmations { - // skip if method is wrong - method := subjectConfirmation.Method - if method != "" && method != subjectConfirmationMethodBearer { - continue +// +// Some of these fields are optional, but we're going to be strict here since +// we have no other way of guarenteeing that this is actually the response to +// the request we expect. +func (p *provider) validateSubject(subject *subject, inResponseTo string) error { + // Optional according to the spec, but again, we're going to be strict here. + if len(subject.SubjectConfirmations) == 0 { + return fmt.Errorf("Subject contained no SubjectConfrimations") + } + + var errs []error + // One of these must match our assumptions, not all. + for _, c := range subject.SubjectConfirmations { + err := func() error { + if c.Method != subjectConfirmationMethodBearer { + return fmt.Errorf("unexpected subject confirmation method: %v", c.Method) } - subjectConfirmationData := subjectConfirmation.SubjectConfirmationData - if subjectConfirmationData == nil { - continue + + data := c.SubjectConfirmationData + if data == nil { + return fmt.Errorf("SubjectConfirmation contained no SubjectConfirmationData") } - inResponseTo := subjectConfirmationData.InResponseTo - if inResponseTo != "" { - // TODO also validate InResponseTo if present + if data.InResponseTo != inResponseTo { + return fmt.Errorf("expected SubjectConfirmationData InResponseTo value %q, got %q", inResponseTo, data.InResponseTo) } - // only validate that subjectConfirmationData is not expired + + notBefore := time.Time(data.NotBefore) + notOnOrAfter := time.Time(data.NotOnOrAfter) now := p.now() - notOnOrAfter := time.Time(subjectConfirmationData.NotOnOrAfter) - if !notOnOrAfter.IsZero() { - if now.After(notOnOrAfter) { - continue - } + if !notBefore.IsZero() && before(now, notBefore) { + return fmt.Errorf("at %s got response that cannot be processed before %s", now, notBefore) } - // validate recipient if present - recipient := subjectConfirmationData.Recipient - if recipient != "" && recipient != p.redirectURI { - continue + if !notOnOrAfter.IsZero() && after(now, notOnOrAfter) { + return fmt.Errorf("at %s got response that cannot be processed because it expired at %s", now, notOnOrAfter) } - validSubjectConfirmation = true + if r := data.Recipient; r != "" && r != p.redirectURI { + return fmt.Errorf("expected Recipient %q got %q", p.redirectURI, r) + } + return nil + }() + if err == nil { + // Subject is valid. + return nil } + errs = append(errs, err) } - if !validSubjectConfirmation { - return fmt.Errorf("no valid SubjectConfirmation was found on this Response") + + if len(errs) == 1 { + return fmt.Errorf("failed to validate subject confirmation: %v", errs[0]) } - return nil + return fmt.Errorf("failed to validate subject confirmation: %v", errs) } -// Validates the Conditions element and all of it's content +// validationConditions ensures that dex is the intended audience +// for the request, and not another service provider. // // See: https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf // "2.3.3 Element " -func (p *provider) validateConditions(assertion *assertion) error { - // Checks if a Conditions element exists - conditions := assertion.Conditions - if conditions == nil { - return nil - } - // Validates Assertion timestamps +func (p *provider) validateConditions(conditions *conditions) error { + // Ensure the conditions haven't expired. now := p.now() notBefore := time.Time(conditions.NotBefore) - if !notBefore.IsZero() { - if now.Add(allowedClockDrift).Before(notBefore) { - return fmt.Errorf("at %s got response that cannot be processed before %s", now, notBefore) - } + if !notBefore.IsZero() && before(now, notBefore) { + return fmt.Errorf("at %s got response that cannot be processed before %s", now, notBefore) } + notOnOrAfter := time.Time(conditions.NotOnOrAfter) - if !notOnOrAfter.IsZero() { - if now.After(notOnOrAfter.Add(allowedClockDrift)) { - return fmt.Errorf("at %s got response that cannot be processed because it expired at %s", now, notOnOrAfter) + if !notOnOrAfter.IsZero() && after(now, notOnOrAfter) { + return fmt.Errorf("at %s got response that cannot be processed because it expired at %s", now, notOnOrAfter) + } + + // Sometimes, dex's issuer string can be different than the redirect URI, + // but if dex's issuer isn't explicitly provided assume the redirect URI. + expAud := p.entityIssuer + if expAud == "" { + expAud = p.redirectURI + } + + // AudienceRestriction elements indicate the intended audience(s) of an + // assertion. If dex isn't in these audiences, reject the assertion. + // + // Note that if there are multiple AudienceRestriction elements, each must + // individually contain dex in their audience list. + for _, r := range conditions.AudienceRestriction { + values := make([]string, len(r.Audiences)) + issuerInAudiences := false + for i, aud := range r.Audiences { + if aud.Value == expAud { + issuerInAudiences = true + break + } + values[i] = aud.Value } - } - // Validates audience - audienceValue := p.entityIssuer - if audienceValue == "" { - audienceValue = p.redirectURI - } - audienceRestriction := conditions.AudienceRestriction - if audienceRestriction != nil { - audiences := audienceRestriction.Audiences - if audiences != nil && len(audiences) > 0 { - values := make([]string, len(audiences)) - issuerInAudiences := false - for i, audience := range audiences { - if audience.Value == audienceValue { - issuerInAudiences = true - break - } - values[i] = audience.Value - } - if !issuerInAudiences { - return fmt.Errorf("required audience %s was not in Response audiences %s", audienceValue, values) - } + + if !issuerInAudiences { + return fmt.Errorf("required audience %s was not in Response audiences %s", expAud, values) } } return nil @@ -544,24 +582,14 @@ func verifyResponseSig(validator *dsig.ValidationContext, data []byte) (signed [ return signed, false, err } -func uuidv4() string { - u := make([]byte, 16) - if _, err := rand.Read(u); err != nil { - panic(err) - } - u[6] = (u[6] | 0x40) & 0x4F - u[8] = (u[8] | 0x80) & 0xBF - - r := make([]byte, 36) - r[8] = '-' - r[13] = '-' - r[18] = '-' - r[23] = '-' - hex.Encode(r, u[0:4]) - hex.Encode(r[9:], u[4:6]) - hex.Encode(r[14:], u[6:8]) - hex.Encode(r[19:], u[8:10]) - hex.Encode(r[24:], u[10:]) - - return string(r) +// before determines if a given time is before the current time, with an +// allowed clock drift. +func before(now, notBefore time.Time) bool { + return now.Add(allowedClockDrift).Before(notBefore) +} + +// after determines if a given time is after the current time, with an +// allowed clock drift. +func after(now, notOnOrAfter time.Time) bool { + return now.After(notOnOrAfter.Add(allowedClockDrift)) } diff --git a/connector/saml/saml_test.go b/connector/saml/saml_test.go index a84b7071..2d73ff50 100644 --- a/connector/saml/saml_test.go +++ b/connector/saml/saml_test.go @@ -7,7 +7,6 @@ import ( "errors" "io/ioutil" "sort" - "strings" "testing" "time" @@ -47,6 +46,7 @@ type responseTest struct { now string inResponseTo string redirectURI string + entityIssuer string // Attribute customization. usernameAttr string @@ -196,6 +196,51 @@ func TestAssertionSignedNotResponse(t *testing.T) { test.run(t) } +func TestInvalidSubjectInResponseTo(t *testing.T) { + test := responseTest{ + caFile: "testdata/ca.crt", + respFile: "testdata/assertion-signed.xml", + now: "2017-04-04T04:34:59.330Z", + usernameAttr: "Name", + emailAttr: "email", + inResponseTo: "invalid-id", // Bad InResponseTo value. + redirectURI: "http://127.0.0.1:5556/dex/callback", + wantErr: true, + } + test.run(t) +} + +func TestInvalidSubjectRecipient(t *testing.T) { + test := responseTest{ + caFile: "testdata/ca.crt", + respFile: "testdata/assertion-signed.xml", + now: "2017-04-04T04:34:59.330Z", + usernameAttr: "Name", + emailAttr: "email", + inResponseTo: "6zmm5mguyebwvajyf2sdwwcw6m", + redirectURI: "http://bad.com/dex/callback", // Doesn't match Recipient value. + wantErr: true, + } + test.run(t) +} + +func TestInvalidAssertionAudience(t *testing.T) { + test := responseTest{ + caFile: "testdata/ca.crt", + respFile: "testdata/assertion-signed.xml", + now: "2017-04-04T04:34:59.330Z", + usernameAttr: "Name", + emailAttr: "email", + inResponseTo: "6zmm5mguyebwvajyf2sdwwcw6m", + redirectURI: "http://127.0.0.1:5556/dex/callback", + // EntityIssuer overrides RedirectURI when determining the expected + // audience. In this case, ensure the audience is invalid. + entityIssuer: "http://localhost:5556/dex/callback", + wantErr: true, + } + test.run(t) +} + // TestTwoAssertionFirstSigned tries to catch an edge case where an attacker // provides a second assertion that's not signed. func TestTwoAssertionFirstSigned(t *testing.T) { @@ -236,6 +281,7 @@ func (r responseTest) run(t *testing.T) { EmailAttr: r.emailAttr, GroupsAttr: r.groupsAttr, RedirectURI: r.redirectURI, + EntityIssuer: r.entityIssuer, // Never logging in, don't need this. SSOURL: "http://foo.bar/", } @@ -355,152 +401,3 @@ func TestVerifySignedMessageAndSignedAssertion(t *testing.T) { func TestVerifyUnsignedMessageAndUnsignedAssertion(t *testing.T) { runVerify(t, "testdata/idp-cert.pem", "testdata/idp-resp.xml", false) } - -func TestValidateStatus(t *testing.T) { - p := newProvider("", "") - var err error - resp := response{} - // Test missing Status element - err = p.validateStatus(&resp) - if err == nil || !strings.HasSuffix(err.Error(), `Status`) { - t.Fatalf("validation should fail with missing Status") - } - // Test missing StatusCode element - resp.Status = &status{} - err = p.validateStatus(&resp) - if err == nil || !strings.HasSuffix(err.Error(), `StatusCode`) { - t.Fatalf("validation should fail with missing StatusCode") - } - // Test failed request without StatusMessage - resp.Status.StatusCode = &statusCode{ - Value: ":Requester", - } - err = p.validateStatus(&resp) - if err == nil || !strings.HasSuffix(err.Error(), `"Requester"`) { - t.Fatalf("validation should fail with code %q", "Requester") - } - // Test failed request with StatusMessage - resp.Status.StatusMessage = &statusMessage{ - Value: "Failed", - } - err = p.validateStatus(&resp) - if err == nil || !strings.HasSuffix(err.Error(), `"Requester" -> Failed`) { - t.Fatalf("validation should fail with code %q and message %q", "Requester", "Failed") - } -} - -func TestValidateSubjectConfirmation(t *testing.T) { - p := newProvider("", "") - var err error - var notAfter time.Time - subj := &subject{} - // Subject without any SubjectConfirmation - err = p.validateSubjectConfirmation(subj) - if err == nil { - t.Fatalf("validation of %q should fail", "Subject without any SubjectConfirmation") - } - // SubjectConfirmation without Method and SubjectConfirmationData - subj.SubjectConfirmations = []subjectConfirmation{subjectConfirmation{}} - err = p.validateSubjectConfirmation(subj) - if err == nil { - t.Fatalf("validation of %q should fail", "SubjectConfirmation without Method and SubjectConfirmationData") - } - // SubjectConfirmation with invalid Method and no SubjectConfirmationData - subj.SubjectConfirmations = []subjectConfirmation{subjectConfirmation{ - Method: "invalid", - }} - err = p.validateSubjectConfirmation(subj) - if err == nil { - t.Fatalf("validation of %q should fail", "SubjectConfirmation with invalid Method and no SubjectConfirmationData") - } - // SubjectConfirmation with valid Method and empty SubjectConfirmationData - subjConfirmationData := subjectConfirmationData{} - subj.SubjectConfirmations = []subjectConfirmation{subjectConfirmation{ - Method: "urn:oasis:names:tc:SAML:2.0:cm:bearer", - SubjectConfirmationData: &subjConfirmationData, - }} - err = p.validateSubjectConfirmation(subj) - if err != nil { - t.Fatalf("validation of %q should succeed", "SubjectConfirmation with valid Method and empty SubjectConfirmationData") - } - // SubjectConfirmationData with invalid Recipient - subjConfirmationData.Recipient = "invalid" - err = p.validateSubjectConfirmation(subj) - if err == nil { - t.Fatalf("validation of %q should fail", "SubjectConfirmationData with invalid Recipient") - } - // expired SubjectConfirmationData - notAfter = p.now().Add(-time.Duration(60) * time.Second) - subjConfirmationData.NotOnOrAfter = xmlTime(notAfter) - subjConfirmationData.Recipient = defaultRedirectURI - err = p.validateSubjectConfirmation(subj) - if err == nil { - t.Fatalf("validation of %q should fail", " expired SubjectConfirmationData") - } - // valid SubjectConfirmationData - notAfter = p.now().Add(+time.Duration(60) * time.Second) - subjConfirmationData.NotOnOrAfter = xmlTime(notAfter) - subjConfirmationData.Recipient = defaultRedirectURI - err = p.validateSubjectConfirmation(subj) - if err != nil { - t.Fatalf("validation of %q should succed", "valid SubjectConfirmationData") - } -} - -func TestValidateConditions(t *testing.T) { - p := newProvider("", "") - var err error - var notAfter, notBefore time.Time - cond := conditions{ - AudienceRestriction: &audienceRestriction{}, - } - assert := &assertion{} - // Assertion without Conditions - err = p.validateConditions(assert) - if err != nil { - t.Fatalf("validation of %q should succeed", "Assertion without Conditions") - } - // Assertion with empty Conditions - assert.Conditions = &cond - err = p.validateConditions(assert) - if err != nil { - t.Fatalf("validation of %q should succeed", "Assertion with empty Conditions") - } - // Conditions with valid timestamps - notBefore = p.now().Add(-time.Duration(60) * time.Second) - notAfter = p.now().Add(+time.Duration(60) * time.Second) - cond.NotBefore = xmlTime(notBefore) - cond.NotOnOrAfter = xmlTime(notAfter) - err = p.validateConditions(assert) - if err != nil { - t.Fatalf("validation of %q should succeed", "Conditions with valid timestamps") - } - // Conditions where notBefore is 45 seconds after now - notBefore = p.now().Add(+time.Duration(45) * time.Second) - cond.NotBefore = xmlTime(notBefore) - err = p.validateConditions(assert) - if err == nil { - t.Fatalf("validation of %q should fail", "Conditions where notBefore is 45 seconds after now") - } - // Conditions where notBefore is 15 seconds after now - notBefore = p.now().Add(+time.Duration(15) * time.Second) - cond.NotBefore = xmlTime(notBefore) - err = p.validateConditions(assert) - if err != nil { - t.Fatalf("validation of %q should succeed", "Conditions where notBefore is 15 seconds after now") - } - // Audiences contains the redirectURI - validAudience := audience{Value: p.redirectURI} - cond.AudienceRestriction.Audiences = []audience{validAudience} - err = p.validateConditions(assert) - if err != nil { - t.Fatalf("validation of %q should succeed: %v", "Audiences contains the redirectURI", err) - } - // Audiences is not empty and not contains the issuer - invalidAudience := audience{Value: "invalid"} - cond.AudienceRestriction.Audiences = []audience{invalidAudience} - err = p.validateConditions(assert) - if err == nil { - t.Fatalf("validation of %q should succeed", "Audiences is not empty and not contains the issuer") - } -} diff --git a/connector/saml/types.go b/connector/saml/types.go index 076067b7..1f7d919e 100644 --- a/connector/saml/types.go +++ b/connector/saml/types.go @@ -86,6 +86,7 @@ type nameID struct { type subjectConfirmationData struct { XMLName xml.Name `xml:"urn:oasis:names:tc:SAML:2.0:assertion SubjectConfirmationData"` + NotBefore xmlTime `xml:"NotBefore,attr,omitempty"` NotOnOrAfter xmlTime `xml:"NotOnOrAfter,attr,omitempty"` Recipient string `xml:"Recipient,attr,omitempty"` InResponseTo string `xml:"InResponseTo,attr,omitempty"` @@ -115,7 +116,7 @@ type conditions struct { NotBefore xmlTime `xml:"NotBefore,attr,omitempty"` NotOnOrAfter xmlTime `xml:"NotOnOrAfter,attr,omitempty"` - AudienceRestriction *audienceRestriction `xml:"AudienceRestriction,omitempty"` + AudienceRestriction []audienceRestriction `xml:"AudienceRestriction,omitempty"` } type statusCode struct {