Project

General

Profile

Bug #69673

Invalid ProxyRestriction generated when Count is set

Added by Maxime Besson 4 months ago. Updated 4 months ago.

Status:
Fermé
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
28 September 2022
Due date:
% Done:

0%

Estimated time:
Patch proposed:
No
Planning:
No

Description

Lasso 2.8.0 generates invalid AuthnRequests when a ProxyRestriction element is added with a set Count

  ...
  <saml:Conditions>
    <saml:ProxyRestriction>
      <saml:Audience>http://test.example.com/saml</saml:Audience>
      <saml:Count>0</saml:Count>
    </saml:ProxyRestriction>
  </saml:Conditions>
  ...

This XML is invalid per the SAML specification (and is rejected as such by Onelogin's samltool.com, keycloak, and any implementation that checks the SAML XML schema

In http://docs.oasis-open.org/security/saml/v2.0/saml-schema-assertion-2.0.xsd, Count is defined as an attribute of ProxyRestriction, and not a subelement, the correct result should be:

  <saml:Conditions>
    <saml:ProxyRestriction Count="0">
      <saml:Audience>http://test.example.com/saml</saml:Audience>
    </saml:ProxyRestriction>
  </saml:Conditions>

This feature (ProxyRestriction) is used by LemonLDAP::NG (which relies on Lasso) and causes compatibility issues with some SAML IDPs

I have attached a C test case to show the issue


Files

invalid-proxy-restriction.c (445 Bytes) invalid-proxy-restriction.c Maxime Besson, 28 September 2022 02:28 PM
fix-ProxyRestriction-Count.diff (588 Bytes) fix-ProxyRestriction-Count.diff Maxime Besson, 28 September 2022 05:59 PM

Associated revisions

Revision 3a7ad361 (diff)
Added by Benjamin Dauvergne 4 months ago

Fix parsing of Count attribute of saml:ProxyRestriction (#69673)

History

#1

Updated by Benjamin Dauvergne 4 months ago

Great catch. Would you provide a patch to fix it ?

#2

Updated by Benjamin Dauvergne 4 months ago

  • Status changed from Nouveau to Information nécessaire
  • Assignee set to Maxime Besson
#3

Updated by Frédéric Péters 4 months ago

  • Project changed from Produits Entr'ouvert to Lasso
#4

Updated by Maxime Besson 4 months ago

The attached patch solves my use case but I'm not sure if anything else needs to be done in the lib.

Perhaps a release note should mention the fact that Lasso systems connected to each other should be upgraded at the same time to avoid errors when receiving incorrect messages from an older version?

#5

Updated by Benjamin Dauvergne 4 months ago

Maxime Besson a écrit :

The attached patch solves my use case but I'm not sure if anything else needs to be done in the lib.

Perhaps a release note should mention the fact that Lasso systems connected to each other should be upgraded at the same time to avoid errors when receiving incorrect messages from an older version?

Of course, I'll add it but I'm pretty sure you are the sole users of this SAML feature :-) For my education, what is the use case which mandates the use of this ProxyRestriction and with which software on the other party do you use it ? Or is it used solely among LemondLDAP instances ?

#6

Updated by Benjamin Dauvergne 4 months ago

  • Tracker changed from Support to Bug
  • Status changed from Information nécessaire to Fermé
  • Target version set to 2.8.1
commit 3a7ad3610f64cfa4a231fb543712371c1828e5e4 (HEAD -> main, origin/main, origin/HEAD)
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Wed Sep 28 18:18:36 2022 +0200

    Fix parsing of Count attribute of saml:ProxyRestriction (#69673)

#7

Updated by Maxime Besson 4 months ago

Benjamin Dauvergne a écrit :

Of course, I'll add it but I'm pretty sure you are the sole users of this SAML feature :-) For my education, what is the use case which mandates the use of this ProxyRestriction and with which software on the other party do you use it ? Or is it used solely among LemondLDAP instances ?

LemonLDAP sets a ProxyRestriction by default when sending an AuthnRequest, the intended purpose of this is to prevent the IDP from using another IDP to authenticate the user. However, after reading the spec carefully, I think this is not what ProxyCondition is for.
ProxyCondition prevents a SP receiving an assertion (LemonLDAP) from using it on another service (such as a LemonLDAP-protected app) which is what we do in every case. So I think we will end up removing it.

This patch however might help with old LLNG versions, and makes Lasso more spec-compliant, so thanks for including it!

Also available in: Atom PDF