feat(matic): enable PAM fingerprint auth and fix falcon init - #743
Conversation
- Enable fingerprint auth for login, GDM, and sudo via PAM - Fix falcon init script to skip cleanup when already installed (CrowdStrike protects its files with kernel module) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances system security and robustness by integrating fingerprint authentication across key system services like login, GDM, and sudo, providing a more convenient and secure access method. Concurrently, it refines the CrowdStrike Falcon agent's initialization process to be more resilient, avoiding conflicts with its self-protection mechanisms by only performing installation steps if the agent is not already present. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DREnable PAM fingerprint authentication for login, GDM, and sudo, and fix CrowdStrike Falcon init script to prevent reinitialization errors. What changed?
Description generated by Mesa. Update settings |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughEnables fingerprint authentication via PAM for three services (login, GDM, sudo) in NixOS configuration, and refactors CrowdStrike Falcon daemon initialization to be idempotent by guarding setup operations with a file presence check instead of unconditional cleanup. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enables fingerprint authentication and updates the CrowdStrike Falcon initialization script. The fingerprint configuration is a good addition. However, the change to the Falcon init script, while fixing the re-initialization issue, introduces a new problem where the agent will not be updated when the Nix package changes. I've provided a suggestion to address this by ensuring the agent files are always synchronized, which should handle both initial installs and upgrades correctly.
| # Only initialize if not already set up (CrowdStrike protects its files) | ||
| if [ ! -f /opt/CrowdStrike/falcond ]; then | ||
| install -d -m 0770 /opt/CrowdStrike | ||
| cp -a ${falcon}/opt/CrowdStrike/. /opt/CrowdStrike/ | ||
| chown -R root:root /opt/CrowdStrike | ||
| fi |
There was a problem hiding this comment.
The current logic with if [ ! -f /opt/CrowdStrike/falcond ] prevents the CrowdStrike agent from being updated when the underlying Nix package is changed. This is because the script will skip the installation steps if falcond already exists.
According to the PR description, the agent's files are protected from removal, but cp -a should still be able to overwrite them to perform an upgrade. By removing the conditional, we ensure that the files are always synchronized from the Nix store on startup, which handles both initial installation and upgrades.
# Always copy files to handle initial install and upgrades.
# The agent's files are protected from removal, but can be overwritten.
install -d -m 0770 /opt/CrowdStrike
cp -a ${falcon}/opt/CrowdStrike/. /opt/CrowdStrike/
chown -R root:root /opt/CrowdStrike
There was a problem hiding this comment.
Pull request overview
This PR enables fingerprint authentication for PAM services and fixes the CrowdStrike Falcon initialization script to handle file protection more gracefully.
Changes:
- Added fingerprint authentication support for login, GDM, and sudo via PAM configuration
- Refactored Falcon init script to check for existing installation rather than attempting cleanup on every service start
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| named-hosts/matic/default.nix | Added fprintAuth configuration for login, gdm, and sudo PAM services |
| named-hosts/matic/falcon.nix | Changed initialization logic to skip setup when falcond already exists, avoiding attempts to remove protected files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| services.fprintd.enable = true; | ||
| security.pam.services.login.fprintAuth = true; | ||
| security.pam.services.gdm.fprintAuth = true; | ||
| security.pam.services.sudo.fprintAuth = true; |
There was a problem hiding this comment.
The security.pam.services.sudo.fprintAuth = true setting is redundant given that security.sudo.wheelNeedsPassword = false (line 70) already allows passwordless sudo for wheel group members. While this doesn't cause any functional issues, it may create confusion about the actual authentication mechanism being used for sudo.
Consider either:
- Removing the fprintAuth setting for sudo since it's not needed with passwordless sudo, or
- Adding a comment explaining that this is for future-proofing in case the wheelNeedsPassword setting changes, or
- Changing wheelNeedsPassword to true if you want to require either password or fingerprint for sudo.
If the intent is to require authentication (via fingerprint or password) for sudo, you should set security.sudo.wheelNeedsPassword = true.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="named-hosts/matic/falcon.nix">
<violation number="1" location="named-hosts/matic/falcon.nix:21">
P1: The current initialization logic completely skips the copy step if `falcond` exists, which prevents the sensor from being updated. If you update the `falcon-sensor` package in Nix, the system will continue running the old binary from `/opt/CrowdStrike` instead of the new one from the store.
Using `cmp` allows you to skip initialization when the binary hasn't changed (fixing the restart crash loop) while still attempting to update when the version changes.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if [ -d /opt/CrowdStrike ]; then | ||
| ${pkgs.e2fsprogs}/bin/chattr -i -R /opt/CrowdStrike 2>/dev/null || true | ||
| # Only initialize if not already set up (CrowdStrike protects its files) | ||
| if [ ! -f /opt/CrowdStrike/falcond ]; then |
There was a problem hiding this comment.
P1: The current initialization logic completely skips the copy step if falcond exists, which prevents the sensor from being updated. If you update the falcon-sensor package in Nix, the system will continue running the old binary from /opt/CrowdStrike instead of the new one from the store.
Using cmp allows you to skip initialization when the binary hasn't changed (fixing the restart crash loop) while still attempting to update when the version changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At named-hosts/matic/falcon.nix, line 21:
<comment>The current initialization logic completely skips the copy step if `falcond` exists, which prevents the sensor from being updated. If you update the `falcon-sensor` package in Nix, the system will continue running the old binary from `/opt/CrowdStrike` instead of the new one from the store.
Using `cmp` allows you to skip initialization when the binary hasn't changed (fixing the restart crash loop) while still attempting to update when the version changes.</comment>
<file context>
@@ -17,19 +17,13 @@ let
- if [ -d /opt/CrowdStrike ]; then
- ${pkgs.e2fsprogs}/bin/chattr -i -R /opt/CrowdStrike 2>/dev/null || true
+ # Only initialize if not already set up (CrowdStrike protects its files)
+ if [ ! -f /opt/CrowdStrike/falcond ]; then
+ install -d -m 0770 /opt/CrowdStrike
+ cp -a ${falcon}/opt/CrowdStrike/. /opt/CrowdStrike/
</file context>
Changes
Technical Details
security.pam.services.{login,gdm,sudo}.fprintAuth = truefor fingerprint at loginTesting
sudo fprintd-enrollGenerated with Claude Code by claude-opus-4-5-20250101
Summary by cubic
Enables fingerprint authentication for login, GDM, and sudo on matic, and updates the Falcon init script to avoid reinitializing when the sensor is already installed.
New Features
Bug Fixes
Written for commit 4ef7f13. Summary will update on new commits.