Skip to content

Conversation

@RichardKnop
Copy link

@RichardKnop RichardKnop commented Jul 7, 2017

Thank you for this useful library.

This PR fixes two issues I have found out while using this library to integrate a web application with Active Directory single sign on.

  1. Related to this issue: samlsp: RequireAccount CSP missing script-src for script #92 and also this issue: Content Security Policy Violation #93

The JS script hash for content security settings only include one of inline scripts. I have combined two inline scripts into one and corrected the sha256 hash.

This is the new hash:

echo -n "document.getElementById('SAMLSubmitButton').style.visibility=\"hidden\";document.getElementById('SAMLRequestForm').submit();" | openssl dgst -sha256 -binary | openssl enc -base64
  1. There was a bug in samlsp.New where the returned middleware's m.ServiceProvider.IDPMetadata would be empty. This was because the loop would assign pointer to variable from for loop to the m.ServiceProvider.IDPMetadata field during iteration where len(e.IDPSSODescriptors) > 0, however in subsequent iterations the pointer would be moved to empty variable without descriptiors.

Please review :)

- Fixed script hash to remove JS console errors when redirecting
- Fixed samlsp.New method to not overwrite m.ServiceProvider.IDPMetadata
@RichardKnop RichardKnop force-pushed the feature/service-provider-fixes branch from 6b4f886 to f367785 Compare July 7, 2017 15:13
@RichardKnop
Copy link
Author

@crewjam What do you think?

@umayr
Copy link
Contributor

umayr commented Jul 10, 2017

@RichardKnop I applied your patch as I ran into the same issue as #93 and created a server using the example code in README. The patch does fix the issue and prevent the JS endless loop however, I get forbidden on endpoint /saml/acs. Logs are following:

ERROR: Status code was not urn:oasis:names:tc:SAML:2.0:status:Success
2017/07/10 18:28:45 getPossibleRequestIDs: cookie: saml_L8bd8wIFz--YxYDbixeX6ktBW4Gnn9DASrlt0vOqyzdhXXo7XTG_g2kj=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6ImlkLTk5YjYxOTg4YjA1NTkwZmVmYjY3ZjhlYzVjYmIxNzZmMjZhZDNmYmIiLCJ1cmkiOiIvaGVsbG8ifQ.8hDljB-caJpYzxXFJ4M2Ugnqukv3NbQiP2Sy3_lRrg4
2017/07/10 18:28:45 RESPONSE: ===
<?xml version="1.0" encoding="UTF-8"?><saml2p:Response xmlns:saml2p="urn:oasis:names:tc:SAML:2.0:protocol" Destination="http://localhost:8000/saml/acs" ID="_3aafbe809b38fab40a2808b71292e7bf" InResponseTo="id-99b61988b05590fefb67f8ec5cbb176f26ad3fbb" IssueInstant="2017-07-10T13:28:45.081Z" Version="2.0"><saml2:Issuer xmlns:saml2="urn:oasis:names:tc:SAML:2.0:assertion" Format="urn:oasis:names:tc:SAML:2.0:nameid-format:entity">https://idp.testshib.org/idp/shibboleth</saml2:Issuer><saml2p:Status><saml2p:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Responder"/><saml2p:StatusMessage>Unable to encrypt assertion</saml2p:StatusMessage></saml2p:Status></saml2p:Response>

Is it something related to your patch or I'm missing something here?

@RichardKnop
Copy link
Author

@umayr I had the same problem using www.testshib.org

I have switched instead and am testing single sign on integration with www.onelogin.com

Create a developer account at www.onelogin.com and there you can create SAML Test Connector (IdP) (it's one of the app templates in their library).

Then just configure that IdP to use your service provider and it will work (works for me).

I couldn't get it working with Testshib.

@RichardKnop
Copy link
Author

@umayr You can also debug testshib here: https://idp.testshib.org/cgi-bin/idplog.cgi?lines=3000&logname=idp-process.log

But that wasn't very helpful for me as I couldn't figure out how to fix the encryption error. So I gave up on testshib.

@RichardKnop RichardKnop force-pushed the feature/service-provider-fixes branch from 35c4026 to f367785 Compare July 20, 2017 19:15
@fliar
Copy link

fliar commented Sep 21, 2017

@RichardKnop i tried this lib yesterday and suffered the same issue, wished i could get here earlier
tried go-saml today, and i got "Error Message: Error decoding authentication request message" on testshib, so maybe there's something wrong with testshib ?
maybe i should give onelogin a try, and your pr, thanks for your info
sorry if i'm off the topic

@crewjam
Copy link
Owner

crewjam commented Sep 25, 2017

First off, sorry it took me so long to get to this & thanks for the PR.

I refactored your change to remove the samlsp.go fix, which landed in #90 already.

The rest merged in #117.

Thanks!

@crewjam crewjam closed this Sep 25, 2017
@RichardKnop
Copy link
Author

👍

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.

4 participants