Skip to content

fix: improve SAML signature validation for redirect binding #621

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

gsaandy
Copy link

@gsaandy gsaandy commented May 17, 2025

This resolves the merge conflicts in #449

In addition to the changes mentioned in the above PR, it also fixes the following

  • Removes the signature validation using SAMLResponse payload for HTTP-Redirect binding
  • Fix the Signature validation failures for ADFS because of decode/encode while reconstructing the sign data
  • fix: add missing Signature and SigAlg query params with single logout HTTTP-Redirect binding request

Tested single logout with Okta and Microsoft Entra ID(Azure AD)

Jguer and others added 3 commits January 2, 2023 13:34
add support for sha1 & sha512

add tests

use query sign in redirect

implement review feedback

- Return error if signature is unsupported
- wrap errors

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
Co-authored-by: Orgad Shaneh <orgads@gmail.com>
- Removes the signature validation using SAMLResponse payload for HTTP-Redirect binding
- Fix the Signature validation failures for ADFS because of decode/encode while reconstructing the sign data
@gsaandy gsaandy requested a review from crewjam as a code owner May 17, 2025 22:27
@gsaandy
Copy link
Author

gsaandy commented May 17, 2025

Review please @crewjam @andreas-kupries @Jguer @omerkarj

@gsaandy
Copy link
Author

gsaandy commented May 20, 2025

@crewjam - just wondering if you’ve had a chance to take a look at this PR. I'm happy to help with any changes needed to get it fixed and merged

@gsaandy
Copy link
Author

gsaandy commented May 24, 2025

@crewjam - just wondering if you’ve had a chance to take a look at this PR. I'm happy to help with any changes needed to get it fixed and merged

Hello @crewjam - Any chance this can be reviewed.

@gsaandy
Copy link
Author

gsaandy commented Jun 7, 2025

@crewjam - could you help to review this PR.

@Lumengrid
Copy link

any ETA on this ?

@gsaandy
Copy link
Author

gsaandy commented Jun 21, 2025

@crewjam - could you help to review this?

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.

3 participants