simplesamlphp / SAML-tracer

Browser extension for examining SAML messages
https://addons.mozilla.org/nl/firefox/addon/saml-tracer/
BSD 2-Clause "Simplified" License
142 stars 39 forks source link

Support for decrypting encrypted SAML assertions #31

Closed MohammadAKHussain closed 10 months ago

MohammadAKHussain commented 6 years ago

Feature request: Is it possible to add a feature to SAML tracer to add/load a private key, in order to decrypt encrypted SAML assertions?

I've run into some configurations where SAML assertions are encrypted, preventing the ability to view the assertion. The only way I found by searching online was to use the online tool (www.samltool.com/decrypt.php), but even that page itself recommends not using production keys online for obvious reasons.

Here's the structure I'm seeing for such SAML responses, if this helps: ` <samlp:Response ID="_e███████████████████3" Version="2.0" IssueInstant="2017-12-11T03:22:09.999Z" Destination="https://███████████████████/Shibboleth.sso/SAML2/POST" Consent="urn:oasis:names:tc:SAML:2.0:consent:unspecified" InResponseTo="_4███████████████████0" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"

http://███████████████████/adfs/services/trust CN=███████████████████ 1███████████████████7 L██████████████████████████████████████w== D██████████████████████████████████████████████████████████████████████████████████████████████████████████████████g== `

Thank you.

Many thanks for this excellent add-on by the way. SAML tracer alone was enough reason to switch to Firefox for a couple of my colleagues, as it proved to be the easiest to use, and the most reliable for capturing/viewing SAML assertions. Your work is truly appreciated :)

badstreff commented 6 years ago

Is there any progress made on this? This is something that would be extremely useful

khlr commented 6 years ago

Hi @BadStreff! I'd guess there's no one actively working on this at the moment. ¯\_(ツ)_/¯

badstreff commented 6 years ago

Are pull requests welcome? I'm not extremely familiar with Javascript but this is something I would be willing to take a look at - I have a feeling more than just us in the thread would find this useful.

khlr commented 6 years ago

As far as I can tell, I think your contribution would be very welcome! :)

khlr commented 10 months ago

@badstreff: I strongly suspect that this topic has also slipped out of your focus. 😉 Are you still interested in implementing this feature? If not, perhaps someone else is?

badstreff commented 10 months ago

Haha yes it is something I ended up working around. Although I can't remember the specifics as I've slept a couple times since then :)

khlr commented 10 months ago

Hehe, sure, you're right 😁

Ok, so this issue is still quite interesting. Unfortunately, I don't have quite as many opportunities to pop off a suitable test environment. Anders Abel's StubIdp (which I really like to use for testing) unfortunately doesn't support token encryption. @AndersAbel: That's probably not a feature you want to add to the StupIdp, is it?

tvdijen commented 10 months ago

@khlr I can set up a test-environment for you like I did before, but are we really going to allow people to upload private keys into the SAML-tracer add-on? And what about encrypted private keys? Or symmetric keys? I'm a bit concerned about where those keys will end up after use (? stored in browser profile ? stored in Temporary Internet Files?) and t.b.h. it scares the shit out of me :') I would argue that the people working with SAML-tracer in a supportdesk-environment are not in possession of the key material anyway.. @jaimeperez Thoughts?

khlr commented 10 months ago

Though it would only be used locally during your SAML-tracer session, you are absolutely right, it would be "somewhere" locally. And yes, whether it should be there... probably not 😉 SAML-tracer would then also have to take care of cleaning it up safely and reliably...

Another point is what the guys from One Login at SAMLtool .com have in mind: Even if it were "only local" for us, you shouldn't encourage anyone to paste their private keys anywhere...

So after thinking about it for a moment, you've convinced me. Yes, it would be a nice feature, but it's a bit "too hot". 🔥

MohammadAKHussain commented 10 months ago

That's unfortunate to read. I don't know why the certificates/keys need to be stored anywhere. I don't know how add-ons work and how they use memory and what they access from the browser, but I was thinking we can just paste the private certificates in some field in SAML-tracer (no need to "load" them), and they are used on the fly, and discarded after decrypting the assertion, as soon as the SAML-tracer window is closed.

Also, kindly remember, not everyone is going to have access to the private certificates/keys in the first place. This function is going to be only those who are involved in configuring/troubleshooting SAML integrations. Without having the right certificates/keys, this function isn't useful to anyone else.

tvdijen commented 10 months ago

Well, not too long ago some browsers were leaking your passwords through their builtin spell-checker, so I wouldn't want to depend on them too much if I were you ;)

"no need to "load" them" Well, that's exactly what you need to do if you want to "use them on the fly" as you describe.. It's data-in-use after you have transferred them from your system to your workstation (data-in-motion). Private keys should be generated on your IdP/SP server and never leave there.

MohammadAKHussain commented 10 months ago

I understand your point now. I still can't help but feel we're over-complicating things, too. It's not like we're forcing anyone to use the add-on and such feature if it's added.

With today's security practices, such servers are very secure, to the point where in my company no one (not even network admins) can do anything but read. No files can be added or modified. There is no way to troubleshoot SAML issues using any dedicated tools. A browser with an approved add-on becomes your only solution.

AndersAbel commented 10 months ago

Hehe, sure, you're right 😁

Ok, so this issue is still quite interesting. Unfortunately, I don't have quite as many opportunities to pop off a suitable test environment. Anders Abel's StubIdp (which I really like to use for testing) unfortunately doesn't support token encryption. @AndersAbel: That's probably not a feature you want to add to the StupIdp, is it?

Thanks for the kind words about the StubIdp.

There's a new, improved StubIdp on my roadmap. Encrypted assertions is a potential new feature to add.

And adding the support on the Idp side would solve the private-key problem. The current version of the StubIdp has the private key published (it's a testing tool, right?) but thinking of it now, the next version will not expose the private key. However, as it is the Idp it will be able to show the un-encrypted data.

khlr commented 10 months ago

@MohammadAKHussain: Do you often have to deal with encrypted assertions? So that you really need this feature? I mean, this issue will soon be celebrating its sixth birthday 😉 How did you work around that in the meantime?

What I'd suggest instead of including this functionality into SAML-tracer: Create e.g. a simple Shell-Script that decrypts the assertion. You could copy the encrypted assertion from SAML-tracer.

MohammadAKHussain commented 10 months ago

Guys I completely understand that I'm just sitting here wishing someone else would do the difficult, time consuming job that I cannot/do not know how to do myself.

Honestly, I don't encounter this that often anymore. When I do, I really struggle, and I simply cannot help our clients (we are the SP), and instead we push for unencrypted assertions (not ok with some clients). But as I said, I get this now about once a year now, due to us pushing clients to change, not because I have another solution.

As for the script: on those servers, we can't even copy to or copy from them. I also don't even have the technical skills to write such script anyways, even if I was going to write it manually whenever I connect to such servers.

That said, if it's too much work, if it's going to work only for the IDP, not the SP, please feel free to just close this. You're under no obligation to do this for someone else anyways. And I really appreciate you looking into it in the first pace,

khlr commented 10 months ago

@AndersAbel: I'm glad to hear that X will be developed further soon!

@MohammadAKHussain: Thank you for your understanding. I'm sorry that we can't help you any further. Hence I'm going to close this issue.