Verify manual SAML IdP signatures with the configured certificate

SAML providers configured with an SSO URL and a PEM signing certificate
(idpCertificate or idpCertFile) instead of IdP metadata never verified an
IdP signature. buildManualMetadata put the certificate into the IdP
descriptor's X509Certificate field as a PEM block, but crewjam/saml
base64-decodes that field as DER, so every ACS Response and LogoutResponse
failed with "cannot parse certificate: illegal base64 data at input byte 0"
before any key was compared. Providers configured from IdP metadata were
unaffected because metadata carries base64 DER.

Store base64 DER, as metadata does. The saved idpCertificate PEM and
idpCertFile path are read as before; only the descriptor rebuilt in memory
on each load changes, so no saved configuration needs migrating.

TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures signs an ACS
Response and a LogoutResponse with one test IdP key and checks that
inline-certificate, certificate-file and metadata providers all accept
them, and that a Response signed by another key is refused. The test
allows IdP-initiated responses because ProcessResponse records no
AuthnRequest IDs, so at the default every ACS Response is refused on
InResponseTo. That is a separate open bug, not fixed here.
This commit is contained in:
rcourtman
2026-10-07 20:48:08 +01:00
parent 2eec0549a3
commit 3cef2c1921
7 changed files with 233 additions and 1 deletions
@@ -8947,6 +8947,14 @@ cookie for any response (the logout request already cleared both), and
`/api/saml/{id}/logout` refuses a request that carries no session. No agent
registration, enrolment, install, update or removal path changed.
### Manual SAML IdP certificate published as base64 DER
`internal/api/saml_service.go` changed only so a SAML provider configured with
`idpSsoUrl` and a PEM `idpCertificate` or `idpCertFile` puts that certificate
into its in-memory IdP descriptor as base64 DER, which crewjam/saml can read
when it verifies the IdP's ACS Responses and LogoutResponses. No agent
registration, enrolment, install, update or removal path changed.
### Demo write guard ignores websocket upgrade headers
`internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or
@@ -6495,6 +6495,27 @@ That same SSO boundary also owns manual SAML endpoint validation payloads.
through the same validated absolute HTTP(S) helpers instead of letting the
manual logout URL drift out of the request model or bypass the governed URL
normalization path.
That same manual SAML configuration must verify signatures with the IdP
certificate the administrator supplied. With no IdP metadata,
`SAMLService.buildManualMetadata` in `internal/api/saml_service.go` builds the
IdP descriptor from `idpSsoUrl`, optional `idpSloUrl` and the PEM signing
certificate in `idpCertificate` or `idpCertFile`, and must put that certificate
in the signing `KeyDescriptor` as base64 DER, the `<X509Certificate>` content
IdP metadata carries. crewjam/saml base64-decodes that field to verify every
ACS Response and LogoutResponse, so the PEM block it once held failed each
check with "illegal base64 data" before any key was compared, and no manually
configured provider could accept a login or complete SLO. The stored
`idpCertificate` stays the PEM the administrator supplied; only the descriptor
rebuilt in memory on each load changed, so no saved configuration needs
migrating. `TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures` in
`internal/api/contract_test.go` signs an ACS Response and a LogoutResponse with
one test IdP key and proves an inline-certificate provider, a certificate-file
provider and a metadata provider accept both alike, while a Response signed by
another key is refused. The proof enables IdP-initiated responses because
`SAMLService.ProcessResponse` records no AuthnRequest IDs: with
`allowIdpInitiated` off, crewjam refuses every ACS Response on `InResponseTo`
whatever its signature. That is a known open exception, not part of this
contract.
That same SSO provider-detail boundary must return the non-secret nested
provider configuration used by the settings edit form. `GET
/api/security/sso/providers/{id}` may keep the flat list/card fields for
@@ -6886,6 +6886,15 @@ refuses a LogoutResponse that does not answer one of them. The records are not
persisted, and the SLO handler no longer writes session state. No storage,
retention, backup, migration or recovery path is added or moved.
### Manual SAML IdP certificate published as base64 DER
`internal/api/saml_service.go` now encodes a manually configured SAML IdP
signing certificate as base64 DER in the IdP descriptor it rebuilds in memory
on each load. The saved settings are read as before: `idpCertificate` holds
the PEM the administrator pasted and `idpCertFile` the path of a PEM file, so
nothing persisted changes or needs migrating. No storage, retention, backup,
migration or recovery path is added or moved.
### Demo write guard ignores websocket upgrade headers
`internal/api/demo_middleware.go` no longer exempts every request carrying
+75
View File
@@ -4450,6 +4450,81 @@ func TestContract_SAMLLoginRejectsUnsupportedMethods(t *testing.T) {
}
}
// A SAML provider configured by hand, with the IdP's SSO URL and its PEM
// signing certificate pasted inline or named by IDPCertFile, verifies the
// IdP's signatures with that certificate exactly as a provider configured from
// IdP metadata does: the ACS accepts the IdP's signed Response and reads the
// user from it, the IdP's signed answer to an SP-initiated logout completes
// that logout, and a Response signed by any other key is refused. Manual
// metadata used to hold the certificate as a PEM block where crewjam/saml
// base64-decodes DER, so every signature check for these providers failed
// before reaching the key.
func TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures(t *testing.T) {
idp := newSAMLManualCertTestIdP(t)
impostor := newSAMLManualCertTestIdP(t)
certFile := filepath.Join(t.TempDir(), "idp-signing.pem")
if err := os.WriteFile(certFile, idp.certPEM, 0o600); err != nil {
t.Fatalf("write IdP certificate file: %v", err)
}
manual := func(inline, file string) *config.SAMLProviderConfig {
return &config.SAMLProviderConfig{
IDPSSOURL: "https://idp.example.com/sso",
IDPSLOURL: "https://idp.example.com/slo",
IDPEntityID: samlManualCertTestIdPEntityID,
IDPCertificate: inline,
IDPCertFile: file,
}
}
for _, tc := range []struct {
name string
cfg *config.SAMLProviderConfig
}{
{name: "IdP metadata", cfg: &config.SAMLProviderConfig{IDPMetadataXML: idp.metadataXML()}},
{name: "manual inline certificate", cfg: manual(string(idp.certPEM), "")},
{name: "manual certificate file", cfg: manual("", certFile)},
} {
t.Run(tc.name, func(t *testing.T) {
// ProcessResponse records no AuthnRequest IDs, so by default
// crewjam refuses every Response on InResponseTo. Allowing
// IdP-initiated responses skips that check and leaves the
// signature, issuer, destination and timing checks to decide.
tc.cfg.AllowIDPInitiated = true
service, err := NewSAMLService(context.Background(), "okta", tc.cfg, "https://pulse.example.com")
if err != nil {
t.Fatalf("NewSAMLService: %v", err)
}
result, _, err := service.ProcessResponse(idp.acsPost(t, service))
if err != nil {
t.Fatalf("ACS refused the configured IdP's signed Response: %s", samlRejectionDetail(err))
}
if result.Username != "alice" {
t.Fatalf("ACS read username %q from the Response, want alice", result.Username)
}
if _, _, err := service.ProcessResponse(impostor.acsPost(t, service)); err == nil {
t.Fatal("ACS accepted a Response signed by a key the provider was not given")
}
dataPath := t.TempDir()
resetSessionStoreForTests()
t.Cleanup(resetSessionStoreForTests)
resetCSRFStoreForTests()
t.Cleanup(resetCSRFStoreForTests)
InitSessionStore(dataPath)
InitCSRFStore(dataPath)
router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")}
router.samlManager.services["okta"] = service
requestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "alice"))
rec := deliverSAMLTestLogoutResponse(router, idp.slo.redirectQuery(t, requestID), "")
if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/?logout=success" {
t.Fatalf("SLO refused the configured IdP's signed LogoutResponse: status=%d location=%q body=%q",
rec.Code, rec.Header().Get("Location"), rec.Body.String())
}
})
}
}
func TestContract_FindingJSONSnapshot(t *testing.T) {
now := time.Date(2026, 2, 8, 13, 14, 15, 0, time.UTC)
lastSeen := now.Add(5 * time.Minute)
+106
View File
@@ -8,6 +8,8 @@ import (
"encoding/base64"
"encoding/pem"
"encoding/xml"
"errors"
"fmt"
"io"
"net/http"
"net/http/httptest"
@@ -726,3 +728,107 @@ func TestExtractSAMLProviderID(t *testing.T) {
t.Fatalf("expected empty provider for non-saml path, got %q", got)
}
}
const samlManualCertTestIdPEntityID = "https://idp.example.com/metadata"
// samlManualCertTestIdP holds an IdP signing key and its PEM certificate, the
// form an administrator pastes or points IDPCertFile at for a provider
// configured without IdP metadata. It signs ACS Responses and LogoutResponses
// the way the real IdP would.
type samlManualCertTestIdP struct {
certPEM []byte
idp *saml.IdentityProvider
slo *samlSLOTestIdP
}
func newSAMLManualCertTestIdP(t *testing.T) *samlManualCertTestIdP {
t.Helper()
certPEM, _, key := generateTestCert(t)
block, _ := pem.Decode(certPEM)
if block == nil {
t.Fatal("decode IdP certificate PEM")
}
cert, err := x509.ParseCertificate(block.Bytes)
if err != nil {
t.Fatalf("parse IdP certificate: %v", err)
}
entityID, err := url.Parse(samlManualCertTestIdPEntityID)
if err != nil {
t.Fatalf("parse IdP entity ID: %v", err)
}
const signatureMethod = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"
return &samlManualCertTestIdP{
certPEM: certPEM,
idp: &saml.IdentityProvider{
Key: key,
Certificate: cert,
MetadataURL: *entityID,
SignatureMethod: signatureMethod,
},
slo: &samlSLOTestIdP{signer: &saml.ServiceProvider{
EntityID: samlManualCertTestIdPEntityID,
Key: key,
Certificate: cert,
SignatureMethod: signatureMethod,
}},
}
}
// metadataXML publishes the same signing certificate as IdP metadata does,
// base64 DER inside <X509Certificate>.
func (idp *samlManualCertTestIdP) metadataXML() string {
return `<?xml version="1.0"?>
<EntityDescriptor xmlns="urn:oasis:names:tc:SAML:2.0:metadata" entityID="` + samlManualCertTestIdPEntityID + `">
<IDPSSODescriptor protocolSupportEnumeration="urn:oasis:names:tc:SAML:2.0:protocol">
<KeyDescriptor use="signing">
<KeyInfo xmlns="http://www.w3.org/2000/09/xmldsig#">
<X509Data><X509Certificate>` + base64.StdEncoding.EncodeToString(idp.idp.Certificate.Raw) + `</X509Certificate></X509Data>
</KeyInfo>
</KeyDescriptor>
<SingleSignOnService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://idp.example.com/sso"/>
<SingleLogoutService Binding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect" Location="https://idp.example.com/slo"/>
</IDPSSODescriptor>
</EntityDescriptor>`
}
// acsPost returns the HTTP-POST binding request to the service's ACS carrying
// a signed, unsolicited (no InResponseTo) Response that identifies alice.
func (idp *samlManualCertTestIdP) acsPost(t *testing.T, service *SAMLService) *http.Request {
t.Helper()
spMetadata := service.sp.Metadata()
authn := &saml.IdpAuthnRequest{
IDP: idp.idp,
HTTPRequest: httptest.NewRequest(http.MethodPost, "https://idp.example.com/sso", nil),
ServiceProviderMetadata: spMetadata,
SPSSODescriptor: &spMetadata.SPSSODescriptors[0],
ACSEndpoint: &saml.IndexedEndpoint{Binding: saml.HTTPPostBinding, Location: service.sp.AcsURL.String()},
Now: time.Now(),
}
if err := (saml.DefaultAssertionMaker{}).MakeAssertion(authn, &saml.Session{
ID: "idp-session-alice",
CreateTime: time.Now(),
ExpireTime: time.Now().Add(time.Hour),
Index: "idx-alice",
NameID: "alice",
}); err != nil {
t.Fatalf("make assertion: %v", err)
}
form, err := authn.PostBinding()
if err != nil {
t.Fatalf("sign ACS Response: %v", err)
}
req := httptest.NewRequest(http.MethodPost, service.sp.AcsURL.String(),
strings.NewReader(url.Values{"SAMLResponse": {form.SAMLResponse}}.Encode()))
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
return req
}
// samlRejectionDetail adds crewjam/saml's private reason to a rejected
// response, which its public error ("Authentication failed") hides.
func samlRejectionDetail(err error) string {
var invalid *saml.InvalidResponseError
if errors.As(err, &invalid) && invalid.PrivateErr != nil {
return err.Error() + ": " + invalid.PrivateErr.Error()
}
return fmt.Sprint(err)
}
+4 -1
View File
@@ -359,8 +359,11 @@ func (s *SAMLService) addIDPCertificate(metadata *saml.EntityDescriptor) error {
Use: "signing",
KeyInfo: saml.KeyInfo{
X509Data: saml.X509Data{
// Base64 DER, as <X509Certificate> carries it in IdP
// metadata: crewjam/saml base64-decodes this field to
// verify signatures and cannot read a PEM block.
X509Certificates: []saml.X509Certificate{
{Data: string(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}))},
{Data: base64.StdEncoding.EncodeToString(cert.Raw)},
},
},
},
+10
View File
@@ -7,6 +7,7 @@ import (
"crypto/rand"
"crypto/rsa"
"crypto/x509"
"encoding/base64"
"encoding/pem"
"errors"
"fmt"
@@ -90,6 +91,7 @@ func TestBuildManualMetadataAndCertificate(t *testing.T) {
cfg.IDPIssuer = "issuer"
certPEM, _, _ := generateTestCert(t)
cfg.IDPCertificate = string(certPEM)
certBlock, _ := pem.Decode(certPEM)
metadata, err := service.buildManualMetadata()
if err != nil {
@@ -104,6 +106,14 @@ func TestBuildManualMetadataAndCertificate(t *testing.T) {
if len(metadata.IDPSSODescriptors[0].KeyDescriptors) == 0 {
t.Fatal("expected key descriptor with certificate")
}
// crewjam/saml reads <X509Certificate> content: base64 DER, never PEM.
certs := metadata.IDPSSODescriptors[0].KeyDescriptors[0].KeyInfo.X509Data.X509Certificates
if len(certs) != 1 {
t.Fatalf("expected one signing certificate, got %d", len(certs))
}
if certs[0].Data != base64.StdEncoding.EncodeToString(certBlock.Bytes) {
t.Fatalf("expected the signing certificate as base64 DER, got %.40q", certs[0].Data)
}
cfg.IDPSSOURL = "https://user:pass@idp.example.com/sso"
if _, err := service.buildManualMetadata(); err == nil {