Skip to content
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

Review adding AD as an external authentication source #3149

Merged
merged 33 commits into from
Sep 26, 2024

Conversation

asteflova
Copy link
Member

@asteflova asteflova commented Jul 18, 2024

What changes are you introducing?

After reviewing the existing procedure for adding Active Directory as an external authentication source when the base system of the Foreman server is connected directly to AD, I propose these changes:

  • Drop steps related to privilege separation for Apache
  • Drop a couple of other instructions that are now obsolete
  • Drop adding more detailed explanations of the steps the procedure recommends
  • Add a list of authentication features available to AD users
  • Other edits for clarity and to set the context of the whole scenario

The current procedure for direct AD integration + configuring AD as an external authentication source + using GSS Proxy for privilege separation is faulty. For example, the steps don't really achieve Apache keytab separation as promised, they combine Samba-based and SSSD-based AD join, and some of them could be replaced with a reference to FreeIPA/IdM docs to make sure we don't need to fret about keeping in sync with SSSD development.

In its latest state (after quite a few changes in course), this PR proposes a complete restart to how we document the use case of direct AD integration -- and to document a minimum viable workflow to achieve direct AD integration and adding AD as an external authentication source:

* First, refer users to FreeIPA/IdM docs for the integration to AD (referencing SSSD as the recommended way to join an AD domain and omitting references to Samba)
* Then, advise users on how to configure AD as an external authentication source

This means that the procedures no longer attempt to configure privilege separation with GSS Proxy. If needed, we can add steps to that purpose later after we agree on the necessary minimum proposed in this PR.

Why are you introducing these changes? (Explanation, links to references, issues, etc.)

https://issues.redhat.com/browse/SAT-22855 requests making changes to how the existing procedure on direct AD integration + configuring AD as an external authentication source handles privilege separation for Apache with GSS proxy. While investigating that scenario, it turned out that proper privilege separation may currently not be possible to achieve. Therefore, the agreed-upon approach is to first document the basic steps for AD enrollment + authentication source configuration and then, in another PR, continue the work by adding steps to achieve proper privilege separation.

This PR originated with https://issues.redhat.com/browse/SAT-22855 and then... escalated.

According to https://issues.redhat.com/browse/SAT-22855, the procedure for direct AD integration with GSS-proxy needs updating. This PR continues the effort from #2737.

Additionally, https://issues.redhat.com/browse/SAT-26355 requests adding two oddjob* packages to the same assembly.

Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)

Some might object to leaving Samba out of the picture. These resources explain why SSSD is preferred over Samba for direct AD integration:

* 1.1. Overview of direct integration using SSSD
* 6.1. Direct integration of Linux systems into Active Directory

I suspect that the procedures might have originally involved Samba because SSSD was not sufficiently mature when the procedures were written. However, SSSD has evolved in the past few years and can stand on its own now, except for limited edge cases.

Checklists

  • I am okay with my commits getting squashed when you merge this PR.
  • I am familiar with the contributing guidelines.

Please cherry-pick my commits into:

  • Foreman 3.11/Katello 4.13
  • Foreman 3.10/Katello 4.12
  • Foreman 3.9/Katello 4.11 (Satellite 6.15; orcharhino 6.8/6.9)
  • Foreman 3.8/Katello 4.10
  • Foreman 3.7/Katello 4.9 (Satellite 6.14)
  • Foreman 3.6/Katello 4.8
  • Foreman 3.5/Katello 4.7 (Satellite 6.13; orcharhino 6.6/6.7)
  • We do not accept PRs for Foreman older than 3.5.

@asteflova asteflova force-pushed the SAT-22855_ad_gss-proxy branch 2 times, most recently from 737203b to 468d121 Compare July 18, 2024 17:54
Copy link
Member Author

@asteflova asteflova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I'm not the original author of these changes, I'm adding a few notes that I made while familiarizing myself with the bug report. Feel free to ignore them.

@pr-processor pr-processor bot added Waiting on contributor Requires an action from the author Needs re-review and removed Not yet reviewed Waiting on contributor Requires an action from the author labels Jul 19, 2024
@lhellebr
Copy link

I've commented on some issues based on my memory and I will proceed with testing this actual workflow documented, verbatim, without changes I've suggested.

@asteflova
Copy link
Member Author

I believe I have resolved all comments you've posted so far @lhellebr Let me know what else needs changing. Thanks!

@lhellebr
Copy link

Tested with the latest version and when running the installer, I get the following error.

2024-07-24 12:18:57 [NOTICE] [configure] System configuration has finished.

