-
Notifications
You must be signed in to change notification settings - Fork 108
Update Kerberos SSO documentation for foremanctl #4543
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
base: master
Are you sure you want to change the base?
Update Kerberos SSO documentation for foremanctl #4543
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates Kerberos SSO and FreeIPA authentication documentation to incorporate foremanctl workflows, including enrollment, login, and recovery steps, and aligns the Configuring User Authentication guide and nightly metadata with the new content. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- In the new and updated Kerberos/FreeIPA modules, double-check that references to
foremanctlvs.hammerare consistently scoped (for example, in titles, prerequisites, and command snippets) so users can easily tell which tool a given step applies to. - Since you added
proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc, consider linking to this procedure from the main Kerberos SSO configuration assembly and any troubleshooting sections so users can discover the reset workflow when they run into misconfiguration issues. - For the
nightly.jsonchanges, verify that the new or updated entries for the Configuring User Authentication and Kerberos/FreeIPA docs use consistent slugs/URLs and titles with the correspondingmaster.adocfiles to avoid broken or duplicated navigation items.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new and updated Kerberos/FreeIPA modules, double-check that references to `foremanctl` vs. `hammer` are consistently scoped (for example, in titles, prerequisites, and command snippets) so users can easily tell which tool a given step applies to.
- Since you added `proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc`, consider linking to this procedure from the main Kerberos SSO configuration assembly and any troubleshooting sections so users can discover the reset workflow when they run into misconfiguration issues.
- For the `nightly.json` changes, verify that the new or updated entries for the Configuring User Authentication and Kerberos/FreeIPA docs use consistent slugs/URLs and titles with the corresponding `master.adoc` files to avoid broken or duplicated navigation items.
## Individual Comments
### Comment 1
<location> `guides/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc:17` </location>
<code_context>
+
[options="nowrap", subs="+quotes,verbatim,attributes"]
----
+ifdef::foremanctl[]
+# foremanctl deploy --pam-service foreman-prod
+endif::[]
</code_context>
<issue_to_address>
**issue (review_instructions):** The .Verification section for the foremanctl variant is just a placeholder and does not provide actual verification steps, so the verification instruction is not fulfilled.
The non-foremanctl path includes a concrete verification step (`--help | grep foreman-ipa-authentication` and expected values), but the foremanctl path only has the placeholder `#How can users verify this?#`.
Please add explicit verification instructions for the foremanctl scenario (for example, a command or UI check that confirms external authentication has been reset and what output or behavior to expect), or otherwise align it with the level of verification provided for the installer-based variant.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/modules/proc*.adoc`
**Instructions:**
- Verification steps are included where appropriate.
</details>
</issue_to_address>
### Comment 2
<location> `guides/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc:13` </location>
<code_context>
+[IMPORTANT]
+====
+Resetting external authentication prevents users from accessing {Project} as described in xref:configuring-kerberos-sso-with-{FreeIPA-context}-in-{project-context}[] and xref:configuring-kerberos-sso-for-active-directory-users-in-project_{context}[].
+However, some configuration files, such as configuration files for the System Security Services Daemon (SSSD), will remain modified because {Project} does not have access to the previous state of these files.
+====
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The phrase "will remain modified" uses passive voice and could be rephrased to an active construction to meet the style requirement.
Consider rephrasing to avoid the passive construction, for example:
"However, some configuration files, such as those for the System Security Services Daemon (SSSD), stay in their modified state because {Project} does not have access to their previous state."
This keeps the meaning while avoiding passive voice.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `guides/common/*.adoc,guides/common/modules/*.adoc`
**Instructions:**
Avoid passive voice in verbs.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...es/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc
Show resolved
Hide resolved
|
These preview links are relevant for tech review: Configuring User Authentication, update to contain only content that's expected to work with foremanctl:
Configuring User Authentication, based on the current installer (section 12. Resetting external authentication configuration for Kerberos SSO is mostly new and needs a review): |
ff45f7c to
b752f34
Compare
ekohl
left a comment
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.
Not a complete read through but some things that jumped out to me.
Can we define {project-package-install} as just {package-install} for foremanctl? We already do that for everything except the satellite builds everywhere else and keeps the changes smaller.
...on/modules/proc_configuring-the-active-directory-authentication-source-on-projectserver.adoc
Outdated
Show resolved
Hide resolved
adamruzicka
left a comment
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.
It reads well, but the verification part for disabling with foremanctl needs to be flushed out
...ules/proc_configuring-host-based-access-control-for-freeipa-users-logging-in-to-project.adoc
Outdated
Show resolved
Hide resolved
...es/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc
Show resolved
Hide resolved
| * Username and password | ||
| * Kerberos single sign-on | ||
|
|
||
| ifndef::foremanctl[] |
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.
Does this mean that these are not available with foremanctl? I think that's not true.
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.
I excluded these lines (for now) because the NOTE leads to an LDAP procedure which uses foreman-maintain. The maintain tool will be replaced by foremanctl, which means the procedure cannot yet be included in a foremanctl guide. We can include it after we know what the replacement for foreman-maintain service restart will look like and that the procedure works in the foremanctl world.
The other link leads to an AD procedure, which is being exposed in this PR. However, modifying the NOTE to include two links for a pre-foremanctl version of the guide and one link for the post-foremanctl version would take some rewriting. I don't think it's worth it right now -- we can just include the whole NOTE again after we verify that the LDAP procedure is safe for foremanctl.
guides/common/modules/proc_enrolling-projectserver-in-your-freeipa-domain.adoc
Outdated
Show resolved
Hide resolved
guides/common/modules/proc_logging-in-to-hammer-cli-with-freeipa-credentials.adoc
Show resolved
Hide resolved
b0073ce to
b4e1a67
Compare
A few threads are still open (for visibility due to open questions not related to the documentation update or just interesting information being shared) but other than that, I think I've implemented all the feedback so far. Which means these guides are ready for re-review. |
...ules/proc_configuring-host-based-access-control-for-freeipa-users-logging-in-to-project.adoc
Show resolved
Hide resolved
...ules/proc_configuring-host-based-access-control-for-freeipa-users-logging-in-to-project.adoc
Outdated
Show resolved
Hide resolved
...on/modules/proc_configuring-the-active-directory-authentication-source-on-projectserver.adoc
Outdated
Show resolved
Hide resolved
...es/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc
Outdated
Show resolved
Hide resolved
ce28e1d to
34dcea9
Compare
|
Hello @adamruzicka @lhellebr @maximiliankolb, do you have any further comments or suggestions? If not, can you please consider acking this PR? |
adamruzicka
left a comment
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.
One pedantic comment, otherwise it reads well
...ules/proc_configuring-host-based-access-control-for-freeipa-users-logging-in-to-project.adoc
Outdated
Show resolved
Hide resolved
adamruzicka
left a comment
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.
Reads well
guides/common/modules/proc_configuring-hammer-cli-to-accept-freeipa-credentials.adoc
Outdated
Show resolved
Hide resolved
|
One clarifying comment, otherwise LGTM |
maximiliankolb
left a comment
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.
A few minor suggestions on style. I cannot comment on the tech at all.
...ules/proc_configuring-host-based-access-control-for-freeipa-users-logging-in-to-project.adoc
Outdated
Show resolved
Hide resolved
...es/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc
Outdated
Show resolved
Hide resolved
...es/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc
Outdated
Show resolved
Hide resolved
...es/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc
Outdated
Show resolved
Hide resolved
Lennonka
left a comment
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.
Just a few nitpicks.
guides/common/modules/proc_configuring-hammer-cli-to-accept-freeipa-credentials.adoc
Outdated
Show resolved
Hide resolved
...es/common/modules/proc_resetting-external-authentication-configuration-for-kerberos-sso.adoc
Outdated
Show resolved
Hide resolved
871a886 to
cba9b96
Compare
|
All suggestions have been applied, @maximiliankolb and/or @Lennonka, can you please re-review? |
maximiliankolb
left a comment
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.
LGTM style-wise.
Lennonka
left a comment
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.
LGTM, thanks!
Update IPA Kerberos auth source for foremanctl Update AD external auth source for foremanctl Update procedure on resetting external auth Move reset to its own module Update ext auth reset with additional details Drop equal signs Define project-package-install for foremanctl Clarify auth sources affected by auth config reset Update PAM service option Add ext auth overview to foremanctl Configuring User Auth Clarify that auth source are mutually exclusive Add verification step for resetting Kerberos SSO Apply suggestions from style review Co-authored-by: Maximilian Kolb <[email protected]> Call out a f-ctl prereq explicitly Ensure standalone Hammer scenario is covered Drop an internal comment about why a prereq is here Reference --add-feature hammer Apply easy fixes from style review Co-authored-by: Lena Ansorgová <[email protected]> Co-authored-by: Maximilian Kolb <[email protected]> Move stdout outside of a command block
cba9b96 to
810954c
Compare
|
Squashing some commits so that we can add two separate commits to master when merging. No changes to the diff. |
What changes are you introducing?
Updating documentation on enabling FreeIPA-based authentication for foremanctl. This also includes publishing a foremanctl-flavored Configuring User Authentication guide.
Why are you introducing these changes? (Explanation, links to references, issues, etc.)
theforeman/foremanctl#312
Anything else to add? (Considerations, potential downsides, alternative solutions you have explored, etc.)
N/A
Contributor checklists
Please cherry-pick my commits into:
Summary by Sourcery
Update Kerberos SSO and external authentication documentation to cover foremanctl and related workflows.
Documentation: