Contrib.social

Advanced/security options UX / default values

Open on Drupal.org →
Created on 26 April 2020, over 4 years ago
Updated 10 August 2023, over 1 year ago

This issue is opened for review by anyone who has an opinion on how some 'advanced' features should be configured by default.

There are some 'security related' options in the module which have been around forever but which I've never really thought through properly. Until now: contributions of various extra config options by others have made me dive into the library code. I suspect we can make the default configuration/testing experience a bit simpler for users by deviating from the standard library settings, without compromising security / future extensibility / testing experience for others.

So... I'll write up my current thoughts here. If someone else has an opinion on their effects, or the user experience, I'll listen. Otherwise... these might be 'live notes' I'll slowly modify in the future.

These are the options in question:

'UX' related

  • The 'security' section has been split into two sections "SAML Message Construction" and "SAML Message Validation":
    • Primarily because the section was getting too big and therefore confusing; it was getting harder to see what each option does.
    • It feels to me like new users will have more of a handle of what all these options do, if they are split by incoming vs outoing message, rather than security vs. miscellaneous options.
    • As a side note: naming it "security" is not hugely important to draw people's attention to going over all options carefully. We're shipping with all "secure" options enabled (except for encryption of assertions, which IMHO is not necessary for most people operating over HTTPS). I'll just add a note to the README saying that if you turn them off while testing, you need to think of turning them on again.
  • Until now I was leaning towards shorter description texts - partly because I did not understand some options (like "Specify authentication context", which until now was called "Requested authn context" without any explanation). I've switched from that to trying to explain things in a concise manner, including what the SAML toolkit library does. It likely still doesn't make sense to new people, but it's something to hold on to.
  • The "NameID format" is part of the SP configuration (also) according to the SAML Toolkit library. However, 1) it was never really explained 2) we likely don't need it at all; 3) the default is now sensible ("undefined" rather than empty string). So I moved it into the 'security' section below the new NameID related checkboxes.

Default values / necessity of configuration

  • The NameID related configuration is likely unnecessary. At least the samlauth module doesn't use it at all (so far - because it wants a 'unique ID' to be a SAML attribute value). We might be able to turn the "Specify NameID policy" and "Require NameID" options off by default, which might be easier for users because they won't need to think about them. However
    • We might want to still build in support for NameID as unique ID. (Oh - I see there's even a years-old issue where I said I would.)
    • We might want to build in auto-configuration of various settings by reading the IdP's metadata XML. I'm hesitant to change defaults until then (because I might see things differently after that).
    • Even though it likely makes no difference at all security wise, it still feels better to do as many checks as possible - i.e. always check for the presence of a NameID unless the user explicitly turns that check off, even if we don't use it.
  • Over various iterations / evolutions of understanding, I've renamed "Encode urls in lowercase" to "'Raw' encoding of SAML messages", changed the default to True and moved it into a "Debugging" section to signify people don't need to look at it. I'm now convinced that its implementation and naming was based on a misunderstanding, and that this does not have any adverse effect - while fixing things for ADFS IdPs. I hope to send a PR for onelogin/php-saml sometime, to change the default and deprecate this option.
  • The "Retrieve logout signature parameters from $_SERVER['REQUEST']" option is yucky. Like the previous point, it should be set to True by default and deprecated - but unlike the previous point, this might affect existing installs until the onelogin/php-saml code is changed. (I hope to create a separate PR for that sometime.) Until then, this will unfortunately keep adversely affecting the default installs working with ADFS (and other) IdPs.

Here's the comments I dumped into the code for now re. the $_SERVER['REQUEST'] option:

    // This option's default value is FALSE but according to the SAML spec,
    // signing parameters should always be retrieved from the original request
    // instead of recalculated. (As argued in e.g.
    // https://github.com/onelogin/php-saml/issues/130.) The 'TRUE' option
    // (which was implemented in #6a828bf, as a result of
    // https://github.com/onelogin/php-saml/pull/37) reads the parameters from
    // $_SERVER['REQUEST'] but unfortunately this is not always populated in
    // all PHP/webserver configurations. IMHO the code should have a fallback
    // to other 'self encoding' methods if $_SERVER['REQUEST'] is empty; I see
    // no downside to that and it would enable us to always set TRUE / get rid
    // of this option in a future version of the SAML Toolkit library.
    // @todo file PR against SAML toolkit; note it in https://www.drupal.org/project/samlauth/issues/3131028
✨ Feature request
Status

Closed: outdated

Version

3.0

Component

User interface

Created by

🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment over 1 year ago →
    🇺🇸United States rishi kulshreshtha

    @roderik, thank you for your efforts on this module and for creating this ticket.

    It feels to me like new users will have more of a handle of what all these options do, if they are split by incoming vs outoing message, rather than security vs. miscellaneous options.

    That's an excellent suggestion!

    We might want to build in auto-configuration of various settings by reading the IdP's metadata XML. I'm hesitant to change defaults until then (because I might see things differently after that).

    I agree with that approach. In general, we can include an import field that accepts XML, allowing us to process the IdP's metadata and populate the admin/config/people/saml form automatically.

  • Status changed to Closed: outdated over 1 year ago
  • Comment over 1 year ago →
    🇳🇱Netherlands roderik Amsterdam,NL / Budapest,HU

    Thank you for the review.

    The suggestion you mentioned was implemented at the time I created this ticket (2020), and I'm fairly happy with it.

    I'm still struggling with anything around the NameID, and will likely move things around while implementing 🌱 NameID support Fixed ... it's almost impossible to make this configuration understandable for all setups, but I'll try / keep iterating.

    I believe that after so many years of the possibility for feedback, this ticket can be closed. (And the screenshots will likely soon be a bit outdated.)

    The "auto-configuration of various settings" is related to a ticket that was opened separately: ✨ Automatic IdP metadata retrieval Active -- so that can be discussed there if necessary.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024