Skip to content
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
2 changes: 1 addition & 1 deletion .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: stable
go-version: '1.24.x'
- name: golangci-lint
uses: golangci/golangci-lint-action@v7
with:
Expand Down
9 changes: 9 additions & 0 deletions samlidp/samlidp.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,12 @@ func (s *Server) InitializeHTTP() {
mux.HandleFunc("PUT /shortcuts/{id}", s.HandlePutShortcut)
mux.HandleFunc("DELETE /shortcuts/{id}", s.HandleDeleteShortcut)
}

// ServeHTTP delegates to the initialized handler.
func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
if s.Handler == nil {
http.NotFound(w, r)
return
}
s.Handler.ServeHTTP(w, r)
}
83 changes: 53 additions & 30 deletions service_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,29 +304,24 @@ func (r *AuthnRequest) Redirect(relayState string, sp *ServiceProvider) (*url.UR
}

// We can't depend on Query().set() as order matters for signing
reqString := requestStr.String()
query := rv.RawQuery
if len(query) > 0 {
query += "&SAMLRequest=" + url.QueryEscape(requestStr.String())
query += "&" + string(samlRequest) + "=" + url.QueryEscape(reqString)
} else {
query += "SAMLRequest=" + url.QueryEscape(requestStr.String())
query += string(samlRequest) + "=" + url.QueryEscape(reqString)
}

if relayState != "" {
query += "&RelayState=" + relayState
query += "&RelayState=" + url.QueryEscape(relayState)
}
if len(sp.SignatureMethod) > 0 {
query += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod)
signingContext, err := GetSigningContext(sp)

if err != nil {
return nil, err
var errSig error
query, errSig = sp.signQuery(samlRequest, query, reqString, relayState)
if errSig != nil {
return nil, errSig
}

sig, err := signingContext.SignString(query)
if err != nil {
return nil, err
}
query += "&Signature=" + url.QueryEscape(base64.StdEncoding.EncodeToString(sig))
}

rv.RawQuery = query
Expand Down Expand Up @@ -1397,36 +1392,51 @@ func (sp *ServiceProvider) MakeRedirectLogoutRequest(nameID, relayState string)
if err != nil {
return nil, err
}
return req.Redirect(relayState), nil
return req.Redirect(relayState, sp)
}