Error 1: Puppet Exec resource 'ipa-getkeytab' failed. Logs:
  /Stage[main]/Foreman::Config/Exec[ipa-getkeytab]/before
    before to File[/etc/httpd/conf/http.keytab]
  /Stage[main]/Foreman::Config/Exec[ipa-getkeytab]
    Starting to evaluate the resource (273 of 1654)
    Failed to call refresh: '/bin/echo Get keytab           && KRB5CCNAME=KEYRING:session:get-http-service-keytab kinit -k           && KRB5CCNAME=KEYRING:session:get-http-service-keytab /usr/sbin/ipa-getkeytab -k /etc/httpd/conf/http.keytab -p HTTP/<FQDN>           && kdestroy -c KEYRING:session:get-http-service-keytab' returned 1 instead of one of [0]
    '/bin/echo Get keytab           && KRB5CCNAME=KEYRING:session:get-http-service-keytab kinit -k           && KRB5CCNAME=KEYRING:session:get-http-service-keytab /usr/sbin/ipa-getkeytab -k /etc/httpd/conf/http.keytab -p HTTP/<FQDN>           && kdestroy -c KEYRING:session:get-http-service-keytab' returned 1 instead of one of [0]
    Evaluated in 0.04 seconds
  /Stage[main]/Foreman::Config/Exec[ipa-getkeytab]/creates
    Checking that 'creates' path '/etc/httpd/conf/http.keytab' exists
    Checking that 'creates' path '/etc/httpd/conf/http.keytab' exists
  Exec[ipa-getkeytab](provider=posix)
    Executing '/bin/echo Get keytab           && KRB5CCNAME=KEYRING:session:get-http-service-keytab kinit -k           && KRB5CCNAME=KEYRING:session:get-http-service-keytab /usr/sbin/ipa-getkeytab -k /etc/httpd/conf/http.keytab -p HTTP/<FQDN>           && kdestroy -c KEYRING:session:get-http-service-keytab'
    Executing '/bin/echo Get keytab           && KRB5CCNAME=KEYRING:session:get-http-service-keytab kinit -k           && KRB5CCNAME=KEYRING:session:get-http-service-keytab /usr/sbin/ipa-getkeytab -k /etc/httpd/conf/http.keytab -p HTTP/<FQDN>           && kdestroy -c KEYRING:session:get-http-service-keytab'
  /Stage[main]/Foreman::Config/Exec[ipa-getkeytab]/returns
    Get keytab
    kinit: Cannot determine realm for host (principal host/<FQDN>@)
    change from 'notrun' to ['0'] failed: '/bin/echo Get keytab           && KRB5CCNAME=KEYRING:session:get-http-service-keytab kinit -k           && KRB5CCNAME=KEYRING:session:get-http-service-keytab /usr/sbin/ipa-getkeytab -k /etc/httpd/conf/http.keytab -p HTTP/<FQDN>           && kdestroy -c KEYRING:session:get-http-service-keytab' returned 1 instead of one of [0]
    Get keytab
    kinit: Cannot determine realm for host (principal host/<FQDN>@)

1 error was detected during installation.
Please address the errors and re-run the installer to ensure the system is properly configured.
Failing to do so is likely to result in broken functionality.

Note that the realm (In this log, <FQDN> is doctored to make it public) is derived from the host's FQDN which is different to the realm. Normally, the realm is taken from the config files created as per docs but here, the installer doesn't use it.
My hypothesis is it's because the installer expects the keytab to be in a certain place (that changed) but I can't confirm it yet.

Additionally, in part 5.8.3 step 8a, it's unclear which section of the file the text should be added to.

@asteflova
Copy link
Member Author

My hypothesis is it's because the installer expects the keytab to be in a certain place (that changed) but I can't confirm it yet.

I don't think I can help with that but let me know if I'm missing something and there is anything I'm expected to do about this.

Additionally, in part 5.8.3 step 8a, it's unclear which section of the file the text should be added to.

I clarified that the lines go to the domain section for the AD domain. And I also added a reference to the sssd-ad man page (there are multiple man pages related to sssd.conf so it might be helpful to point users to the most relevant one).

@lhellebr
Copy link

My hypothesis is it's because the installer expects the keytab to be in a certain place (that changed) but I can't confirm it yet.

I don't think I can help with that but let me know if I'm missing something and there is anything I'm expected to do about this.

It means what we agreed to do here #3149 (comment) and what was suggested in the ticket was kinda illegal. Not by the nature of the change, after all, we followed the KB... but it seems the satellite EXPECTS the keytab to be in /etc/httpd/conf/http.keytab. It can be fixed either by:

  1. Devels changing this expectation. This would be justifiable but IMO not necessary. CC @adamruzicka
  2. You just changing it back to the original path. Effectively negating what is perhaps the main point of the whole ticket - but solving the same problem in a different manner.

Anyway, this must be fixed, it's a blocker, the process doesn't work as is.

@lhellebr
Copy link

When exactly following the docs in this PR's current state but changing /etc/gssapi/http.conf to /etc/httpd/conf/http.keytab, I got a working configuration.

@adamruzicka
Copy link
Contributor

but solving the same problem in a different manner.

