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

Support newer cryptographic algorithms for SAML signing and signature verification #82

Open
andrew-ar opened this issue Mar 6, 2024 · 5 comments
Labels
auth enhancement New feature or request

Comments

@andrew-ar
Copy link

andrew-ar commented Mar 6, 2024

Is your feature request related to a problem? Please describe.
This issue is to ask what the maintainers intend to do about cryptographic obsolescence to future-proof this project.

The https://github.com/amdonov/xmlsig library used by this project has not been updated for 6 years. It lacks current cryptographic algorithms and over time the algorithms it supports are likely to be undermined by cryptoanalytical attacks. In particular, SHA256 is the only modern hash algorithm supported: when that is deprecated (just as MD5 and SHA1 were before it), there are no other good options left to use, resulting in a risk of forgery of SAML responses allowing unauthorised access.

Describe the solution you'd like
Replace the old https://github.com/amdonov/xmlsig library with one that's actively maintained. Is there a reason for not using https://github.com/russellhaering/goxmldsig for all operations?

Describe alternatives you've considered
We can live with this for the time being, but long-term the only good option is to replace the obsolete library.

I have some background in cryptography and golang myself, if necessary I'd be willing to maintain a fork if that helps? e.g. see my PR here.

Additional context
Over time, cryptoanalysts develop new mathematical attacks against cryptographic algorithms and computing power continues to get cheaper. This results in cryptographic algorithms which are "good enough" today being deprecated over time. For example:

These have lead to deprecations of such algorithms, for example:

It's very likely that the set of crypto algorithms in the XML signature libraries used here will one day become deprecated too.

This repository depends on two XML signature projects:

  1. https://github.com/russellhaering/goxmldsig
  2. https://github.com/amdonov/xmlsig

The goxmldsig library appears to be updated periodically and supports various newer algorithms such as SHA384 and SHA512.

However, https://github.com/amdonov/xmlsig has not had a commit for 6 years and does not appear to be maintained any more. It supports DSA and RSA encryption and SHA1 and SHA256 hashing only, omitting several newer algorithms included in the XML Signature Syntax and Processing Version 2.0 standard (from 2015) and the corresponding RFC 4051, notably ECDSA encryption and SHA384 and SHA512 hashing. It's a risk to rely on this obsolete library.

As noted above SHA1 is already deprecated, so SHA256 is the only viable signature algorithm for people concerned about cryptographic security. If a flaw is discovered in SHA256, users of Zitadel SAML won't have any available alternative algorithm to safely use. Cryptanalysis of the SHA2 family is already in progress. How long before it becomes deprecated too?

@andrew-ar andrew-ar added the enhancement New feature or request label Mar 6, 2024
@andrew-ar
Copy link
Author

Edited the description to fix a copy/paste error on a URL.

@andrew-ar
Copy link
Author

Some additional information in case it helps, I was reading the Microsoft Entra ID (Azure Active Directory) documentation here and I notice it mentions that they support SHA384 and SHA512. This means certain Entra configurations won't work with Zitadel today.

Even if the crypto algorithm limitation isn't addressed soon, this might be worth mentioning in the proposed Entra ID Integration Guide as it would affect the IdP SAML signing configuration a user would need to set up. I hope this information is helpful.

@fforootd
Copy link
Member

Hi @andrew-ar you are raising some valid points here.

Let us work on an thorough reply in the next few days.

@fforootd
Copy link
Member

CC @stebenz maybe also @livio-a

@livio-a
Copy link
Member

livio-a commented Mar 20, 2024

@stebenz can we have a short meet in the next few days.

I just noticed you tried to use that lib in the first place anyway, but apparently there was an issue:

/*
commented as russellhaering/goxmldsig produces invalid signatures for responses currently
func Create(signingContext *dsig.SigningContext, element interface{}) (*xml_dsig.SignatureType, error) {
data, _, err := canonicalize(element)
if err != nil {
return nil, err
}
doc := etree.NewDocument()
if err := doc.ReadFromBytes(data); err != nil {
return nil, err
}
signedEl, err := signingContext.SignEnveloped(doc.Root())
if err != nil {
return nil, err
}
sigEl := signedEl.Child[len(signedEl.Child)-1]
sigTyped := sigEl.(*etree.Element)
sigDoc := etree.NewDocument()
sigDoc.SetRoot(sigTyped)
reqBuf, err := sigDoc.WriteToBytes()
if err != nil {
return nil, err
}
sig, err := xml.DecodeSignature("", string(reqBuf))
if err != nil {
return nil, err
}
// unfortunately as the unmarshilling is correct but the innerXML attributes still contain the element with namespace they have to be cleaned out
sig.InnerXml = ""
sig.SignedInfo.InnerXml = ""
sig.SignedInfo.CanonicalizationMethod.InnerXml = ""
sig.SignedInfo.SignatureMethod.InnerXml = ""
for i := range sig.SignedInfo.Reference {
ref := sig.SignedInfo.Reference[i]
for j := range ref.Transforms.Transform {
ref.Transforms.Transform[j].InnerXml = ""
}
ref.Transforms.InnerXml = ""
ref.InnerXml = ""
sig.SignedInfo.Reference[i] = ref
}
sig.SignatureValue.InnerXml = ""
sig.KeyInfo.InnerXml = ""
for i := range sig.KeyInfo.X509Data {
d := sig.KeyInfo.X509Data[i]
d.InnerXml = ""
sig.KeyInfo.X509Data[i] = d
}
return sig, nil
}*/

@livio-a livio-a self-assigned this Mar 20, 2024
@livio-a livio-a removed their assignment May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auth enhancement New feature or request
Projects
Status: 📨 Product Backlog
Development

No branches or pull requests

4 participants