-
Notifications
You must be signed in to change notification settings - Fork 990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #26312 - fix default taxonomy for external users #7219
Fixes #26312 - fix default taxonomy for external users #7219
Conversation
Issues: #26312 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the behavior in an unexpected and possibly dangerous way - new users will be auto created from an external auth source, even if the administrator doesn't want that to occur, and there is no way to disable this setting any more.
Also, I don't understand how exactly does this fix setting a taxonomy for external users?
I had a word with @ares also about the design of external users org/loc so adding him to the thread. @tbrisker I am not sure if I understand this correct but I would like to describe my case here.
Let me know your thoughts. |
Note that EXTERNAL auth source can be used for other means of authentication, not just keycloak. E.g. if you configure kerberos, IPA, client cert authentication etc. EXTERNAL auth source means, we rely on Apache to set REMOTE_USER. Therefore this change affects other auth flows as @tbrisker mentions. I understand what you're trying to achieve, but this rather seems as a workaround. It seems our REST for external auth source actually allows setting org/loc. Can that be a manual step then? It seems we don't have hammer command for that. |
I am still missing something :( I am unable to understand how this would affect the other EXTERNAL auth source means. In any case the admin has to set this value to Also, looking at the code base, the setting Let me know your thoughts on this!
@ares yes, makes sense. But I am still keen on this PR, we can add a hammer command for the admin though. There seems to be few loopholes here, that I am unable to understand, @ares can you please elaborate? |
This patch sets this setting value to External by default. Something users needed to do explicitly before. That means if the Apache is configured to set REMOTE_USER even mistakenly, it will be considered valid authentication now. I was refering to foreman/app/services/sso/apache.rb Line 39 in ce12cd9
The API part I mentioned was for setup, not for authentication itself. If I understood correctly, we need External auth source to exist and assign taxonomies to it. And that can be done through API today. The UI part would make it easier (but big to cherry-pick), the hammer is missing. I'm not against this PR if it proves to have no effect to other auth methods. But especially if you want this to be cherry-picked, it shouldn't impact anything else. |
@@ -20,7 +20,7 @@ def self.default_settings | |||
# websockets_encrypt depends on key/cert when true, so initialize it last | |||
self.set('websockets_encrypt', N_("VNC/SPICE websocket proxy console access encryption (websockets_ssl_key/cert setting required)"), !!SETTINGS[:require_ssl], N_('Websockets encryption')), | |||
self.set('login_delegation_logout_url', N_('Redirect your users to this url on logout (authorize_login_delegation should also be enabled)'), nil, N_('Login delegation logout URL')), | |||
self.set('authorize_login_delegation_auth_source_user_autocreate', N_('Name of the external auth source where unknown externally authentication users (see authorize_login_delegation) should be created (keep unset to prevent the autocreation)'), nil, N_('Authorize login delegation auth source user autocreate')), | |||
self.set('authorize_login_delegation_auth_source_user_autocreate', N_('Name of the external auth source where unknown externally authentication users (see authorize_login_delegation) should be created (If you want to prevent the autocreation, keep unset)'), 'External', N_('Authorize login delegation auth source user autocreate')), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we even unset a value if there's a default? At least the help text doesn't match its function anymore since keep
implies it's the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekohl thanks for the correction in the message :) and yes, you can unset the value!
@ares I am not sure about whether or not this PR brings any drawback. If the user sets REMOTE_USER mistakenly and he does not provide the correct credentials, then he/she will as it is fail to login. I think, there are enough number of checks in each SSO method to validate the users coming from different sources. Although, I would like to bring your attention to this Bugzilla, https://bugzilla.redhat.com/show_bug.cgi?id=1599908. So right now, if a user logs in as an external user using any of the methods like kerberos, etc, then the user does not get assigned to any org/loc. I think this should go in the core but if you still feel the slightest hesitation towards this PR then I will be happy to do the hammer part :) Let me know your thoughts. |
I think the linked BZ is the same cause. Correct me if I'm wrong, but this PR basically causes External auth source be present by default and have default taxonomies assigned. That seems reasonable, but the original concern is, we will now auto create and login users if the web server sets REMOTE_USER to something. We fully trust the value (hence external authentication). If users misconfigured Apache, e.g. having client cert authentication enabled for the whole vhost (we do this by default btw), it can start creating users which is probably unexpected. You only need to have yubikey inserted to your drive and have it's CA trustted. I will let @tbrisker to clarify his concern. But I'm proposing a workaround for both the BZ you linked as well as keycloak - set taxonomies on external auth source manually. If we can find out remediation for unexpected change, I'm happy to have External auth sources created as assigned correctly on default instance. |
AFAIK when you provide credentials but the |
Is there a way user can set REMOTE_USER env variable from headers? AFAIK Apache will prevent this by prefixing user headers with HTTP_. If you spoof REMOTE_USER you'll find the value under HTTP_REMOTE_USER. |
Sorry I don't have much insight on this 🤦♂️ but I still think checks would not let users to just login if |
@ares although as suggested I am adding a hammer command to assign default taxonomies for external users. |
My concern is dismissed by the fact we have authorized_login_delegation setting which still defaults to |
After some reading it appears that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me clarify my main concern here:
Currently, my understanding is that if you have external auth source (such as apache sso or keycloak) configured, if this setting is nil
, that means you have to manually create a user in foreman before that user can log in using the external auth source. This is useful if you only want to give specific users access to foreman by using their external login.
if you change this to setting to External
, that means that every user who can sign in to the external auth source also gets access to Foreman immediately, since their user gets auto-created when they log in. This is usually not what you would expect to happen unless you manually set this setting. In fact, changing this behaviour would cause a significant security issue for those who did not want to grant their entire organization access to Foreman.
When you remove the setting from the BLANK_ATTRS
list, that means that users can no longer set it to empty, meaning that they can't even disable this behaviour.
The other concern I have that you are relying on changing a runtime setting (which should be changed at runtime) to alter the behaviour of the seeding. The creation of the external auth source in the seed is done to make sure that if this setting has been set by the user, the AuthSourceExternal
entry does exist in the database with the correct name, so that it would not fail when an unknown user tries to log in. I am not sure of why this was added, but the fact that it causes taxonomies to be assigned to the auth source is a side effect, not the goal of the seed.
The reason that occurs is that during the initial taxonomy seeding, the default taxonomies get assigned to all existing taxable objects - and since the auth source seed runs earlier, the external auth source is assigned to them. This means that the external auth source would only be assigned to the default taxonomies on a new install, but on existing installations/upgrades it would still remain unassigned.
So in short - I agree with @ares that it is better to work around it by setting the taxonomy explicitly via API rather than modifying this setting to get the side affect. Long term the correct solution is to allow properly managing the auth source from the UI like we do for LDAP.
AFAIK, you can not have an external auth source without the setting being set to
If the setting is set to
I am not sure about the internals, but by lookign at it, I am being able to unset the attribute to
Looking at the seed this line https://github.com/theforeman/foreman/blob/develop/db/seeds.d/030-auth_sources.rb#L11 is never being executed at all. Not at the time of installation and also not at the time of upgrade.
This condition imo does not run in any case. When there is a new installation the
Right now we only have a :PUT request to update the Auth Source(theforeman/hammer-cli-foreman#461), but that would come into picture only when the AuthSourceExternal is present. To make that present we need to add an API endpoint that makes it possible to create an AuthSourceExternal. IMO, this should go into the core. @ares @tbrisker let me know your thoughts. I might be wrong here, just trying to get an idea of what we need to do to get this done with :) |
That is incorrect. You can have this setting set to
This is again incorrect. This setting allows any user who is authenticated by external auth source to get a user in Foreman. Remember there are multiple implementations of external auth, not just keycloak.
This covers the (probably rare) condition where the setting has been set to "External" (or anything else - could also be e.g. "apache" or "myCoolAuthSource") but no user has been auto created by it until the seeds run again. That is the only case in which the auth source would indeed be created by the seed. In any case, relying on this setting to get the auth source created makes no sense to me.
I would be fine with seeding an External Auth source always, but this requires a bit more thought than just removing the condition in the seed. For example, what do we do if there is already an auth source with a different name? What if they had more than one created (i.e. used one name, created it and changed the name)? |
I'd say the update is enough if our long term plan is to seed it automatically. Note there should always be just one external auth source. Edit API is there already. So we need CLI and UI for the same. |
@ares The only issue I see with this approach is that when the Foreman is initially installed, at that time the admin will not be able to use the update API because at that point in time there is no One update: the PR that consumes the update API is merged in hammer. |
What I'm trying to say is - let's seed the auth source by default. Let's also add UI and Hammer for updating taxonomies for existing external auth source that is created as part of the seed. But let's do that in 1.25/2.0 as this is potentially breaking. If we do both changes, create API is not necessary. Introducing that only for 1.24 does not feel very valuable if there's a way to do it manually and if we're improving that in next version. |
@ares that looks like a reasonable solution to me. Let me try a work around form the Keycloak side in 1.24. What I propose is: send the org/loc information in the JWT from Keycloak, once decoded, we can use that to assign it to the appropriate user! |
How would you like to do the mapping? Or how keycloak knows about Foreman's orgnization so that it can put it to the token? |
@ares in Keycloak we have an Few questions that I answered to myself:
Again this is in theory, I am not sure if this would work as explained but this is what I would propose. This would be a time-consuming PR, for now, do we agree on seeding the external users by default? |
I have trouble following the entire discussion since I'm largely unfamiliar with external auth. I may be missing things. I have a preference for only enabling external auth when the (web)server is actually configured to use that rather than enabling by default. In theforeman/puppet-foreman#779 I've started code to configure the system for keycloak. Since the installer is aware of the entire system and able to manage settings, we can do it there. It's using Does this help this discussion? |
If I understand it correctly, @ekohl somehow when an admin enables SSO(via keyclaok) in installer, the setting would be set to |
At this point I think |
There are two parts to this: one is creating the ExternalAuthSource object in the database, the other is actually enabling it (via
As was mentioned multiple times before, setting
I don't think this is worth the effort and I don't think that we should rely on the external auth source to set location/organization in foreman. I don't expect the keycloak admin to need to know the foreman internals. |
FTR, fully agree with @tbrisker's reply |
If keycloak is set via the installer, it must be set to external. Otherwise the installer shouldn't set it and let the user change it. That's at least my understanding. Edit: Just to be clear: right now I don't know how to proceed with the installer because it's not clear to me how & what settings to set. Should I wait with that until this PR has come to a resolution? |
Perhaps as a first step the installer will only set up the apache mod needed for it to work and leave the settings for the user to manually configure themselves? or does the mod need also some settings for it to work? |
No, I can leave the settings out. I just don't know how to properly verify it works end to end. |
Closing this PR as we decided to create a rake task for AuthSourceExternal and admins could then change org/loc from hammer command. PR for the rake task: https://github.com/theforeman/foreman/pull/7264/files |
This PR makes sure that the external users are assigned default taxonomy on creation.
@tbrisker can you please take a look at this one?