feat: add group configuration (+)

will only check users of such group

also cleaned some code:
- Input doesn't need to be UTF-8 valid.
- Main fuction is dead-simple, very readable.
- Added some modularity.
- Add some docs on behavior details.

Cargo.lock

@@ -62,6 +62,7 @@ name = "keycloak_pam"
 version = "0.1.0"
 dependencies = [
  "curl",
+ "libc",
  "nonstick",
  "serde",
  "serde_json",

Cargo.toml

@@ -17,6 +17,7 @@ trim = true
 
 [dependencies]
 curl = { version = "0.4.49", default-features = false }
+libc = { version = "0.2.186", default-features = false }
 nonstick = { version = "0.1.2", default-features = false, features = ["link"] }
 serde = { version = "1.0.228", default-features = false, features = ["derive"] }
 serde_json = { version = "1.0.149", default-features = false, features = [

README.md

@@ -12,12 +12,18 @@ This is being worked on, refer to the test configuration at [`tests/nixos/config
 
 Module can be easily configured with PAM arguments, for the sake of completeness refer to the source code.
 
-You can `cargo doc` to find the documentation for this at `keycloak_pam > Args`.
+You can `cargo doc` to find the documentation for this at `keycloak_pam > args > Args`.
 
-# Restrictions
+# Restrictions and Guarantees
 
 With the purpose of being tiny and minimal, the module uses libcurl for the few requests it makes, so HTTP/S and all networking behavior is offloaded to it.
 
+This is also made in rust, which means default `String` and `&str` are guaranteed to be UTF-8 encoded and invalid UTF-8 input might fail. I did try to use OS and C strings for all input, but I recommend still avoiding non-UTF8 input.
+
+The code should **NEVER** panic (aside from memory management issues), no `unwrap`, `expect` or such methods should ever be used, slice indexing is avoided and errors are gracefully managed.
+
+To check the group, keycloak-pam uses libc's `getgrnam_r`, this takes a buffer we hardcoded to 4 KiB, if there happens to be a group bigger than this, keycloak-pam won't be able to handle and will fail explaining this.
+
 # Behavior
 
 To authenticate users, the module uses the endpoint `{BASE_URL}/realms/{realm}/protocol/openid-connect/token` as a configured client. This needs a client ID and its secret that you need to get and configure per realm.

default.nix

@@ -9,7 +9,7 @@ pkgs.rustPlatform.buildRustPackage {
 
   src = pkgs.lib.cleanSource ./.;
 
-  cargoHash = "sha256-Mpcd7OKJi7QAkwpUZd8aBfTsdQ+bRGUz5Dr36iJyi4g=";
+  cargoHash = "sha256-We/E3T6SRZeTj3GZNySjhdlcg49v8FD8sGA5pyKSOXA=";
 
   buildInputs = [
     pkgs.pam

shell.nix

@@ -5,7 +5,7 @@
     {
       devShells.default = pkgs.mkShell {
         inputsFrom = [
-          inputs'.error's-rust-overlay.devShells.rust-nightly
+          inputs'.error's-rust-overlay.devShells.rust
         ];
 
         shellHook = ''
@@ -20,20 +20,18 @@
           done
 
           if [ -z "$QEMU_BRIDGE_HELPER_PATH" ]; then
-            printf "\033[1;33m%s\033[0m\n" \
-              "WARN: 'qemu-bridge-helper' not found, make sure it is installed and the nix shell hook is looking for it" >&2
+            printf "\033[1;33m%s%s\033[0m\n" \
+              "WARN: 'qemu-bridge-helper' not found, make sure it is " \
+              "installed and the nix shell hook is looking for it" >&2
           fi
         '';
 
         packages = with pkgs; [
-          # cargo
           curl
           delta
           git
           nixfmt
           pam
-          # rust-analyzer
-          # rustc
         ];
       };
     };

src/args.rs

@@ -0,0 +1,36 @@
+use std::ffi::CStr;
+
+use crate::ext::CStrExt as _;
+
+pub const DEFAULT_CONFIGURATION_PATH: &CStr = c"/etc/keycloak-pam.json";
+
+/// # Arguments
+///
+/// - `config=`, optional, accepts a path to load json configuration from, defaults to [`DEFAULT_CONFIGURATION_PATH`]
+/// - `debug`, optional, flag, ignores the PAM policy about silence and prints regardless
+pub struct Args<'a> {
+    pub config: &'a CStr,
+    pub debug: bool,
+}
+
+impl<'a> Args<'a> {
+    #[must_use]
+    pub fn parse(args: &[&'a CStr]) -> Self {
+        let mut parsed_args = Args {
+            config: DEFAULT_CONFIGURATION_PATH,
+            debug: false,
+        };
+
+        for arg in args {
+            if arg == &c"debug" {
+                parsed_args.debug = true;
+            }
+
+            if let Some((b"config", path)) = arg.split_ch(b'=') {
+                parsed_args.config = path;
+            }
+        }
+
+        parsed_args
+    }
+}

src/ext.rs

@@ -1,4 +1,7 @@
-use std::ffi::CStr;
+use std::{
+    ffi::{CStr, OsStr},
+    os::unix::ffi::OsStrExt as _,
+};
 
 pub trait SliceExt {
     fn split_once_ch(&self, ch: u8) -> Option<(&[u8], &[u8])>;
@@ -13,10 +16,15 @@ impl SliceExt for [u8] {
 }
 
 pub trait CStrExt {
+    fn to_os_str(&self) -> &OsStr;
     fn split_ch(&self, ch: u8) -> Option<(&[u8], &CStr)>;
 }
 
 impl CStrExt for CStr {
+    fn to_os_str(&self) -> &OsStr {
+        OsStr::from_bytes(self.to_bytes())
+    }
+
     fn split_ch(&self, ch: u8) -> Option<(&[u8], &CStr)> {
         let self_slice = self.to_bytes_with_nul();
 
@@ -27,3 +35,13 @@ impl CStrExt for CStr {
         })
     }
 }
+
+pub trait StrExt {
+    fn as_os_str(&self) -> &OsStr;
+}
+
+impl StrExt for str {
+    fn as_os_str(&self) -> &OsStr {
+        OsStr::from_bytes(self.as_bytes())
+    }
+}

src/keycloak/mod.rs

@@ -1,20 +1,25 @@
 use std::{
-    ffi::OsStr,
+    ffi::{CStr, CString, OsStr},
     fs::File,
     io::{self},
+    os::unix::ffi::OsStrExt as _,
 };
 
 use curl::easy::{Easy, List};
 use serde::Deserialize;
 
+use crate::{ext::CStrExt as _, libc as mylibc};
+
 mod api;
 
+#[allow(clippy::unsafe_derive_deserialize)] // seriously? -_-
 #[derive(Debug, Deserialize)]
 pub struct KeycloakAPI {
     base_url: String,
     default_realm: String,
     client_id: String,
     client_secret: String,
+    unix_group: CString,
 }
 
 #[derive(Debug, thiserror::Error)]
@@ -68,13 +73,35 @@ impl KeycloakAPI {
         Ok(serde_json::from_reader(f)?)
     }
 
-    pub fn check_credentials(&self, username: &str, password: &str) -> Result<(), LoginError> {
+    pub fn should_skip(&self, username: &OsStr) -> Result<bool, mylibc::GetgrnamError> {
+        mylibc::getgrnam_use(&self.unix_group, |group| {
+            let mut members_ptr = group.gr_mem;
+
+            // SAFETY: The array pointer CANNOT be null, even if incremented.
+            while let member_cstr_ptr = unsafe { *members_ptr }
+                && !member_cstr_ptr.is_null()
+            {
+                // SAFETY: its guaranteed to be a CStr and its non-null
+                let member_osstr = unsafe { CStr::from_ptr(member_cstr_ptr) }.to_os_str();
+                if member_osstr == username {
+                    return false;
+                }
+
+                // SAFETY: if current pointer wasn't NULL, we can increase to the next one
+                members_ptr = unsafe { members_ptr.add(1) };
+            }
+
+            true
+        })
+    }
+
+    pub fn check_credentials(&self, username: &OsStr, password: &OsStr) -> Result<(), LoginError> {
         let body = format!(
             "grant_type=password&client_id={CLIENT_ID}&client_secret={CLIENT_SECRET}&username={USERNAME}&password={PASSWORD}",
             CLIENT_ID = urlencoding::encode(&self.client_id),
             CLIENT_SECRET = urlencoding::encode(&self.client_secret),
-            USERNAME = urlencoding::encode(username),
-            PASSWORD = urlencoding::encode(password),
+            USERNAME = urlencoding::encode_binary(username.as_bytes()),
+            PASSWORD = urlencoding::encode_binary(password.as_bytes()),
         );
 
         let (status_code, body) = curl_request(

src/lib.rs

@@ -1,100 +1,78 @@
 //! Privileged PAM module.
 
-use std::{
-    ffi::{CStr, OsStr},
-    os::unix::ffi::OsStrExt as _,
-};
+use std::ffi::CStr;
 
 use nonstick::{ErrorCode, ModuleClient, PamModule, pam_export};
 
 use keycloak::KeycloakAPI;
 
+use crate::args::Args;
 use crate::ext::CStrExt as _;
 
+pub mod args;
 pub mod ext;
 pub mod keycloak;
+pub mod libc;
 
-pub const DEFAULT_CONFIGURATION_PATH: &CStr = c"/etc/keycloak-pam.json";
-
-struct KeycloakPAM;
+pub struct KeycloakPAM;
 pam_export!(KeycloakPAM);
 
-/// # Arguments
+/// Conditionally prints the error and returns the value intouched.
 ///
-/// - `config=`, optional, accepts a path to load json configuration from, defaults to [`DEFAULT_CONFIGURATION_PATH`]
-/// - `debug`, optional, flag, ignores the PAM policy about silence and prints regardless
-pub struct Args<'a> {
-    config: &'a CStr,
-    debug: bool,
-}
-
-impl<'a> Args<'a> {
-    fn parse(args: &[&'a CStr]) -> Self {
-        let mut parsed_args = Args {
-            config: DEFAULT_CONFIGURATION_PATH,
-            debug: false,
+/// All prints should use this macro to also format output.
+macro_rules! error_print {
+    ($expr:expr, $if_print:ident) => {{
+        let expr = $expr;
+        if let Err(ref error) = expr
+            && $if_print
+        {
+            eprintln!("keycloak-pam: {error}");
         };
-
-        for arg in args {
-            if arg == &c"debug" {
-                parsed_args.debug = true;
-            }
-
-            if let Some((b"config", path)) = arg.split_ch(b'=') {
-                parsed_args.config = path;
-            }
-        }
-
-        parsed_args
-    }
+        expr
+    }};
 }
 
+/// Errors:
+///
+/// - [`ErrorCode::ConversationError`]: Something wrong converting types (e.g. UTF-8)
+/// - [`ErrorCode::ServiceError`]: Something misconfigured in the service (e.g. bad configuration)
+/// - [`ErrorCode::PermissionDenied`]: User not meant to be handled (e.g. user not member of
+///   configured group)
+/// - [`ErrorCode::AuthInfoUnavailable`]: Error fetch auth info (e.g. keycloak backend down)
+/// - [`ErrorCode::AuthenticationError`]: Failed to authenticate (e.g. wrong credentials)
 impl<M: ModuleClient> PamModule<M> for KeycloakPAM {
     fn authenticate(
         handle: &mut M,
         args: Vec<&CStr>,
         flags: nonstick::AuthnFlags,
     ) -> nonstick::Result<()> {
-        fn osstr_to_str(p: &OsStr) -> nonstick::Result<&str> {
-            p.to_str().ok_or(ErrorCode::AuthInfoUnavailable)
-        }
+        use ErrorCode as E;
 
         let args = Args::parse(&args);
 
         let username = handle.username(None)?;
-        let username = osstr_to_str(&username)?;
         let password = handle.authtok(None)?;
-        let password = osstr_to_str(&password)?;
 
         let no_silent_flag = (flags & nonstick::AuthnFlags::SILENT).is_empty();
-        let can_print = no_silent_flag || args.debug;
+        let can_print = no_silent_flag | args.debug;
 
-        let api_client =
-            match KeycloakAPI::load_from_configuration(OsStr::from_bytes(args.config.to_bytes())) {
-                Ok(v) => v,
-                Err(error) => {
-                    if can_print {
-                        eprintln!("{error}");
-                    }
-                    return Err(ErrorCode::ServiceError);
-                }
-            };
+        {
+            let api_client = KeycloakAPI::load_from_configuration(args.config.to_os_str());
+            let api_client = error_print!(api_client, can_print).map_err(|_| E::ServiceError)?;
 
-        match api_client.check_credentials(username, password) {
-            Ok(()) => Ok(()),
-            Err(error) => {
-                if can_print {
-                    eprintln!("{error}");
-                }
-
-                let err = match error {
-                    keycloak::LoginError::Fetch(_)
-                    | keycloak::LoginError::InvalidAPIResponse(_) => ErrorCode::ServiceError,
-                    keycloak::LoginError::InvalidLogin(_) => ErrorCode::CredentialsError,
-                };
-
-                Err(err)
+            let should_skip = error_print!(api_client.should_skip(&username), can_print)
+                .map_err(|_| E::ServiceError)?;
+            if should_skip {
+                return Err(E::PermissionDenied);
             }
+
+            let checked_credentials = api_client.check_credentials(username.as_os_str(), &password);
+            error_print!(checked_credentials, can_print).map_err(|error| match error {
+                keycloak::LoginError::Fetch(_) | keycloak::LoginError::InvalidAPIResponse(_) => {
+                    E::AuthInfoUnavailable
+                }
+                keycloak::LoginError::InvalidLogin(_) => E::AuthenticationError,
+            })
         }
     }
 

src/libc.rs

@@ -0,0 +1,72 @@
+use std::{ffi::CStr, mem::MaybeUninit};
+
+use libc::{ERANGE, c_char};
+
+#[derive(Debug, thiserror::Error)]
+pub enum GetgrnamError {
+    #[error("no matching group record found")]
+    NoMatchingRecord,
+    #[error("insufficient buffer space. The group is too big to be handled by keycloak-pam")]
+    Range,
+    /// Ace variant for unexpected errors, error handling is shallow here
+    #[error("unexpected getgrnam_r error")]
+    Unexpected,
+}
+
+// TODO: Reorganise this function, error handing is shallow
+
+/// # Safety
+///
+/// The function is completely safe to use but the use of `cb` requires unsafe:
+///
+/// - All raw pointers in there are only valid for the duration of the function as the buffer they
+///   point to is local to this function.
+pub fn getgrnam_use<T>(
+    cname: &CStr,
+    mut cb: impl FnMut(libc::group) -> T,
+) -> Result<T, GetgrnamError> {
+    use GetgrnamError as E;
+
+    let name = cname.as_ptr();
+    let mut grp = MaybeUninit::uninit();
+    let mut buf = [c_char::from(0); 4096];
+    let mut result: MaybeUninit<*mut libc::group> = MaybeUninit::uninit();
+
+    // SAFETY: its just a C call, I'm respecting signature and assumptions
+    let ret = unsafe {
+        libc::getgrnam_r(
+            name,
+            grp.as_mut_ptr(),
+            buf.as_mut_ptr(),
+            buf.len(),
+            result.as_mut_ptr(),
+        )
+    };
+
+    // SAFETY: Pointers inside `grp` point may point to the buffer `buf` so they cannot be
+    // moved outside of this function.
+
+    // > On success, getgrnam_r() [..] return zero, and set `*result` to `grp`.
+    //
+    // > If no matching group record was found, these functions return 0 and store NULL in
+    // `*result`.
+    //
+    // > In case of error, an error number is returned, and NULL stored in `*result`.
+    //
+    // SAFETY: Either way, `result` is initialized to a nullable pointer.
+    let result_ptr = unsafe { result.assume_init() };
+    if ret == 0 && result_ptr == grp.as_mut_ptr() {
+        let gr = unsafe { grp.assume_init() };
+        Ok(cb(gr))
+    } else if result_ptr.is_null() {
+        Err(match ret {
+            0 => E::NoMatchingRecord,
+            ERANGE => E::Range,
+            _ => E::Unexpected,
+        })
+    } else {
+        // ret should be != 0 and result NULL, but any other case is also unexpected and I'm
+        // not doing much FFI error handling rn
+        Err(E::Unexpected)
+    }
+}

tests/nixos/configuration.nix

@@ -94,9 +94,14 @@ in
       };
     };
 
-  users.users.admin = {
-    password = config.services.keycloak.initialAdminPassword;
-    isNormalUser = true;
+  users = {
+    users.admin = {
+      password = config.services.keycloak.initialAdminPassword;
+      isNormalUser = true;
+      extraGroups = [ "keycloak-login" ];
+    };
+
+    groups.keycloak-login = { };
   };
 
   services.keycloak = {
@@ -141,7 +146,8 @@ in
                 "base_url": "http://127.0.0.1:${toString config.services.keycloak.settings.http-port}",
                 "default_realm": "master",
                 "client_id": "account",
-                "client_secret": "7bsrMrRuNKpqsxcOOJavWjEqek12ZCkj"
+                "client_secret": "7bsrMrRuNKpqsxcOOJavWjEqek12ZCkj",
+                "unix_group": "keycloak-login"
               }
             '';
           };