So if we kept the old path (/etc/httpd/conf/http.keytab), then this would reduce the changes here to:

  • setting a different selinux label on it
  • setting a different user group on it

If this is enough to resolve the issue then I'd prefer it over changing the puppet modules to expect the keytab to be in a different place, especially if we want to have this done soon.

@ekohl
Copy link
Member

ekohl commented Jul 25, 2024

If this is enough to resolve the issue then I'd prefer it over changing the puppet modules to expect the keytab to be in a different place, especially if we want to have this done soon.

You can use --foreman-http-keytab /path/to/keytab. It just defaults to ${apache::conf_dir}/http.keytab.

I'm not sure about the SELinux policy, because that comes from the base SELinux policy. Looks like the default is /etc/httpd/conf/keytab which we never used.

# semanage fcontext -l | grep keytab
/etc/httpd/conf/keytab                             regular file       system_u:object_r:httpd_keytab_t:s0 
/etc/krb5\.keytab                                  all files          system_u:object_r:krb5_keytab_t:s0 
/etc/krb5kdc/kadm5\.keytab                         regular file       system_u:object_r:krb5_keytab_t:s0 
/var/kerberos/krb5(/.*)?                           all files          system_u:object_r:krb5_keytab_t:s0 
/var/kerberos/krb5kdc/kadm5\.keytab                regular file       system_u:object_r:krb5_keytab_t:s0 

Looking at the procedure it states The Apache user must not have access to the keytab file. so then it is indeed very illogical to use the Apache config directory.

@asteflova
Copy link
Member Author

If possible, I'd like to limit this PR to addressing https://issues.redhat.com/browse/SAT-22855 in a way that enables us to close that ticket. That would involve:

  • Reviewing the keytab's location and permissions (ongoing conversation)
  • A decision on whether to include a step for SPN in AD (resolved)
  • Adding oddjob* packages (resolved)

The last time I tried to look at the whole AD/SSSD integration procedure from a bigger perspective, it resulted in #3056 which became unmanageable and I had to close it. Because it's absolutely true that there are larger issues to deal with. Can we track them in https://issues.redhat.com/browse/SAT-26039 in order to avoid blocking this PR?

asteflova and others added 4 commits September 19, 2024 11:14
/etc/samba/smb.conf is already present by default, it's better to use
that rather than create a separate configuration file for this
@asteflova
Copy link
Member Author

@mmuehlfeldRH: I addressed your suggestions and resolved the related comment threads.

@maximiliankolb: I addressed the one breaking change you found (installing packages) and resolved the other comments.

@ekohl: I believe I addressed your request for changes as well by dropping the steps related to privilege separation for Apache and verifying that the remaining steps work.

@asteflova asteflova added the style review done No issues from docs style/grammar perspective label Sep 19, 2024
@asteflova
Copy link
Member Author

@adamruzicka and/or @lhellebr: Is the latest state something you'd be comfortable approving?

@adamruzicka
Copy link
Contributor

I've tested this prior to the samba changes and back then it worked well. With the samba changes it reads well, so +1 from me, even though I didn't have a chance to test it out end to end.

@asteflova
Copy link
Member Author

Thanks for confirming @adamruzicka! With my testing, I believe this covers tech review.

@asteflova asteflova added tech review done No issues from the technical perspective and removed Needs tech review Requires a review from the technical perspective labels Sep 26, 2024
@asteflova
Copy link
Member Author

There are still pending change requests but I checked them and they are either resolved or I have provided an explanation. Comment threads have been resolved. I'm going to go ahead with the merge now. Thanks everyone who provided their input!

@asteflova asteflova merged commit 01add9f into theforeman:master Sep 26, 2024
9 checks passed
asteflova added a commit that referenced this pull request Sep 26, 2024
* Drop steps related to GSS proxy from Samba-based joining
* List login methods for AD users with direct integration
* Use smb.conf to store settings for interacting with AD
* Further edits for readability and based on peer review

---------

Co-authored-by: alazik <[email protected]>
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
Co-authored-by: Maximilian Kolb <[email protected]>
Co-authored-by: mmuehlfeldRH <[email protected]>
(cherry picked from commit 01add9f)
asteflova added a commit that referenced this pull request Sep 26, 2024
* Drop steps related to GSS proxy from Samba-based joining
* List login methods for AD users with direct integration
* Use smb.conf to store settings for interacting with AD
* Further edits for readability and based on peer review

---------

Co-authored-by: alazik <[email protected]>
Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
Co-authored-by: Maximilian Kolb <[email protected]>
Co-authored-by: mmuehlfeldRH <[email protected]>
@asteflova
Copy link
Member Author

asteflova commented Sep 26, 2024

Merged to "master" and cherry-picked:

1d6a8d3..3d5e5d4 3.12 -> 3.12
0d40788..0ddfeaa 3.11 -> 3.11

Conflicts on 3.10 and below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
style review done No issues from docs style/grammar perspective tech review done No issues from the technical perspective
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants