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

Soft validate with missing cert shouldn't raise error #89

Closed

Conversation

caius
Copy link

@caius caius commented Jun 3, 2013

We should be able to soft validate without an exception being raised, which includes when the certificate is missing. This patch brings the behaviour of that validation inline with the fingerprint validation in the same method. (Returns false for soft validation, raises exception for non-soft validation.)

@Lordnibbler
Copy link
Contributor

@caius can you please rebase off master and see if your specs still pass?

@caius
Copy link
Author

caius commented Feb 27, 2014

@Lordnibbler sure thing, will give it a go

We should be able to soft validate without an exception being raised, this includes when the certificate is missing.
@caius
Copy link
Author

caius commented Feb 27, 2014

@Lordnibbler rebased on master successfully. Test fails against master, passes with the patch to xml_security.rb in this PR. Please can has review & merge? 😀

@phene
Copy link
Contributor

phene commented Mar 17, 2014

+1 for fixing this behavior.

@eccegordo
Copy link

sorta related to this can anyone explain how to validate a saml response where there is NOT an X.509 cert embedded in the SAML XML?

I have access to the cert and the signature but not sure what part of OpenSSL to use for the validation.

In other words this particular line does not work for me in my specific use case.

cert                    = OpenSSL::X509::Certificate.new(cert_text)

@wbajzek
Copy link
Contributor

wbajzek commented Apr 21, 2014

+1 on this issue, and @eccegordo's question.

@caius
Copy link
Author

caius commented Jul 9, 2014

@Lordnibbler any chance of getting this merged in now please? 😄

@phene
Copy link
Contributor

phene commented Jul 14, 2014

I ended up also including this fix in my PR here: #139

@Lordnibbler
Copy link
Contributor

@luisvm @inakidelamadrid @pitbulk does this issue look good? Would we rather fix via #139 ?

@Lordnibbler
Copy link
Contributor

fixed via #139 @phene @caius

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants