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

better integration with xml-encryption 1.1.0 release #429

Closed
markstos opened this issue Mar 26, 2020 · 5 comments · Fixed by #584
Closed

better integration with xml-encryption 1.1.0 release #429

markstos opened this issue Mar 26, 2020 · 5 comments · Fixed by #584
Milestone

Comments

@markstos
Copy link
Contributor

markstos commented Mar 26, 2020

I'd like to address multiple issues with our relationship with the xml-encryption package:

  1. xml-encryption 1.0 disabled some insecure algorithms. By extension we did too. But they expose an option to re-enable the insecure algorithms if you need them. Maybe we should to.
  2. xml-encryption 1.1.0 offers an option to warn if there are insecure algorithms. By combining this option with the one above, a project could start to issue warnings for insecure algorithms without failing, easing the upgrade. We could expose both options.
  3. We have a place where list a number of algorithms which we claim should be in sync with xml-encryption, but we are not: https://github.com/bergie/passport-saml/blob/fb1bda0d19e5b206e772562bf0ab6d1a1ce96ae4/lib/passport-saml/saml.js#L1264 xml-encryption supports two "GCML" algorithms which we don't, but should. Also, we should not include "tripledes-cbc" as a supported algorithm if insecure algorithms are disabled. Currently they are, but point 1. above suggests we should.
@cjbarth
Copy link
Collaborator

cjbarth commented Mar 30, 2021

@markstos Is this something that we wanted to address for the upcoming v3 release?

@markstos
Copy link
Contributor Author

@cjbarth We could enable the first phase-- issuing warnings by default for algorithms that are insecure that we will be dropping in a future release.

Also, if we are able to enable the GCML algorithms to be in sync with the spec, we should (assuming they are still considered secure).

In version three, we could have an "insecure algorithms option" that defaults to true-with-warning, with the option to go ahead and disable the insecure algorithms.

So yes, I think it would good to address aspects of this in the next version.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 30, 2021

I was thinking that we could do like we did with a few other things and release a small PR for 2.x that has warnings and then just use the switch for the 3.x branch that we're building now.

@markstos
Copy link
Contributor Author

markstos commented Apr 2, 2021

That works too.

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 2, 2021

I'll gladly review a PR from you on that and I can probable make the 2.x deprecation version that matches the changes you make.

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 a pull request may close this issue.

2 participants