Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 23384050 | Triggered | Generic Password | 0f8a96d | manager/dp.patch | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
WalkthroughAdds PAM Access and OpenID Connect Device Authorization configuration and UI entries, translation keys, and applies a new patch (dp.patch) in Dockerfile build steps for full, manager, portal, and uwsgi-portal images. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 1
🧹 Nitpick comments (3)
full/dp.patch (1)
419-424: Minor: Trailing newline removed in fr.json.The French translation file has its trailing newline removed (closing brace directly after last entry). This is cosmetic but may cause issues with some JSON tools that expect a trailing newline.
Changes.md (1)
3-6: Consider expanding changelog entry to cover both features.The current entry "Add device-pam" only mentions one aspect. This PR also adds OIDC Device Authorization (RFC 8628) support.
Consider:
## v2.21.3-3 _(dev)_ * Fix updateConf script when value is multiline * Improve Patroni patch -* Add device-pam +* Add PAM Access token support +* Add OIDC Device Authorization Grant (RFC 8628)manager/dp.patch (1)
167-1054: Translation additions are well-structured.The localization changes add consistent keys across all 13 language files. The English translations are complete, and the French translations include proper PAM Access localizations.
Consider completing translations for the device authorization keys in non-English locales (currently many use English text as placeholders). This is optional and could be done as a follow-up if native translations are available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Changes.md(1 hunks)full/Dockerfile(1 hunks)full/dp.patch(1 hunks)manager/Dockerfile(1 hunks)manager/dp.patch(1 hunks)portal/Dockerfile(1 hunks)transform(1 hunks)uwsgi-portal/Dockerfile(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Hadolint
portal/Dockerfile
[warning] 2-2: Hadolint: Always tag the version of an image explicitly (DL3006).
[warning] 11-11: Hadolint: Pin versions in apt get install. Instead of apt-get install <package> use apt-get install <package>=<version> (DL3008).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (17)
transform (1)
10-32: Confirm that ignoringCommon/Conf/DefaultValues.pmis intended long‑termAdding
Common/Conf/DefaultValues\.pmto@ignoreis consistent with the otherCommon/Conf/*entries and the transform logic remains correct. Just make sure we never rely on downstream patches/diffs for this file (e.g., custom default values for new PAM/OIDC options), since they will now be silently filtered out by this script.full/dp.patch (3)
7-54: PAM Access attributes look well-structured.The configuration attributes for PAM Access are properly defined with appropriate types, defaults, and validation. The
keyTestregex for server group names appropriately restricts to safe characters.Consider documenting or enforcing elsewhere that
pamAccessMaxDurationshould be >=pamAccessTokenDurationto prevent misconfiguration.
62-104: OIDC Device Authorization attributes are RFC 8628 compliant.The defaults align well with RFC 8628 recommendations:
- 5-second polling interval (minimum recommended by spec)
- 10-minute device code expiration (reasonable user experience)
- 8-character user code (good balance of entropy and usability)
All features are disabled by default, following secure-by-default principles.
108-166: UI tree structure follows existing patterns.The CTrees.pm and Tree.pm modifications correctly integrate the new PAM Access and Device Authorization features into the manager UI. Help links point to appropriate documentation sections.
manager/Dockerfile (1)
30-42: Patch integration looks correct.The
dp.patchis properly added to the patch loop withset -eensuring build failure if the patch doesn't apply cleanly. The subsequentManager::Build->run()call will regenerate configuration files to incorporate the new PAM Access and Device Authorization attributes.uwsgi-portal/Dockerfile (1)
35-45: Patch integration consistent with other Dockerfiles.The
dp.patchis correctly added to the uwsgi-portal patch sequence, maintaining consistency with the manager and full Dockerfiles.full/Dockerfile (2)
23-35: Patch integration matches manager/Dockerfile.The
dp.patchinclusion and subsequent build regeneration are consistent with the manager Dockerfile, which is expected for the full (combined) image.
34-34: No action needed.portal/Dockerfilealready includesdp.patchat line 46, ensuring consistency across Dockerfiles.manager/dp.patch (9)
62-66: LGTM!The
portalDisplayPamAccessattribute follows the established pattern for portal display toggles with a conservative default of disabled.
99-104: LGTM!The
oidcRPMetaDataOptionsAllowDeviceAuthorizationGrantattribute follows the established pattern for per-RP grant type toggles with a secure default of disabled.
108-117: LGTM!The addition of
oidcRPMetaDataOptionsAllowDeviceAuthorizationGrantto the CTrees list is correctly placed among other grant type options.
118-127: LGTM!The
portalDisplayPamAccessaddition to the portal display options tree is properly positioned.
224-227: JSON key reordering observed.The SAML IDP metadata option keys (
samlIDPMetaDataOptionsURLandsamlIDPMetaDataOptionsUserAttribute) are being reordered across all language files. This appears to be alphabetical sorting and has no functional impact on JSON.Also applies to: 287-290, 350-353, 413-416, 482-485, 545-548, 608-611, 671-674, 734-737, 797-800, 860-863, 923-926, 986-989, 1049-1052
154-163: Ensure the documentation anchor#device-authorizationis added to openidconnectservice.html.The Device Authorization section in the patch references
openidconnectservice.html#device-authorization. This documentation file is not present in the repository, so verify that the corresponding documentation update exists or will be included separately.
7-53: VerifyperlExprfunction exists in upstream LLNG codebase.The patch references
perlExprat line 27 forpamAccessServerGroupsvalidation, but this repository contains only patches for the LemonLDAP::NG Docker build process, not the LLNG source code itself. The function must be defined in the upstream LLNG source repository (https://github.com/LemonLdapNG/lemonldap-ng) for this patch to apply successfully. Confirm thatperlExpris available in the target LLNG version before merging.
74-91: The OIDC Device Authorization attributes are RFC 8628 compliant. The polling interval (5s) matches the RFC requirement for client-side defaults when omitted by the server. The user code length (8) aligns with RFC recommendations for base-20 character entropy. The expiration (600s) is valid; RFC 8628 does not mandate a protocol default, though its example uses 1800s.
132-146: Thepamaccess.htmlhelp file is missing from the patch.The PAM Access UI section references
pamaccess.htmlbut this documentation file is not included in the patch and does not exist in the codebase. The same applies toopenidconnectservice.htmlreferenced in the same section. Ensure these help documentation files are either added to the patch or will be provided separately.
| +"pamAccess":"PAM Access", | ||
| +"pamAccessActivation":"Activation", | ||
| +"pamAccessHeartbeatInterval":"Heartbeat interval", | ||
| +"pamAccessHeartbeatRequired":"Require heartbeat", | ||
| +"pamAccessInactiveThreshold":"Inactive threshold", | ||
| +"pamAccessMaxDuration":"Maximum token duration", | ||
| +"pamAccessRp":"OIDC Relying Party", | ||
| +"pamAccessRule":"Authorization rule", | ||
| +"pamAccessServerGroups":"Server groups", | ||
| +"pamAccessTokenDuration":"Default token duration", |
There was a problem hiding this comment.
Unused translation key pamAccessRule defined but no corresponding attribute.
The translation key pamAccessRule appears in all language files but there's no corresponding attribute definition in Attributes.pm or node in Tree.pm. This creates dead translation strings.
Either:
- Remove
pamAccessRulefrom all translation files if not needed - Add the corresponding attribute if the feature is planned
#!/bin/bash
# Verify if pamAccessRule is referenced anywhere in the codebase beyond translations
rg -n 'pamAccessRule' --type-not jsonAlso applies to: 262-271, 325-334, 388-397, 457-466, 520-529, 583-592, 646-655, 709-718, 772-781, 835-844, 898-907, 961-970, 1024-1033
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
manager/dp.patch (1)
199-208: Unused translation keypamAccessRule.This issue was already flagged in a previous review. The
pamAccessRuletranslation key exists across all language files but has no corresponding attribute definition inAttributes.pmor node inTree.pm.full/dp.patch (1)
199-208: Unused translation keypamAccessRule.This was flagged in a previous review - the
pamAccessRulekey exists in all language files but has no corresponding attribute or node definition.
🧹 Nitpick comments (1)
full/dp.patch (1)
252-261: Translations are mostly English fallbacks for non-French locales.Only French (
fr.json) has properly localized translations for the new PAM Access strings. Other languages use English text. This is acceptable for an initial implementation but consider adding proper translations in a follow-up.Also applies to: 305-314, 358-367, 411-420, 464-473, 517-526, 570-579, 623-632, 676-685, 729-738, 782-791, 835-844, 888-897
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Changes.md(1 hunks)full/Dockerfile(1 hunks)full/dp.patch(1 hunks)manager/Dockerfile(1 hunks)manager/dp.patch(1 hunks)portal/Dockerfile(1 hunks)transform(1 hunks)uwsgi-portal/Dockerfile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- Changes.md
🚧 Files skipped from review as they are similar to previous changes (2)
- uwsgi-portal/Dockerfile
- transform
🧰 Additional context used
🪛 GitHub Actions: Hadolint
portal/Dockerfile
[warning] 2-2: Hadolint DL3006: Always tag the version of an image explicitly.
[warning] 11-11: Hadolint DL3008: Pin versions in apt-get install. Instead of 'apt-get install ' use 'apt-get install ='.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (8)
full/Dockerfile (1)
34-34: LGTM!The
dp.patchis correctly added to the patch application loop, consistent with the existing pattern.portal/Dockerfile (1)
46-46: LGTM!The
dp.patchaddition follows the established pattern. The Hadolint warnings (DL3006, DL3008) flagged in the pipeline are pre-existing issues unrelated to this change.manager/dp.patch (2)
7-53: Well-structured PAM Access attribute definitions.The attribute configuration follows the established patterns with appropriate types, defaults, and documentation. The
pamAccessServerGroupsuseskeyTextContainerwith proper validation regex and Perl expression testing.
74-91: OIDC Device Authorization configuration looks correct.The three new attributes (
oidcServiceDeviceAuthorizationExpiration,oidcServiceDeviceAuthorizationPollingInterval,oidcServiceDeviceAuthorizationUserCodeLength) align with RFC 8628 requirements with reasonable defaults (600s expiration, 5s polling interval, 8-character user code).manager/Dockerfile (1)
41-41: LGTM!The
dp.patchaddition is consistent with the other Dockerfiles. The subsequentManager::Build->runcall will properly regenerate the configuration files with the new PAM Access and Device Authorization attributes.full/dp.patch (3)
7-53: PAM Access attributes are well-defined.The attribute definitions follow established patterns. Note that
pamAccessTokenDuration(default 600s) andpamAccessMaxDuration(default 3600s) have a sensible relationship where the max is greater than the default.
132-146: PAM Access UI section properly integrated.The new
pamAccessnode group is correctly placed with appropriate help link (pamaccess.html) and all defined attributes are included in the nodes array.
154-162: Device Authorization UI section follows the pattern.The
oidcServiceMetaDataDeviceAuthorizationgroup is correctly structured with RFC 8628 reference in the help link.
1 participant
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.