From dfd27693ee0e14799b4b6fdef2e28e39bd11d33a Mon Sep 17 00:00:00 2001 From: Ayke van Laethem Date: Tue, 14 Aug 2018 03:16:52 +0200 Subject: [PATCH 1/2] Be more flexible in the accepted XML --- sign.go | 5 +++-- sign_test.go | 11 ----------- validate.go | 18 ++++++++++-------- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/sign.go b/sign.go index e2c2852..2be34b7 100644 --- a/sign.go +++ b/sign.go @@ -92,10 +92,11 @@ func (ctx *SigningContext) constructSignedInfo(el *etree.Element, enveloped bool dataId := el.SelectAttrValue(ctx.IdAttribute, "") if dataId == "" { - return nil, errors.New("Missing data ID") + reference.CreateAttr(URIAttr, "") + } else { + reference.CreateAttr(URIAttr, "#"+dataId) } - reference.CreateAttr(URIAttr, "#"+dataId) // /SignedInfo/Reference/Transforms transforms := ctx.createNamespacedElement(reference, TransformsTag) diff --git a/sign_test.go b/sign_test.go index 56baa2a..febfec6 100644 --- a/sign_test.go +++ b/sign_test.go @@ -96,17 +96,6 @@ func TestSignErrors(t *testing.T) { _, err := ctx.SignEnveloped(authnRequest) require.Error(t, err) - - randomKeyStore = RandomKeyStoreForTest() - ctx = NewDefaultSigningContext(randomKeyStore) - - authnRequest = &etree.Element{ - Space: "samlp", - Tag: "AuthnRequest", - } - - _, err = ctx.SignEnveloped(authnRequest) - require.Error(t, err) } func TestSignNonDefaultID(t *testing.T) { diff --git a/validate.go b/validate.go index 55feb39..c977512 100644 --- a/validate.go +++ b/validate.go @@ -233,16 +233,17 @@ func (ctx *ValidationContext) verifySignedInfo(sig *types.Signature, canonicaliz } func (ctx *ValidationContext) validateSignature(el *etree.Element, sig *types.Signature, cert *x509.Certificate) (*etree.Element, error) { - idAttr := el.SelectAttr(ctx.IdAttribute) - if idAttr == nil || idAttr.Value == "" { - return nil, errors.New("Missing ID attribute") + idAttrEl := el.SelectAttr(ctx.IdAttribute) + idAttr := "" + if idAttrEl != nil { + idAttr = idAttrEl.Value } var ref *types.Reference // Find the first reference which references the top-level element for _, _ref := range sig.SignedInfo.References { - if _ref.URI == "" || _ref.URI[1:] == idAttr.Value { + if _ref.URI == "" || _ref.URI[1:] == idAttr { ref = &_ref } } @@ -298,9 +299,10 @@ func contains(roots []*x509.Certificate, cert *x509.Certificate) bool { // findSignature searches for a Signature element referencing the passed root element. func (ctx *ValidationContext) findSignature(el *etree.Element) (*types.Signature, error) { - idAttr := el.SelectAttr(ctx.IdAttribute) - if idAttr == nil || idAttr.Value == "" { - return nil, errors.New("Missing ID attribute") + idAttrEl := el.SelectAttr(ctx.IdAttribute) + idAttr := "" + if idAttrEl != nil { + idAttr = idAttrEl.Value } var sig *types.Signature @@ -380,7 +382,7 @@ func (ctx *ValidationContext) findSignature(el *etree.Element) (*types.Signature // Traverse references in the signature to determine whether it has at least // one reference to the top level element. If so, conclude the search. for _, ref := range _sig.SignedInfo.References { - if ref.URI == "" || ref.URI[1:] == idAttr.Value { + if ref.URI == "" || ref.URI[1:] == idAttr { sig = _sig return etreeutils.ErrTraversalHalted } From 31adff1268582b18f2856d24f0e3d4ed4a87e998 Mon Sep 17 00:00:00 2001 From: Patrick Boyd Date: Wed, 1 Jul 2020 09:31:22 -0500 Subject: [PATCH 2/2] Actually verify certs --- validate.go | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/validate.go b/validate.go index c977512..58c16af 100644 --- a/validate.go +++ b/validate.go @@ -402,7 +402,7 @@ func (ctx *ValidationContext) findSignature(el *etree.Element) (*types.Signature return sig, nil } -func (ctx *ValidationContext) verifyCertificate(sig *types.Signature) (*x509.Certificate, error) { +func (ctx *ValidationContext) verifyCertificate(sig *types.Signature, verify bool) (*x509.Certificate, error) { now := ctx.Clock.Now() roots, err := ctx.CertificateStore.Certificates() @@ -438,8 +438,24 @@ func (ctx *ValidationContext) verifyCertificate(sig *types.Signature) (*x509.Cer } // Verify that the certificate is one we trust - if !contains(roots, cert) { - return nil, errors.New("Could not verify certificate against trusted certs") + if verify { + pool := x509.NewCertPool() + for _, c := range roots { + pool.AddCert(c) + } + opts := x509.VerifyOptions{ + Roots: pool, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, + } + + _, err := cert.Verify(opts) + if err != nil { + return nil, err + } + } else { + if !contains(roots, cert) { + return nil, errors.New("Could not verify certificate against trusted certs") + } } if now.Before(cert.NotBefore) || now.After(cert.NotAfter) { @@ -460,7 +476,25 @@ func (ctx *ValidationContext) Validate(el *etree.Element) (*etree.Element, error return nil, err } - cert, err := ctx.verifyCertificate(sig) + cert, err := ctx.verifyCertificate(sig, false) + if err != nil { + return nil, err + } + + return ctx.validateSignature(el, sig, cert) +} + +// ValidateWithRootTrust does the same as Verify except it actually verifies the root CA is trusted as well +func (ctx *ValidationContext) ValidateWithRootTrust(el *etree.Element) (*etree.Element, error) { + // Make a copy of the element to avoid mutating the one we were passed. + el = el.Copy() + + sig, err := ctx.findSignature(el) + if err != nil { + return nil, err + } + + cert, err := ctx.verifyCertificate(sig, true) if err != nil { return nil, err }