Produits Entr'ouvert » Lasso

Instead of doing an assign_string(.., NULL) you can do a lasso_release_string(...), it also reset the target to NULL and is more idiomatic in Lasso.

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.

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 and 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.

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.

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.

Thank you for the excellent bug report Dmitri and careful analysis, it is much appreciated.

Dmitrii S. a écrit :

Benjamin Dauvergne a écrit :

I reverted my push; Dmitrii could you add a License: MIT 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.

Sure, added the inbound license.

Btw (just curious), is there a public contribution policy doc for lasso?

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.