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

Add hooks to retrieve last-sent and last-received requests and responses #159

Merged

Conversation

haikuginger
Copy link
Contributor

To get more data for troubleshooting in production, we've added hooks to allow us to pull the relevant XML documents out of python-saml for logging on our side of things. This way, we can retest those documents on a non-production system to determine what might have gone wrong.

@pitbulk
Copy link
Contributor

pitbulk commented Aug 24, 2016

But why you don't collect the requests/responses that gonna be processed in the views that uses the python-saml toolkit?

@haikuginger
Copy link
Contributor Author

In the case of responses, they can contain encrypted assertions that cannot be effectively extracted without duplicating a large amount of python-saml's functionality. In both cases, requests and responses may be b64encoded, which is another layer of obfuscation.

While these concerns may be overcome with enough time spent on them, python-saml already takes these items into consideration as part of its standard operation; the code to get what we want already largely exists.

Oh, and I should note that we've been seeing Travis segfaults while working on this, but tests do pass locally.

@pitbulk
Copy link
Contributor

pitbulk commented Aug 24, 2016

it does not pass pep8/pyflakes. I will review this feature and see if it makes sense and does not compromise security.

@haikuginger
Copy link
Contributor Author

Thanks for pointing that out @pitbulk; I've fixed those issues.

@haikuginger haikuginger force-pushed the haikuginger/last-xml-document-hooks branch from ea8ee66 to 87d4c18 Compare August 25, 2016 03:09
@haikuginger
Copy link
Contributor Author

Rebased from ea8ee6.

@haikuginger
Copy link
Contributor Author

@pitbulk, as with the other PR I have open here, we've been using this code for a while in production, and are hoping to get it reviewed and merged here so that we can return to using the canonical source.

@pitbulk
Copy link
Contributor

pitbulk commented Oct 14, 2016

I'm sorry for the delay.

I think this feature makes sense, but I was busy with a security improvement that I'm publishing today with a new release. I was so busy with that, since I mad this improvements in all the toolkits.

I think next eeek I will be able to review and add this functionality and maybe do a minor release with it.

@pitbulk
Copy link
Contributor

pitbulk commented Nov 8, 2016

This PR covers:

  • Last SAMLResponse processed by process_response

  • Last SAMLRequest sent by login

    But what happens with:

  • Last SAMLRequest sent by logout

  • Last SAMLRequest processed by process_slo

  • Last SAMLResponse processed by process_slo

  • Last SAMLRequest sent by process_slo

  • Last SAMLResponse sent by process_slo

@haikuginger
Copy link
Contributor Author

@pitbulk, our what our organization needs is just the login request and the response received. I can look into whether we can expand our scope on this to cover those items in a subsequent PR, but for right now, we'd like to get this piece merged.

@bradenmacdonald
Copy link
Contributor

@pitbulk Hi :) @haikuginger is working with me on this. We want to have something you can merge, so please let us know what you'd need for that to happen. To answer your question, currently the last SAML request from those other processes is not saved. If you it's important to have that consistency in place before this can be merged, let us know.

@pitbulk
Copy link
Contributor

pitbulk commented Nov 25, 2016

It's ok, I will improve your PR and merge it soon.

@pitbulk
Copy link
Contributor

pitbulk commented Nov 29, 2016

@bradenmacdonald , @haikuginger

I improved the PR here: #171

It lacks some tests, but I will implement them today and if you agree with the solution, merge it to master.

@pitbulk
Copy link
Contributor

pitbulk commented Nov 30, 2016

#171 is ready, @bradenmacdonald , @haikuginger can you review?

@pitbulk pitbulk merged commit 87d4c18 into SAML-Toolkits:master Dec 1, 2016
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.

None yet

3 participants