Experimental support for remote_user/shib, do mind that this requires…

[einar]

… authentication BEFORE access to RT if enabled.

Sadly I had trouble using already existing options like ExternalSettings as remote user is a bit... different. With the case of Shibboleth I already know that the incoming user is authenticated and allowed to access RT; I just want to populate the user with decent attributes taken from REMOTE_USER. I also had trouble trying to document this patch as the documentation basically states that Cookies are the only SSO-option available so I was at a loss when I started. I'd love a discussion about this patch, it does work for me (with 2 additional settings in RT_SiteConfig.pm; WebRemoteUser and ExternalSettingsRemoteUser (same syntax as ExtSett)).

[sartak]

This looks pretty reasonable. As far as behavior goes, I'd suggest simply removing the if ($config->{'type'} eq 'shib') { check to let any REMOTE_USER-based system participate.

For "more than 1-1 mapping in attr_map", I'd handle that by something similar to what we do with LDAP-based auth: let the user specify a coderef value for the env var key instead of a simple string. Pass a reference to %user_info so that the admin can manipulate it however makes sense.

[einar]

Yeah, concerning the type I wasnt sure if I'd simply call everything "remoteuser" or have a specific type for just shib. I finally decided to call it shib as to be very clear that the config (both the flag and the setting) are for just shib (although I do guess the headers could have the same name from other sources). So not sure if the best would be for whoever uses another source, let's say krb5, just sets remoteuser to true then copies the type-part and adds their own sanity check and/or config syntax or if (as you say) it should be more generic. Do you want me to change it?

Sorry about the mapping, I was actually first trying to piggyback on the ExternalSettings but there were a few sanity checks that didnt like anything other than ldap/db/cookie and I was afraid the tests would break now or in the future. When I finally used a new variable for settings instead I could very possibly have used another format but by that time I had already the syntax of ExternalSettings in place & I wasnt sure if you'd be OK with a new settings field for remote users anyways. Do you want me to try and fix this as well?

Also: I'm not even sure how to begin documenting this. If you want me to could you please give me some pointers or is it OK to just (try) and create not-so-hard-to-read code and let the project document it if it gets to pushable quality? :)

[einar]

Fixed the FIXME so now you can have 1->many in attr_map. Please let me have feedback on the other 2 issues & any comments on this patch are also welcome. Thank you. :)

[einar]

@sartak please provide pointers if there's anything else you need? The settings for remoteuser (shib) looks like this now: "https://github.com/einar/docker-rt-swamid/blob/master/start.sh#L332"

[einar]

Ping! Would love to have this merged so I wont have to run patch in my production environment... :)

[sartak]

Hi Einar,

Sorry for the delay.

Yeah, concerning the type I wasnt sure if I'd simply call everything "remoteuser" or have a specific type for just shib. I finally decided to call it shib as to be very clear that the config (both the flag and the setting) are for just shib (although I do guess the headers could have the same name from other sources). So not sure if the best would be for whoever uses another source, let's say krb5, just sets remoteuser to true then copies the type-part and adds their own sanity check and/or config syntax or if (as you say) it should be more generic. Do you want me to change it?

The administrator must opt into this system by specifying ExternalSettingsRemoteUser anyway, so limiting it to Shibboleth is unnecessarily constraining. Otherwise administrators who are using something besides Shibboleth might specify type => 'shib' anyway to be able to use your new feature. That would cause all sorts of confusion.

So please simply get rid of the if ($config->{'type'} eq 'shib') { check so everyone can use it :) You say it's "just for shib" but I think that applies only to your specific ExternalSettingsRemoteUser configuration. The feature itself is not just for shib; it's a way to populate user fields from HTTP headers, which any webserver-based auth system could use.

Sorry about the mapping, I was actually first trying to piggyback on the ExternalSettings but there were a few sanity checks that didnt like anything other than ldap/db/cookie and I was afraid the tests would break now or in the future. When I finally used a new variable for settings instead I could very possibly have used another format but by that time I had already the syntax of ExternalSettings in place & I wasnt sure if you'd be OK with a new settings field for remote users anyways. Do you want me to try and fix this as well?

Being able to specify a code reference makes this feature much more powerful. For example the sysadmin can decide to merge multiple values by joining them together into one, or inspecting them with some sort of business logic to decide which to use, or can do something as simple as transforming the value to all uppercase, etc etc. For example:

'Name' => sub {
    my $user_info = shift;
    $user_info->{Name} = uc RequestENV("HTTP_EPPN");
}

I'd also like to see the being able to specify a plain string instead of an array reference of one element. I imagine that will be the most common case, as it is in your example.

