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 zero signature in SAML response #467

Open
carsonwah opened this issue Feb 4, 2022 · 4 comments
Open

Support zero signature in SAML response #467

carsonwah opened this issue Feb 4, 2022 · 4 comments
Labels
discussion security security related issues

Comments

@carsonwah
Copy link

carsonwah commented Feb 4, 2022

Hi, I'm integrating my application with an IDP that returns encrypted response but without a signature in their SAML response. I encountered Error: ERR_ZERO_SIGNATURE thrown by samlify.

In the source code of samlify, it is hardcoded to expect at least 1 signature in SAML response:

samlify/src/libsaml.ts

Lines 384 to 387 in 1bf1eba

// guarantee to have a signature in saml response
if (selection.length === 0) {
throw new Error('ERR_ZERO_SIGNATURE');
}

Would it be reasonable to support an option to loosen this restriction? If not, is there any underlying reason why we should enforce this behaviour?

Many thanks.

@cisacpalma
Copy link

I have the same problem

@tngan
Copy link
Owner

tngan commented Apr 10, 2022

@carsonwah @cisacpalma According to the section 4.1.3.5 in specification http://docs.oasis-open.org/security/saml/v2.0/saml-profiles-2.0-os.pdf,

The <Assertion> element(s) in the <Response> MUST be signed, if the HTTP POST binding is used

That means the assertion in response issued from identity provider must be signed, you can also sign the outer response as a message signature to provide extra security protection, but assertion signature is the only requirement stated in the specification. There is a discussion thread on it http://docs.oasis-open.org/security/saml/v2.0/saml-profiles-2.0-os.pdf.

it seems optional in Artifact binding but this library does't support it now. It would be good to know the reason why the signature has to be ignored in your use cases, and feel free to discuss here. :D

@tngan tngan added discussion security security related issues labels Apr 10, 2022
@carsonwah
Copy link
Author

carsonwah commented Aug 10, 2022

@tngan Sorry for the late reply. I missed the notification.
I believe you mis-pasted the 2nd link. Would be great if you could share again.

Below are my points of view:

  1. For the "profile specification" you shared, in section 4.1.3, it states:

If the profile is initiated by the service provider, start with Section 4.1.3.1. If initiated by the identity
provider, start with Section 4.1.3.5.

That means the "MUST" you mention only applies to IdP-initiated login.
In my case, it is SP-initiated and should not be related.

  1. In general, signing should be optional, as stated in Assertions and Protocols Specs for SAML 2.0 - section 2.3.3:

The SAML assertion MAY be signed
For the motivation of the line you mentioned, I believe is due to the untrusted nature of POST binding, which is discussed in SAMLBind Spec - section 3.5.5.2.

In my case, IdP is using <EncryptedAssertion> instead of <Assertion>, which is effectively providing integrity as well on the encrypted assertion content. I believe that's why they didn't include a signature even for POST binding.
image

  1. In the Metadata Spec - section 2.4.4 and samlify doc, it is possible to configure wantAssertionsSigned (false by default). Intuitively, it would make more sense to expect the assertion may or may not be signed.

Based on above, I would suggest relaxing this hardcoded constraint, either by making it optional through parameter, or directly removing the constraint (I'm not sure which one makes more sense).

Currently I forked the repo to solely remove this checking in order to work with my IdP. It would be great if this suggestion could be considered. Thanks.

@davendu
Copy link

davendu commented Dec 27, 2022

Hi @tngan. I have the same issue, and I agree with carson's opinion. SP-initiated login does not require signature, which is exactly my case.

I think this should be an option so people working with this type can prevent ERR_ZERO_SIGNATURE errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion security security related issues
Projects
None yet
Development

No branches or pull requests

4 participants