Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support SAML HTTP redirect binding #1045

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Documentation/saml-connector.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,19 @@ connectors:
# urn:oasis:names:tc:SAML:2.0:nameid-format:persistent
#
nameIDPolicyFormat: persistent

# Optional: Specify binding to use, default is HTTP-POST
#
# requestBinding specifies how AuthnRequest is sent to identity provider
# responseBinding specifies how identity provider calls back to dex
#
# Some identity provider may require specific binding
# supported values:
# - post: use HTTP-POST binding (default)
# - redirect: use HTTP redirect binding
#
# requestBinding: post
# responseBinding: post
```

A minimal working configuration might look like:
Expand Down
18 changes: 12 additions & 6 deletions connector/connector.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,26 +65,32 @@ type CallbackConnector interface {
HandleCallback(s Scopes, r *http.Request) (identity Identity, err error)
}

// SAMLConnector bindings
const (
SAMLBindingPOST = "post"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this should probably be httpPost and httpRedirect, right? Since the other ones are SOAP based.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the const name should be "SAMLHTTPPOST" or the value should be "httpPost"? I'm using the same value from config option. I'm OK if we decide to use "http-post", "http-redirect" in values of config option "binding". But I'd like to use the same value internally in code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant the "post" and "redirect" values.

Actually, I think the way it currently is is fine. Let's keep it as is.

SAMLBindingRedirect = "redirect"
)

// SAMLConnector represents SAML connectors which implement the HTTP POST binding.
// RelayState is handled by the server.
//
// See: https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf
// "3.5 HTTP POST Binding"
type SAMLConnector interface {
// POSTData returns an encoded SAML request and SSO URL for the server to
// render a POST form with.
// AuthnRequest builds an encoded SAML request and SSO URL for the server to
// render a POST form or reply a redirection depending on binding.
//
// POSTData should encode the provided request ID in the returned serialized
// AuthnRequest should encode the provided request ID in the returned serialized
// SAML request.
POSTData(s Scopes, requestID string) (sooURL, samlRequest string, err error)
AuthnRequest(s Scopes, requestID string) (binding, ssoURL, samlRequest string, err error)

// HandlePOST decodes, verifies, and maps attributes from the SAML response.
// HandleResponse decodes, verifies, and maps attributes from the SAML response.
// It passes the expected value of the "InResponseTo" response field, which
// the connector must ensure matches the response value.
//
// See: https://www.oasis-open.org/committees/download.php/35711/sstc-saml-core-errata-2.0-wd-06-diff.pdf
// "3.2.2 Complex Type StatusResponseType"
HandlePOST(s Scopes, samlResponse, inResponseTo string) (identity Identity, err error)
HandleResponse(s Scopes, binding, samlResponse, inResponseTo string) (identity Identity, err error)
}

// RefreshConnector is a connector that can update the client claims.
Expand Down
104 changes: 97 additions & 7 deletions connector/saml/saml.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@
package saml

import (
"bytes"
"compress/flate"
"crypto/x509"
"encoding/base64"
"encoding/pem"
"encoding/xml"
"errors"
"fmt"
"io"
"io/ioutil"
"strings"
"time"
Expand Down Expand Up @@ -57,6 +60,10 @@ var (
nameIDformatTransient,
}
nameIDFormatLookup = make(map[string]string)
validBindings = map[string]bool{
connector.SAMLBindingPOST: true,
connector.SAMLBindingRedirect: true,
}
)

func init() {
Expand All @@ -72,6 +79,14 @@ func init() {
}
}

// verifyBinding verifies binding from config
func verifyBinding(binding string) error {
if _, exist := validBindings[binding]; !exist {
return fmt.Errorf("unsupported binding: %s", binding)
}
return nil
}

// Config represents configuration options for the SAML provider.
type Config struct {
// TODO(ericchiang): A bunch of these fields could be auto-filled if
Expand Down Expand Up @@ -113,6 +128,10 @@ type Config struct {
// urn:oasis:names:tc:SAML:2.0:nameid-format:persistent
//
NameIDPolicyFormat string `json:"nameIDPolicyFormat"`

// Specify which binding to use, default is post
RequestBinding string `json:"requestBinding"`
ResponseBinding string `json:"responseBinding"`
}

type certStore struct {
Expand Down Expand Up @@ -165,6 +184,21 @@ func (c *Config) openConnector(logger logrus.FieldLogger) (*provider, error) {
logger: logger,

nameIDPolicyFormat: c.NameIDPolicyFormat,

requestBinding: c.RequestBinding,
responseBinding: c.ResponseBinding,
}

if p.requestBinding == "" {
p.requestBinding = connector.SAMLBindingPOST
} else if err := verifyBinding(p.requestBinding); err != nil {
return nil, err
}

if p.responseBinding == "" {
p.responseBinding = connector.SAMLBindingPOST
} else if err := verifyBinding(p.responseBinding); err != nil {
return nil, err
}

if p.nameIDPolicyFormat == "" {
Expand Down Expand Up @@ -236,11 +270,13 @@ type provider struct {

nameIDPolicyFormat string

requestBinding string
responseBinding string

logger logrus.FieldLogger
}

func (p *provider) POSTData(s connector.Scopes, id string) (action, value string, err error) {

func (p *provider) AuthnRequest(s connector.Scopes, id string) (binding, ssoURL, value string, err error) {
r := &authnRequest{
ProtocolBinding: bindingPOST,
ID: id,
Expand All @@ -252,6 +288,11 @@ func (p *provider) POSTData(s connector.Scopes, id string) (action, value string
},
AssertionConsumerServiceURL: p.redirectURI,
}

if p.responseBinding == connector.SAMLBindingRedirect {
r.ProtocolBinding = bindingRedirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not set this if it's a POST?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already set to POST in a few lines above when constructing the authnRequest.

}

if p.entityIssuer != "" {
// Issuer for the request is optional. For example, okta always ignores
// this value.
Expand All @@ -260,15 +301,23 @@ 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)
}

// for redirect binding, SAMLRequest must be deflated
if p.requestBinding == connector.SAMLBindingRedirect {
data, err = compressRequest(data)
if err != nil {
return "", "", "", fmt.Errorf("deflate 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.requestBinding, p.ssoURL, base64.StdEncoding.EncodeToString(data), nil
}

// HandlePOST interprets a request from a SAML provider attempting to verify a
// HandleResponse interprets a request from a SAML provider attempting to verify a
// user's identity.
//
// The steps taken are:
Expand All @@ -277,12 +326,24 @@ func (p *provider) POSTData(s connector.Scopes, id string) (action, value string
// * 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) {
func (p *provider) HandleResponse(s connector.Scopes,
binding, samlResponse, inResponseTo string) (ident connector.Identity, err error) {
// enforce responseBinding
if binding != p.responseBinding {
return ident, fmt.Errorf("unexpected response binding %s, expect %s as configured", binding, p.responseBinding)
}
rawResp, err := base64.StdEncoding.DecodeString(samlResponse)
if err != nil {
return ident, fmt.Errorf("decode response: %v", err)
}

// with HTTP redirect binding, response is compressed
if p.responseBinding == connector.SAMLBindingRedirect {
if rawResp, err = decompressResponse(rawResp); err != nil {
return ident, fmt.Errorf("inflate response: %v", err)
}
}

// Root element is allowed to not be signed if the Assertion element is.
rootElementSigned := true
if p.validator != nil {
Expand All @@ -293,7 +354,7 @@ func (p *provider) HandlePOST(s connector.Scopes, samlResponse, inResponseTo str
}

var resp response
if err := xml.Unmarshal(rawResp, &resp); err != nil {
if err = xml.Unmarshal(rawResp, &resp); err != nil {
return ident, fmt.Errorf("unmarshal response: %v", err)
}

Expand Down Expand Up @@ -598,3 +659,32 @@ func before(now, notBefore time.Time) bool {
func after(now, notOnOrAfter time.Time) bool {
return now.After(notOnOrAfter.Add(allowedClockDrift))
}

func compressRequest(data []byte) ([]byte, error) {
var buf bytes.Buffer
zw, err := flate.NewWriter(&buf, flate.DefaultCompression)
if err != nil {
return nil, err
}
defer zw.Close()
if _, err := zw.Write(data); err != nil {
return nil, err
}
if err := zw.Flush(); err != nil {
return nil, err
}
return buf.Bytes(), nil
}

func decompressResponse(data []byte) ([]byte, error) {
zr := flate.NewReader(bytes.NewReader(data))
defer zr.Close()
result, err := ioutil.ReadAll(zr)
// the compressed data has no frame, so we don't know the length
// of decompressed data. according to impl of compress/flate,
// treat io.EOF/io.ErrUnexpectedEOF as completion of decompression.
if err == io.EOF || err == io.ErrUnexpectedEOF {
err = nil
}
return result, err
}
38 changes: 36 additions & 2 deletions connector/saml/saml_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ type responseTest struct {
emailAttr string
groupsAttr string

// use redirect binding for response
responseUseRedirect bool

// Expected outcome of the test.
wantErr bool
wantIdent connector.Identity
Expand Down Expand Up @@ -262,6 +265,27 @@ func TestTwoAssertionFirstSigned(t *testing.T) {
test.run(t)
}

// TestResponseRedirectBinding provides a response for HTTP redirect binding
func TestResponseRedirectBinding(t *testing.T) {
test := responseTest{
caFile: "testdata/ca.crt",
respFile: "testdata/good-resp.xml",
now: "2017-04-04T04:34:59.330Z",
usernameAttr: "Name",
emailAttr: "email",
inResponseTo: "6zmm5mguyebwvajyf2sdwwcw6m",
redirectURI: "http://127.0.0.1:5556/dex/callback",
wantIdent: connector.Identity{
UserID: "[email protected]",
Username: "Eric",
Email: "[email protected]",
EmailVerified: true,
},
responseUseRedirect: true,
}
test.run(t)
}

func loadCert(ca string) (*x509.Certificate, error) {
data, err := ioutil.ReadFile(ca)
if err != nil {
Expand All @@ -283,8 +307,13 @@ func (r responseTest) run(t *testing.T) {
RedirectURI: r.redirectURI,
EntityIssuer: r.entityIssuer,
// Never logging in, don't need this.
SSOURL: "http://foo.bar/",
SSOURL: "http://foo.bar/",
ResponseBinding: connector.SAMLBindingPOST,
}
if r.responseUseRedirect {
c.ResponseBinding = connector.SAMLBindingRedirect
}

now, err := time.Parse(timeFormat, r.now)
if err != nil {
t.Fatalf("parse test time: %v", err)
Expand All @@ -299,13 +328,18 @@ func (r responseTest) run(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if r.responseUseRedirect {
if resp, err = compressRequest(resp); err != nil {
t.Fatal(err)
}
}
samlResp := base64.StdEncoding.EncodeToString(resp)

scopes := connector.Scopes{
OfflineAccess: false,
Groups: true,
}
ident, err := conn.HandlePOST(scopes, samlResp, r.inResponseTo)
ident, err := conn.HandleResponse(scopes, c.ResponseBinding, samlResp, r.inResponseTo)
if err != nil {
if !r.wantErr {
t.Fatalf("handle response: %v", err)
Expand Down
Loading