Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRepository branding and docs renamed from "Puppet Modules" to "OpenVox Modules"; dev tooling URLs updated. domain_join gains OIDC support (new params, optional username/password), conditional SSSD package/template selection, guarded join/smartcard logic, and a new SSSD OIDC ERB template. Changes
Sequence Diagram(s)sequenceDiagram
participant Puppet as Puppet Agent
participant FS as Filesystem
participant SSSD as sssd Service
participant IdP as OIDC Provider
participant AD as Active Directory (adcli)
Puppet->>FS: Render sssd.conf (select template based on $oidc)
alt OIDC = true
Puppet->>FS: Write sssd.oidc.conf (client_id, optional client_secret, tenant endpoints)
Puppet->>SSSD: Install `sssd-idp`
Puppet->>SSSD: Start/Restart sssd
SSSD->>IdP: Token / userinfo requests (OIDC flows)
else OIDC = false
Puppet->>FS: Write sssd.conf
Puppet->>AD: Run `adcli join` (Exec 'Join')
Puppet->>SSSD: Install `sssd`
Puppet->>SSSD: Start/Restart sssd
SSSD->>AD: AD/LDAP identity lookups
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
domain_join/manifests/init.pp (2)
343-357:⚠️ Potential issue | 🔴 CriticalCatalog compilation failure:
Exec['Enable SSSD Authentication']subscribes toExec['Join'], which is not declared when$oidcis true.When
$oidcis true, theExec['Join']resource (line 200) is never declared because it's inside theunless $oidcblock (line 189). However,Exec['Enable SSSD Authentication']unconditionally subscribes toExec['Join']on line 346. This will cause a catalog compilation error referencing a non-existent resource.Proposed fix — conditionally subscribe
exec { 'Enable SSSD Authentication': command => "${enablesssd} ${enable_smartcard}", - subscribe => [ - Exec['Join'], - ], path => $facts['path'], refreshonly => true, require => [ Package['authconfig'], Package['oddjob-mkhomedir'], Service['oddjobd'], Package['sssd'], Service['sssd'], ], }Then, inside the
unless $oidcblock, add notification fromExec['Join']:exec { 'Join': command => $command, path => $facts['path'], - notify => Service['sssd'], + notify => [Service['sssd'], Exec['Enable SSSD Authentication']], creates => '/etc/krb5.keytab', ... }Alternatively, you could wrap the subscribe in a conditional or use a collector pattern, depending on how you want the OIDC path to trigger
Enable SSSD Authentication.
54-78:⚠️ Potential issue | 🟠 Major
$usernameand$sensitive_passwordlack default values, making them still required even for OIDC users.Although the types were changed to
Optional[...], parameters without defaults are still required by the Puppet/OpenVox compiler. OIDC users who don't need AD credentials would have to explicitly passundeffor these. Consider adding= undefdefaults and adding validation for the non-OIDC path.Proposed fix
class domain_join ( - Optional[String] $username, - Optional[Sensitive[String]] $sensitive_password, + Optional[String] $username = undef, + Optional[Sensitive[String]] $sensitive_password = undef, ... ) {Then add validation near the top of the class body:
if $oidc { if !$client_id or !$tenant_id { fail('client_id and tenant_id are required when oidc is true') } } else { if !$username or !$sensitive_password { fail('username and sensitive_password are required when oidc is false') } }
🤖 Fix all issues with AI agents
In `@domain_join/manifests/init.pp`:
- Around line 189-214: The non-OIDC branch interpolates
$sensitive_password.unwrap and $username directly which will crash if those
Optional values are undef; update the code around the exec 'Join' command
generation to guard usage with defined() checks (e.g. if
defined($sensitive_password) and defined($username)) before calling
$sensitive_password.unwrap or interpolating $username, and handle the else case
by either generating a safe no-op/meaningful error message or skipping the exec
resource; specifically modify the blocks that set $command (where
$sensitive_password.unwrap and ${username} are used) to only build the sensitive
command when those variables are defined.
- Around line 154-163: In the conditional package resource for 'sssd' driven by
$oidc, change the package name attribute from name => 'sssd-oidc' to name =>
'sssd-idp' so the correct SSSD OIDC/IdP package is installed; update the package
resource inside the if $oidc { ... } block (the resource declared as package {
'sssd': ... } ) to use 'sssd-idp' as the name.
In `@domain_join/README.md`:
- Around line 213-219: Update the README entry for the client_secret attribute
to reflect the manifest change: change the documented data type from
Optional[String] to Optional[Sensitive[String]] for client_secret (and keep the
same default/description unless the manifest changes it); ensure the README line
referencing `client_secret` uses the exact symbol name `client_secret` and
clearly shows the new data type `Optional[Sensitive[String]]` so docs match the
manifest change.
In `@domain_join/templates/sssd.oidc.conf.erb`:
- Around line 1-20: When OIDC is enabled the template uses `@client_id` and
`@tenant_id` but there is no guard, so rendering with oidc true and missing
client_id/tenant_id produces invalid SSSD config; add a validation in the
manifest that checks the OIDC flag and fails fast if required parameters are
missing (e.g., if $oidc or $enable_oidc is true and either $client_id or
$tenant_id is undef/empty then call fail("...")). Reference the manifest
variables ($oidc, $client_id, $tenant_id) and use the Puppet fail() function to
provide a clear error message instructing the user to supply both values.
- Line 20: The sssd OIDC template currently hardcodes fallback_homedir to
"/home/%u" instead of using the template variable; update the sssd.oidc.conf.erb
template to interpolate the `@sssd_home` template variable for the home base
(preserving the "%u" username placeholder) so it honors the same sssd_home
parameter as the non-OIDC template and handle potential trailing slash in
`@sssd_home` when composing the final path.
- Around line 8-10: Change the Puppet type for client_secret from
Optional[String] to Optional[Sensitive[String]] in the resource/class that
provides `@client_secret`, and update the ERB template sssd.oidc.conf.erb to call
.unwrap when inserting the secret (i.e., use `@client_secret.unwrap` when present)
so the sensitive value is stored as Sensitive in the catalog and only unwrapped
at render time; ensure code paths that pass $client_secret into the template
accept Optional[Sensitive[String]] and handle the nil/absent case the same way
as before.
🧹 Nitpick comments (1)
domain_join/manifests/init.pp (1)
154-158: Nit: attribute ordering —ensureshould come beforenameper Puppet style guide.Proposed fix
package { 'sssd': - name => 'sssd-oidc', ensure => installed, + name => 'sssd-oidc', }
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
domain_join/manifests/init.pp (2)
54-78:⚠️ Potential issue | 🔴 CriticalMissing parameter validation: OIDC and non-OIDC paths require different mandatory parameters.
With
$usernameand$sensitive_passwordnowOptional, and the new OIDC parameters alsoOptional, there are no guards ensuring the right set is provided for each mode:
$oidc = false:$usernameand$sensitive_passwordare interpolated into theadcli joincommand (lines 192/196). If either isundef,$sensitive_password.unwrapwill fail at catalog compilation.$oidc = true:$client_idand$tenant_idare presumably consumed by thesssd.oidc.conf.erbtemplate (line 235). If they'reundef, the rendered config will be broken.Add early validation at the top of the class body, e.g.:
Proposed fix
) { + if $oidc { + if $client_id == undef { + fail('client_id is required when oidc is true') + } + if $tenant_id == undef { + fail('tenant_id is required when oidc is true') + } + } else { + if $username == undef { + fail('username is required when oidc is false') + } + if $sensitive_password == undef { + fail('sensitive_password is required when oidc is false') + } + } + if $override_domain {
343-357:⚠️ Potential issue | 🔴 CriticalCatalog compilation will fail when
$oidc = true:Exec['Join']doesn't exist.
Exec['Enable SSSD Authentication']subscribes toExec['Join'](line 346), butExec['Join']is only declared inside theunless $oidcblock (line 200). When$oidc = true, the resource is absent from the catalog and Puppet will fail with a compilation error referencing a nonexistent resource.Additionally, because of
refreshonly => true, this exec will never trigger in OIDC mode even if the reference is fixed — SSSD authentication enablement (authselect/authconfig) would be skipped entirely. Consider whether this exec should also run in the OIDC path (perhaps triggered by a different event).Proposed fix — conditionally subscribe
exec { 'Enable SSSD Authentication': command => "${enablesssd} ${enable_smartcard}", - subscribe => [ - Exec['Join'], - ], path => $facts['path'], - refreshonly => true, require => [ Package['authconfig'], Package['oddjob-mkhomedir'], Service['oddjobd'], Package['sssd'], Service['sssd'], ], + subscribe => $oidc ? { + true => undef, + false => [Exec['Join']], + }, + refreshonly => !$oidc, }If the exec should only run once and not on every Puppet run in OIDC mode, consider adding a
createsorunlessguard instead.
🧹 Nitpick comments (3)
domain_join/manifests/init.pp (3)
321-341: Silently ignoring$smartcardwhen$oidcis true.When
$oidc = true, the$smartcardparameter is silently ignored and$enable_smartcardis set to''. If a user explicitly sets$smartcard => 'required'alongside$oidc => true, there's no feedback that their smartcard configuration is being disregarded. Consider emitting a warning:if $oidc and $smartcard != 'disabled' { warning('Smartcard settings are ignored when OIDC is enabled') }
121-153: Consider: unnecessary packages installed in OIDC mode.
adcli,samba-common-tools, andsamba-commonare always installed, even when$oidc = trueand noadcli joinwill occur. This isn't harmful, but wrapping them inunless $oidc(similar to the join exec) would keep the OIDC footprint minimal.
46-53: Documentation forclient_secretcould clarify its purpose.The
@param client_secretdoc says "Optional secret for client" which is vague. It would help to specify when this is needed (e.g., for confidential OIDC clients) and what happens if it's omitted (e.g., public client / device-code flow).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
domain_join/manifests/init.pp (1)
364-375:⚠️ Potential issue | 🟠 MajorOIDC mode:
Exec['Enable SSSD Authentication']is never triggered.This exec is
refreshonly => trueand is only notified byExec['Join'](line 221-224), which is skipped entirely when$oidcistrue. This means on a fresh OIDC deployment,authselect/pam-auth-updateis never run, so SSSD authentication won't actually be enabled in PAM.Consider adding a notification path for the OIDC case — for example, having
File['/etc/sssd/sssd.conf']orPackage['sssd']notify this exec, or creating a separate trigger for OIDC mode.
🤖 Fix all issues with AI agents
In `@domain_join/manifests/init.pp`:
- Around line 90-97: The validation block is using bare words so the checks
never trigger; change the checks to reference the Puppet variables with sigils
(use !$username and !$sensitive_password) so undef values cause the fail() to
run, and correct the typo in the fail messages ("disbled" -> "disabled"); this
ensures missing values are caught before the Exec['Join'] resource attempts to
.unwrap on undef.
- Around line 84-86: The current validation unconditionally fails when oidc is
enabled if client_secret is undef, but the parameter is declared
Optional[Sensitive[String]] and templates only emit idp_client_secret when
present; either remove the failing check around client_secret (delete the
fail('domain_join: client_secret must be provided when oidc is enabled') block)
so public-client OIDC flows remain supported, or make client_secret truly
required by changing its type from Optional[Sensitive[String]] to
Sensitive[String], update its documentation string to reflect "Required secret
for client" and adjust any templates/conditionals (e.g., idp_client_secret
emission) and callers to always provide client_secret; update references to
client_secret and oidc accordingly.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
domain_join/manifests/init.pp (1)
361-372:⚠️ Potential issue | 🔴 CriticalCritical:
Exec['Enable SSSD Authentication']is never triggered in the OIDC path.This exec has
refreshonly => true, so it only runs when notified. The only notifier isExec['Join'](line 220), which is not declared when$oidc = true(gated at line 204). As a result,authselect/pam-auth-updatenever runs in the OIDC flow, leaving PAM unconfigured for SSSD — users will be unable to authenticate.One approach: add an
unlessguard to make this idempotent instead of relying solely onrefreshonly. For example, on RHEL 8+/Fedora, check ifsssdis already the active authselect profile:🐛 Proposed fix
exec { 'Enable SSSD Authentication': command => "${enablesssd} ${enable_smartcard}", path => $facts['path'], - refreshonly => true, + unless => 'authselect current | grep -q sssd', require => [ Package['authconfig'], Package['oddjob-mkhomedir'],Note: the
unlessguard above only covers theauthselectcode path (RHEL 8+/Fedora 37+). For Debian (pam-auth-update) and RHEL 7 (authconfig), you'll need an equivalent idempotency check, or alternatively keeprefreshonlybut ensure something notifies this exec in the OIDC path (e.g.,File['/etc/sssd/sssd.conf']orPackage['sssd']).
🧹 Nitpick comments (2)
domain_join/manifests/init.pp (2)
169-178: Nit: Puppet style guide recommendsensureas the first attribute.Lines 171–172 place
namebeforeensure. The Puppet style guide convention is to listensurefirst for readability and consistency (as done in line 176).♻️ Suggested reorder
if $oidc { package { 'sssd': - name => 'sssd-idp', ensure => installed, + name => 'sssd-idp', }
339-359: Consider warning when$smartcardis not'disabled'but$oidcistrue.Currently, if both
$oidc = trueand$smartcard != 'disabled'are set, the smartcard setting is silently ignored. Awarning()call would help operators notice the misconfiguration.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@domain_join/manifests/init.pp`:
- Line 264: The notify array has extra spaces inside the square brackets causing
puppet-lint to fail; edit the notify attribute for the resource that currently
reads notify => [ Service['sssd'], Exec['Enable SSSD Authentication'] ], and
remove the space after the opening '[' and before the closing ']' so the bracket
usage is tightly packed (i.e. notify => [Service['sssd'], Exec['Enable SSSD
Authentication']]); keep the same elements Service['sssd'] and Exec['Enable SSSD
Authentication'] unchanged.
🧹 Nitpick comments (2)
domain_join/spec/classes/domain_join_spec.rb (1)
6-24: Consider adding test coverage for the new OIDC code path.The existing test only exercises the non-OIDC path (
oidc => falseby default). The new OIDC feature introduces significant conditional logic (validation, package selection, template selection, join suppression). Adding at least one context withoidc => true(and the requiredclient_id/tenant_id) would guard against regressions.domain_join/manifests/init.pp (1)
339-359: Smartcard settings are silently ignored whenoidcistrue.If a user sets both
oidc => trueandsmartcard => 'required', the smartcard configuration is silently dropped. Consider emitting a warning (viawarning()) or adding a validationfail()if$smartcard != 'disabled'when$oidcis true, so users aren't surprised by the behavior.
1 participant
This also updates some of the references to Puppet to OpenVox.
Summary by CodeRabbit
New Features
Documentation
Tests