https://dev.entrouvert.org/https://dev.entrouvert.org/favicon.ico?15861920342019-06-28T08:32:20ZRedmine Entr’ouvertLasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1812412019-06-28T08:32:20ZBenjamin Dauvergne
<ul><li><strong>Assigné à</strong> mis à <i>John Dennis</i></li></ul><p>It seems ok but I'm not the more knowledgeable person on the ECP binding, I attribute the ticket to John Dennis for review.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1812432019-06-28T08:37:11ZBenjamin Dauvergne
<ul></ul><p>Instead of doing an <code>assign_string(.., NULL)</code> you can do a <code>lasso_release_string(...)</code>, it also reset the target to NULL and is more idiomatic in Lasso.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1812582019-06-28T09:06:58ZDmitrii S.
<ul><li><strong>Fichier</strong> <a href="/attachments/35668">0001-PAOS-Do-not-populate-Destination-attribute.patch</a> <a class="icon-only icon-download" title="Télécharger" href="/attachments/download/35668/0001-PAOS-Do-not-populate-Destination-attribute.patch">0001-PAOS-Do-not-populate-Destination-attribute.patch</a> ajouté</li></ul><p>Benjamin Dauvergne a écrit :</p>
<blockquote>
<p>Instead of doing an <code>assign_string(.., NULL)</code> you can do a <code>lasso_release_string(...)</code>, it also reset the target to NULL and is more idiomatic in Lasso.</p>
</blockquote>
<p>Thanks for triaging and the review.</p>
<p>Updated the patch and retested.</p>
<p>Will wait for feedback from John.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1816722019-07-01T22:00:44ZJohn Dennisjdennis@redhat.com
<ul></ul><p>The analysis was well done, Dmitri correctly identified the root cause, the Destination attribute should not be set when generating an AuthnRequest to be transported via PAOS.</p>
<p>However, I have nits with the proposed patch part of which has to do with the code legacy in lasso. It is unfortunate that lasso_saml20_profile_build_request_msg() takes a url parameter which is obstinately used to the the msg_url (i.e. the receiver of the message). That's fine for many of the SAML profiles but it's not for ECP. ECP has a different requirement, it needs to set the responseConsumerURL attribute in the paos:Request element to the AssertionConsumerServiceURL. Because the existing function signatures (lasso_saml20_profile_build_request_msg() and lasso_saml20_profile_build_*_msg() variants) did not allow for drawing a distinction between the msg_url and an assertionConsumerServiceURL <strong>and</strong> because lasso_saml20_profile_build_request_msg() demanded a url (or it would pick one), the url parameter was abused. For PAOS the url parameter was set to the assertionConsumerServiceURL so that it could eventually populate the paos:responseConsumerURL via the call to lasso_node_export_to_paos_request_full(). That's the history, but to make things cleaner would require a bit of refactoring, we don't need to refactor now to fix this, just pointing out it might be worthwhile down the road.</p>
<p>Given the above I would recommend removing the added comment in lasso_saml20_login_build_authn_request_msg which discusses the destination attribute, it's just confusing matters because this is not the part of the code that is dealing with the issue. If any comment needs to be added here it probably would be better to comment on the abuse of the url parameter. The only other change in lasso_saml20_login_build_authn_request_msg() aside from the comment is renaming the local variable "url" to "consumer_url". I'm good with that, it's always better to be explicit (url is way too generic). However my suggestion would be to maintain consistency with the existing naming practice and call the local variable "assertionConsumerServcieURL" as is done elsewhere.</p>
<p>The actual fix is in lasso_saml20_profile_build_request_msg(), the comment is good but setting the Destination attribute to NULL might be more appropriately done with lasso_release_string(xxx) as opposed to lasso_assign_string(xxx, NULL), I don't think there are any existing examples of using lasso_assign_string() with NULL (it should work fine). But that is a nit I'll leave up to Benjamin as to what he prefers for code consistency.</p>
<p>Thank you for the excellent bug report Dmitri and careful analysis, it is much appreciated.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1819292019-07-03T06:20:19ZDmitrii S.
<ul><li><strong>Fichier</strong> <a href="/attachments/35786">0001-PAOS-Do-not-populate-Destination-attribute.patch</a> <a class="icon-only icon-download" title="Télécharger" href="/attachments/download/35786/0001-PAOS-Do-not-populate-Destination-attribute.patch">0001-PAOS-Do-not-populate-Destination-attribute.patch</a> ajouté</li></ul><p>John,</p>
<p>Thanks a lot for the prompt reply, code review and prior work across the stack.</p>
<blockquote>
<p>That's the history, but to make things cleaner would require a bit of refactoring, we don't need to refactor now to fix this, just pointing out it might be worthwhile down the road.</p>
</blockquote>
<p>I agree that an API redesign might take a significant amount of time and thought while a more tactical fix will solve the problem without that, especially considering that it will be easier to backport a small fix back to older versions of lasso at the distribution level and unblock users in terms of using ECP.</p>
<p>I addressed the review in the 3rd iteration of the patch uploaded with this message. I agree that extra comments are not necessary in lasso_saml20_login_build_authn_request_msg and that variable naming should be consistent with everything else. I added a reference to this bug in a comment in the code for context though.</p>
<p>lasso_release_string is now also used in lasso_saml20_profile_build_request_msg.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1821492019-07-03T18:04:43ZAnonyme
<ul><li><strong>Statut</strong> changé de <i>Nouveau</i> à <i>Résolu (à déployer)</i></li><li><strong>% réalisé</strong> changé de <i>0</i> à <i>100</i></li></ul><p>Appliqué par commit <a class="changeset" title="PAOS: Do not populate "Destination" attribute When ECP profile (saml-ecp-v2.0-cs01) is used with..." href="https://dev.entrouvert.org/projects/lasso/repository/33/revisions/77c0bc86058e3d83042dac9e8297e2a571ed4da6">77c0bc86058e3d83042dac9e8297e2a571ed4da6</a>.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1821502019-07-03T18:07:49ZBenjamin Dauvergne
<ul><li><strong>Statut</strong> changé de <i>Résolu (à déployer)</i> à <i>Solution validée</i></li></ul><p>I reverted my push; Dmitrii could you add a <code>License: MIT</code> header to the bottom of your patch to state that you contributed (and Canonical your employer) under the MIT license ? We do dual licensing on Lasso and we need that for external contributors.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1821512019-07-03T19:31:37ZDmitrii S.
<ul><li><strong>Fichier</strong> <a href="/attachments/35803">0001-PAOS-Do-not-populate-Destination-attribute.patch</a> <a class="icon-only icon-download" title="Télécharger" href="/attachments/download/35803/0001-PAOS-Do-not-populate-Destination-attribute.patch">0001-PAOS-Do-not-populate-Destination-attribute.patch</a> ajouté</li></ul><p>Benjamin Dauvergne a écrit :</p>
<blockquote>
<p>I reverted my push; Dmitrii could you add a <code>License: MIT</code> header to the bottom of your patch to state that you contributed (and Canonical your employer) under the MIT license ? We do dual licensing on Lasso and we need that for external contributors.</p>
</blockquote>
<p>Sure, added the inbound license.</p>
<p>Btw (just curious), is there a public contribution policy doc for lasso?</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1821632019-07-03T21:53:31ZBenjamin Dauvergne
<ul></ul><p>Dmitrii S. a écrit :</p>
<blockquote>
<p>Benjamin Dauvergne a écrit :</p>
<blockquote>
<p>I reverted my push; Dmitrii could you add a <code>License: MIT</code> header to the bottom of your patch to state that you contributed (and Canonical your employer) under the MIT license ? We do dual licensing on Lasso and we need that for external contributors.</p>
</blockquote>
<p>Sure, added the inbound license.</p>
<p>Btw (just curious), is there a public contribution policy doc for lasso?</p>
</blockquote>
<p>Not really, we need a permissive license for external contributions so that we can relicense as we want, but the public distribution license is GPL v2 or later.</p> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1821642019-07-03T21:59:28ZBenjamin Dauvergne
<ul><li><strong>Statut</strong> changé de <i>Solution validée</i> à <i>Résolu (à déployer)</i></li></ul><pre>
commit 1e85f1b2bd30c0d93b4a2ef37b35abeae3d15b56 (HEAD -> master, origin/master, origin/HEAD)
Author: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
Date: Fri Jun 28 02:36:19 2019 +0300
PAOS: Do not populate "Destination" attribute
When ECP profile (saml-ecp-v2.0-cs01) is used with PAOS binding Lasso
populates an AuthnRequest with the "Destination" attribute set to
AssertionConsumerURL of an SP - this leads to IdP-side errors because
the destination attribute in the request does not match the IdP URL.
The "Destination" attribute is mandatory only for HTTP Redirect and HTTP
Post bindings when AuthRequests are signed per saml-bindings-2.0-os
(sections 3.4.5.2 and 3.5.5.2). Specifically for PAOS it makes sense to
avoid setting that optional attribute because an ECP decides which IdP
to use, not the SP.
Fixes Bug: 34409
License: MIT
Signed-off-by: Dmitrii Shcherbakov <dmitrii.shcherbakov@canonical.com>
</pre> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1892332019-09-03T11:48:43ZBenjamin Dauvergne
<ul><li><strong>Version cible</strong> changé de <i>future</i> à <i>2.6.1</i></li></ul> Lasso - Bug #34409: [PAOS][ECP] lasso includes "Destination" attribute in SAML AuthnRequest populated with SP AssertionConsumerServiceURL when ECP workflow is used which leads to IdP-side errorshttps://dev.entrouvert.org/issues/34409?journal_id=1899262019-09-06T12:40:09ZBenjamin Dauvergne
<ul><li><strong>Statut</strong> changé de <i>Résolu (à déployer)</i> à <i>Solution déployée</i></li></ul>