Skip to content

DRAFT: v7.x #493

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 11 commits into
base: 6.x
Choose a base branch
from
Open

DRAFT: v7.x #493

wants to merge 11 commits into from

Conversation

ahacker1-securesaml
Copy link

@ahacker1-securesaml ahacker1-securesaml commented Apr 8, 2025

This significantly improves the security logic around xml-crypto.

Currently, it is an outline, i.e. the code doesn't actually work

Reviewers:

  • Let me know about the security of this, does it make it easier to read, so more secure?
  • is there a better way to design this?
    Currently we remove the checkSignature method from the SignedXML class to simplify the logic, and put it in XMLVerifier

Release process debate:

  • Do we even want to release 7.x
    If feel like the best we can do is to deprecate the library entirely,
    because xml signatures as a whole is unsafe, and using it will ensure the other side will be insecure. (since other implementations are insecure)

@ahacker1-securesaml ahacker1-securesaml marked this pull request as draft April 8, 2025 11:19
@ahacker1-securesaml ahacker1-securesaml marked this pull request as ready for review April 8, 2025 20:34
@ahacker1-securesaml ahacker1-securesaml changed the base branch from master to 6.x April 8, 2025 20:36
@cjbarth
Copy link
Contributor

cjbarth commented Apr 9, 2025

Given the number of people using xml-crypto and the lack of an alternative (that I'm aware of). It seems prudent to continue developing this. Otherwise, we should provide a migration guide to some other method of doing what this library does.

# Conflicts:
#	README.md
#	src/signed-xml.ts
@ahacker1-securesaml
Copy link
Author

ahacker1-securesaml commented Apr 11, 2025

I definitely don't want to encourage users to use xml signatures if they are building some new feature/product/spec.
Let's link something like: https://paseto.io/ , which is actually designed by security experts for them to use. Remember any PASETO's works for any data.

For SAML implementations, we can still support xml-crypto.

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