// Redirect returns a URL suitable for using the redirect binding with the request
func (r *LogoutRequest) Redirect(relayState string) *url.URL {
func (r *LogoutRequest) Redirect(relayState string, sp *ServiceProvider) (*url.URL, error) {
w := &bytes.Buffer{}
w1 := base64.NewEncoder(base64.StdEncoding, w)
w2, _ := flate.NewWriter(w1, 9)
doc := etree.NewDocument()
doc.SetRoot(r.Element())
if _, err := doc.WriteTo(w2); err != nil {
panic(err)
return nil, err
}
if err := w2.Close(); err != nil {
panic(err)
return nil, err
}
if err := w1.Close(); err != nil {
panic(err)
return nil, err
}
rv, err := url.Parse(r.Destination)
if err != nil {
return nil, err
}

rv, _ := url.Parse(r.Destination)

query := rv.Query()
query.Set("SAMLRequest", w.String())
// We can't depend on Query().Set() as order matters for signing
reqString := w.String()
query := rv.RawQuery
if len(query) > 0 {
query += "&" + string(samlRequest) + "=" + url.QueryEscape(reqString)
} else {
query += string(samlRequest) + "=" + url.QueryEscape(reqString)
}
if relayState != "" {
query.Set("RelayState", relayState)
query += "&RelayState=" + url.QueryEscape(relayState)
}
rv.RawQuery = query.Encode()
if len(sp.SignatureMethod) > 0 {
var errSig error
query, errSig = sp.signQuery(samlRequest, query, reqString, relayState)
if errSig != nil {
return nil, errSig
}
}
rv.RawQuery = query

return rv
return rv, nil
}

// MakePostLogoutRequest creates a SAML authentication request using
Expand Down Expand Up @@ -1624,8 +1634,9 @@ func (sp *ServiceProvider) nameIDFormat() string {

// ValidateLogoutResponseRequest validates the LogoutResponse content from the request
func (sp *ServiceProvider) ValidateLogoutResponseRequest(req *http.Request) error {
if data := req.URL.Query().Get("SAMLResponse"); data != "" {
return sp.ValidateLogoutResponseRedirect(data)
query := req.URL.Query()
if data := query.Get("SAMLResponse"); data != "" {
return sp.ValidateLogoutResponseRedirect(query)
}

err := req.ParseForm()
Expand Down Expand Up @@ -1677,7 +1688,8 @@ func (sp *ServiceProvider) ValidateLogoutResponseForm(postFormData string) error
//
// URL Binding appears to be gzip / flate encoded
// See https://www.oasis-open.org/committees/download.php/20645/sstc-saml-tech-overview-2%200-draft-10.pdf 6.6
func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData string) error {
func (sp *ServiceProvider) ValidateLogoutResponseRedirect(query url.Values) error {
queryParameterData := query.Get("SAMLResponse")
retErr := &InvalidResponseError{
Now: TimeNow(),
}
Expand All @@ -1698,6 +1710,15 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData str
if err := xrv.Validate(bytes.NewReader(gr)); err != nil {
return err
}
querySig := false

if query.Get("Signature") != "" && query.Get("SigAlg") != "" {
if err := sp.validateQuerySig(query); err != nil {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see here that the PR validates a signature coming via query elements.

However, in line 1547 the code still calls sp.validateSignature(doc.Root()), which
expects a signature element in the XML document, and wants to verify it.

So, this PR seems to expect a signature in two places, required in the XML document, and optional in the query.

With keycloak I have a signature in the query element, and nothing in the XML document.
I expect this to fail.

I am missing something ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Added a querySig flag for myself, recording when a proper query sig is seen.
Check this flag if xml sign errors out with not present to avoid passing on that error

IOW

	querySig := false
	if query.Get("Signature") != "" && query.Get("SigAlg") != "" {
		if err := sp.validateQuerySig(query); err != nil {
			retErr.PrivateErr = err
			return retErr
		}
		querySig = true
	}

and

	if err := sp.validateSignature(doc.Root()); err != nil {
		if err != errSignatureElementNotPresent || !querySig {
			retErr.PrivateErr = err
			return retErr
		}
 	}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Tested the modification against Keycloak and works. As expected it verifies the detached sig, and then ignores the missing embedded sig.

Also extended further to generate a proper detached sig for logout requests. That solved issues with OKTA.

References:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@andreas-kupries - have you verified both IDP initiated and SP initiated single logouts? I cannot see any reference to handle LogoutRequest?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hi. According to our QA (First two points at rancher/rancher#38494 (comment)) both IDP initiated (logout of Okta) and SP initiated (logout of Rancher) were tested.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice, I was thinking is there a way we can bring the tested fixes to this repo?
cc @crewjam

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe the following signature verification checking is not needed for HTTP-Redirect binding SAMLResponse as it is already done in the previous step by this PR, looks like ValidateLogoutResponseRequest handle the HTTP-Redirect binding and ValidateLogoutResponseForm handles HTTP-POST Binding

if err := sp.validateSignature(doc.Root()); err != nil {

retErr.PrivateErr = err
return retErr
}
querySig = true
}

doc := etree.NewDocument()
if err := doc.ReadFromBytes(gr); err != nil {
Expand All @@ -1706,8 +1727,10 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData str
}

if err := sp.validateSignature(doc.Root()); err != nil {
retErr.PrivateErr = err
return retErr
if err != errSignatureElementNotPresent || !querySig {
retErr.PrivateErr = err
return retErr
}
}

var resp LogoutResponse
Expand Down
142 changes: 142 additions & 0 deletions service_provider_signed.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
package saml

import (
"crypto"
"crypto/rsa"
"crypto/sha1" // #nosec G505
"crypto/sha256"
"crypto/sha512"
"crypto/x509"
"encoding/base64"
"errors"
"fmt"
"net/url"

dsig "github.com/russellhaering/goxmldsig"
)

type reqType string

const (
samlRequest reqType = "SAMLRequest"
samlResponse reqType = "SAMLResponse"
)

var (
// ErrInvalidQuerySignature is returned when the query signature is invalid
ErrInvalidQuerySignature = errors.New("invalid query signature")
// ErrNoQuerySignature is returned when the query does not contain a signature
ErrNoQuerySignature = errors.New("query Signature or SigAlg not found")
)

// Sign Query with the SP private key.
// Returns provided query with the SigAlg and Signature parameters added.
func (sp *ServiceProvider) signQuery(reqT reqType, query, body, relayState string) (string, error) {
signingContext, err := GetSigningContext(sp)

// Encode Query as standard demands. query.Encode() is not standard compliant
toHash := string(reqT) + "=" + url.QueryEscape(body)
if relayState != "" {
toHash += "&RelayState=" + url.QueryEscape(relayState)
}

toHash += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod)

if err != nil {
return "", err
}

sig, err := signingContext.SignString(toHash)
if err != nil {
return "", err
}

query += "&SigAlg=" + url.QueryEscape(sp.SignatureMethod)
query += "&Signature=" + url.QueryEscape(base64.StdEncoding.EncodeToString(sig))

return query, nil
}

// validateSig validation of the signature of the Redirect Binding in query values
// Query is valid if return is nil
func (sp *ServiceProvider) validateQuerySig(query url.Values) error {
sig := query.Get("Signature")
alg := query.Get("SigAlg")
if sig == "" || alg == "" {
return ErrNoQuerySignature
}

certs, err := sp.getIDPSigningCerts()
if err != nil {
return err
}

respType := ""
switch {
case query.Get("SAMLResponse") != "":
respType = "SAMLResponse"
case query.Get("SAMLRequest") != "":
respType = "SAMLRequest"
default:
return fmt.Errorf("no SAMLResponse or SAMLRequest found in query")
}

// Encode Query as standard demands.
// query.Encode() is not standard compliant
// as query encoding order matters
res := respType + "=" + url.QueryEscape(query.Get(respType))

relayState := query.Get("RelayState")
if relayState != "" {
res += "&RelayState=" + url.QueryEscape(relayState)
}

res += "&SigAlg=" + url.QueryEscape(alg)

// Signature is base64 encoded
sigBytes, err := base64.StdEncoding.DecodeString(sig)
if err != nil {
return fmt.Errorf("failed to decode signature: %w", err)
}

var (
hashed []byte
hashAlg crypto.Hash
sigAlg x509.SignatureAlgorithm
)

// Hashed Query
switch alg {
case dsig.RSASHA256SignatureMethod:
hashed256 := sha256.Sum256([]byte(res))
hashed = hashed256[:]
hashAlg = crypto.SHA256
sigAlg = x509.SHA256WithRSA
case dsig.RSASHA512SignatureMethod:
hashed512 := sha512.Sum512([]byte(res))
hashed = hashed512[:]
hashAlg = crypto.SHA512
sigAlg = x509.SHA512WithRSA
case dsig.RSASHA1SignatureMethod:
hashed1 := sha1.Sum([]byte(res)) // #nosec G401
hashed = hashed1[:]
hashAlg = crypto.SHA1
sigAlg = x509.SHA1WithRSA
default:
return fmt.Errorf("unsupported signature algorithm: %s", alg)
}

// validate signature
for _, cert := range certs {
// verify cert is RSA
if cert.SignatureAlgorithm != sigAlg {
continue
}

if err := rsa.VerifyPKCS1v15(cert.PublicKey.(*rsa.PublicKey), hashAlg, hashed, sigBytes); err == nil {
return nil
}
}

return ErrInvalidQuerySignature
}
Loading
Loading