Also: I'm not even sure how to begin documenting this. If you want me to could you please give me some pointers or is it OK to just (try) and create not-so-hard-to-read code and let the project document it if it gets to pushable quality? :)

Once we have this part of the code right, we'd be happy to take over fleshing it out and documenting it.

[einar]

Hi Sartak! Nps, sorry for the delay here as well. Been busy the last few days so haven't had time to look at this but I will have a look at it starting tomorrow(ish) or next week.

I really like your comments as it basically was something similar I wanted from the beginning but didn't dare to implement due to the existing structure of ExternalSettings... Shib will simply be removed but when it comes to the ExternalSettingsRemoteUser do I understand you correctly when I basically change the array (of strings) to a slightly smarter array of strings that can also contain variables? Because I still want a priority list (so, I guess an array) as headers I prefer can still be empty etcetera. Was considering simply processing input as a string and if any HTTP_* found they are converted on the fly (and if the whole string turns up empty, due to empty header or no static string, try the next one).

An example could be something like this (last element would be the default if previous headers & variables are all empty):

	'City'         => [ 'HTTP_L','$city','Stockholm' ],
            'RealName'     => [ 'HTTP_DISPLAYNAME','HTTP_CN','HTTP_GIVENNAME HTTP_SN','John Doe' ],

It turns a bit tricky though if, for example, one of the headers is found or if a mix of string/header is used but for now I take the simple approach and use the "first non-whitespace string found" as more advanced logic can be added later (if anyone needs it).

Please clarify if I'm mistaken (or if this is a bad idea) so we don't need yet another iteration, otherwise I'll try and implement the above. :)

[sartak]

That's not quite what I'm after. I'd say that the values for each ExternalSettingsRemoteUser (how about we rename that to RemoteUserHeaders) can be either:

• a string (which means look at this HTTP header for the value for that user attribute)
• an array of strings (which means look at each HTTP header in turn, using the first one that has a value in it)
• a code reference, which returns the value to use for that user attribute

Code references can implement defaults or anything else that isn't simply an HTTP header lookup.

[einar]

Doesn't this mean that I can't have any type of combination of headers or string+header(s) without having to have specific code for every combination of this type? Also is it not easier to skip the string entirely so that I don't have to have (more than one) array inside the array (in my mind if I treat all input as an array of strings I can manipulate a single element array just the same as a string; less logic)?

Just an idea; would it be OK to keep existing syntax (although remove the type-Shib part and change the name per your wishes) and instead of the last part (although not excluding it) have a subroutine that parses input as strings where HTTP_-strings are converted (I guess you could say a code reference for every variable)? Then you can mix strings and headers or have multiple headers and treat everything as an array of strings as to not mix input variable types.

So basically I'd toss every array of strings, even those with a single element, to a subroutine that:

1. Converts all instances of HTTP_* to corresponding values (per element to honor priority)
2. Goes through all elements in order and as soon as ANY non-whitespace string is found; return that string (whatever it is, so this could be anything from an actual header to a static string to a variable containing something)

I feel like this hopefully addresses all your points above and still help me with cases where a remote user wants to mix strings and headers or just have multiple headers in one element (which we actually wanted to do but couldn't in the current pull request).

[rizlas]

Hi, why this was not further developed? It was a "must have" in 2017 and still is in 2023.

I agree with @sartak statement here https://github.com/bestpractical/rt/pull/208#issuecomment-290187429. This should be the way to develop and close this PR.

Also I would also suggest to remove the needs of WebRemoteUser setting.

I'm not a PERL developer and never written a line of it, but I will test this current PR with my current setup (OIDC+RT).

Hope that this comment will start again the discussion.

@einar I'm open for a collaborative session.

[rizlas]

A quick update. I tested the code from this PR with RT 5.0.3 using OIDC (mod_apache_oidc). This sentence from @sartak is totally true, because the code works out of the box even with oidc if you filll and pass a correct HTTP header.

So please simply get rid of the if ($config->{'type'} eq 'shib') { check so everyone can use it :) You say it's "just for shib" but I think that applies only to your specific ExternalSettingsRemoteUser configuration. The feature itself is not just for shib; it's a way to populate user fields from HTTP headers, which any webserver-based auth system could use.

ProxyFCGISetEnvIf "true" HTTP_MAIL "%{reqenv:OIDC_CLAIM_email}

[7] [Tue Jun 27 21:09:28 2023] [info]: Found header: HTTP_MAIL with value [email protected] Mapped to RT variable: EmailAddress  (/opt/rt5/sbin/../lib/RT/Interface/Web.pm:280)
[7] [Tue Jun 27 21:09:28 2023] [info]: RT variable EmailAddress set to [email protected] taken from header HTTP_MAIL (/opt/rt5/sbin/../lib/RT/Interface/Web.pm:284)