search

italia / spid-keycloak-provider

Italian SPID authentication provider for Keycloak (https://www.keycloak.org/)
Apache License 2.0
62 stars 22 forks source link

SPID checks 16,17,18 (InResponseTo checks) #22

Closed nicolabeghin closed 3 years ago

nicolabeghin commented 3 years ago

ref. #18

As part of the 100+ checks performed by spid-saml-check in order to pass SPID validation, Assertion.Subject.SubjectConfirmationData.InResponseTo image

must match the SAML request ID image

If not available or mismatching, 3 different mandatory tests fails. image

This PR performs the 3 requested additional checks on InResponseTo image

image

lscorcia commented 3 years ago

Impressive work! I will review it and merge as soon as possible. Did you already think about submitting the patches to the main Keycloak project too?

nicolabeghin commented 3 years ago

Unfortunately I just found that there's a massive bug, Keycloak's session-stored Request ID for some reason is not refreshed for each request or retrieved wrong altogether.

In other terms Resolved RequestID from session is fixed for the whole session for I don't know what reason.

image

Any help is appreciated!

nicolabeghin commented 3 years ago

@lscorcia submitted fix - PR can be reviewed and merged if you find it useful.

lscorcia commented 3 years ago

Regarding the InResponseTo issue, it's actually worth reporting this to the upstream Keycloak project. I have opened https://issues.redhat.com/browse/KEYCLOAK-17935 and I prepared a test case to reproduce it in plain SAML.

Edit: As it's a security issue that you discovered, would you mind being mentioned if they acknowledge it as a CVE?

nicolabeghin commented 3 years ago

No problem! In the meantime if you feel it right I think it would be great to have it merged in the spid provider so that we can slowly move toward a full spid compliant implementation (there's quite some road ahead..)

lscorcia commented 3 years ago

Pull request merged with minor changes. Regarding the Keycloak upstream issue, the task has been assigned to a Red Hat developer. When they'll come up with a patch I will update this project with the official fix, meanwhile I used your approach of setting a custom authSession client note.

Thanks for the contribution!