-
Notifications
You must be signed in to change notification settings - Fork 125
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
OAM signature verification #16
Comments
Thanks for reporting this! It might be a few days before I'm able to take a look but I'll try to get to it ASAP. |
(1) seems to be solved now. #20 is designed to solve (2). (3) remains unresolved. Any updates? Thanks. |
Sorry for the delay. I've finally gotten some time to work on this project, I'm going to try and have a fix over the next few days. |
I've created a provider test in https://github.com/russellhaering/gosaml2, and between the two libraries made a lot of progress towards getting it passing - mostly I've totally reworked how namespaces are handled. All three issues you've identified here are resolved (although item 1 is resolved in the SAML library, by falling back to Assertion validation), but signature verification is still failing with the cert you provided. Can you test whether this is working as expected for you now? |
It's failing at verifySignedInfo. Looking at the canonicalization, we're having problems there as well. Current: Expected: The test code I'm using:
The attribute copying looks pretty messy to me. I think it should be handled in this library instead of in gosaml2. Thoughts? |
This should be working on master now, and the test case I created in gosaml2 with the values you provided is passing. As far as at what level this should be handled: the reason for the current structure is that I want to keep the signature verification interface as safe as possible by default. Currently you pass it an element, and that element must have an enveloped signature from a trusted certificate. It will always return either an error, or a validated document rooted at the same element. I'm open to adding new validation calls with different behavior, but it's not obvious to me how best to structure a call that might return a partial document, or an only partially validated document. You're definitely right about the attribute copying being a mess though. I hit that issue repeatedly in the course of fixing this, so I've added a utility method for doing it correctly: https://godoc.org/github.com/russellhaering/goxmldsig/etreeutils#NSSelectOne Using that, you can simplify your test case to: func TestExternalXmlNs(t *testing.T) {
pemString, err := ioutil.ReadFile("rootxmlns.crt")
require.Nil(t, err)
pemBlock, _ := pem.Decode([]byte(pemString))
cert, err := x509.ParseCertificate(pemBlock.Bytes)
require.Nil(t, err)
doc := etree.NewDocument()
err = doc.ReadFromFile("rootxmlns.xml")
require.Nil(t, err)
unverifiedAssertion, err := etreeutils.NSSelectOne(doc.Root(), "urn:oasis:names:tc:SAML:2.0:assertion", "Assertion")
if err != nil {
return nil, err
}
ctx := NewDefaultValidationContext(&MemoryX509CertificateStore{Roots: []*x509.Certificate{cert}})
_, err = ctx.Validate(unverifiedAssertion)
require.Nil(t, err)
} |
Tests are passing on my end as well. Closing. |
A few issues with OAM (Oracle Access Manager) generated assertions:
Signature
does not always reference the top-level elementxmlns
defined at the top level causes canonicalization problemsSigned payload and certificate:
https://gist.github.com/Calpicow/fd1d03ded8dc9da1570155bc14aa689a
https://gist.github.com/Calpicow/ef34701804053d042739137b2efd708a
Expected canonicalization: https://gist.github.com/Calpicow/22062f1cf9283621226c5dee2cb9bb0d
Current canonicalization: https://gist.github.com/Calpicow/405b039350d4efff2537ce2f204b6964
The text was updated successfully, but these errors were encountered: