Re: [pdm-devel] [PATCH datacenter-manager 2/3] server: api: add support for adding openid realms and openid logins

["Shannon Sterz" <s.sterz@proxmox.com>]

On Fri Oct 17, 2025 at 9:57 AM CEST, Fabian Grünbichler wrote:
> On October 14, 2025 3:30 pm, Shannon Sterz wrote:

-->8 snip 8<--

>> +            let (realm, private_auth_state) =
>> +                OpenIdAuthenticator::verify_public_auth_state(PDM_RUN_DIR_M!(), &state)?;
>> +
>> +            let (domains, _digest) = pdm_config::domains::config()?;
>> +            let config: OpenIdRealmConfig = domains.lookup("openid", &realm)?;
>> +            let open_id = openid_authenticator(&config, &redirect_url)?;
>> +            let info = open_id.verify_authorization_code_simple(&code, &private_auth_state)?;
>> +            let name_attr = config.username_claim.as_deref().unwrap_or("sub");
>> +
>> +            // Try to be compatible with previous versions
>> +            let try_attr = match name_attr {
>> +                "subject" => Some("sub"),
>> +                "username" => Some("preferred_username"),
>> +                _ => None,
>> +            };
>> +
>> +            let unique_name = if let Some(name) = info[name_attr]
>> +                .as_str()
>> +                .or_else(|| try_attr.and_then(|att| info[att].as_str()))
>> +            {
>> +                name.to_owned()
>> +            } else {
>> +                bail!("missing claim '{name_attr}'");
>> +            };
>> +
>> +            let user_id = Userid::try_from(format!("{unique_name}@{realm}"))?;
>> +            tested_username = Some(unique_name);
>> +
>> +            if !user_info.is_active_user_id(&user_id) {
>> +                if config.autocreate.unwrap_or(false) {
>> +                    let _lock = proxmox_access_control::user::lock_config()?;
>> +                    let (mut user_config, _digest) = proxmox_access_control::user::config()?;
>> +
>> +                    if let Ok(old_user) = user_config.lookup::<User>("user", user_id.as_str()) {
>> +                        if let Some(false) = old_user.enable {
>> +                            bail!("user '{user_id}' is disabled.");
>> +                        } else {
>> +                            bail!("autocreate user failed - '{user_id}' already exists.");
>> +                        }
>> +                    }
>> +
>> +                    let firstname = info["given_name"]
>> +                        .as_str()
>> +                        .map(|n| n.to_string())
>> +                        .filter(|n| FIRST_NAME_SCHEMA.parse_simple_value(n).is_ok());
>> +
>> +                    let lastname = info["family_name"]
>> +                        .as_str()
>> +                        .map(|n| n.to_string())
>> +                        .filter(|n| LAST_NAME_SCHEMA.parse_simple_value(n).is_ok());
>> +
>> +                    let email = info["email"]
>> +                        .as_str()
>> +                        .map(|n| n.to_string())
>> +                        .filter(|n| EMAIL_SCHEMA.parse_simple_value(n).is_ok());
>> +
>> +                    let user = User {
>> +                        userid: user_id.clone(),
>> +                        comment: None,
>> +                        enable: None,
>> +                        expire: None,
>> +                        firstname,
>> +                        lastname,
>> +                        email,
>> +                    };
>> +
>> +                    user_config.set_data(user.userid.as_str(), "user", &user)?;
>> +                    proxmox_access_control::user::save_config(&user_config)?;
>> +                } else {
>> +                    bail!("user account '{user_id}' missing, disabled or expired.");
>> +                }
>> +            }
>> +
>> +            let api_ticket = ApiTicket::Full(user_id.clone());
>> +            let ticket = Ticket::new(auth_context.auth_prefix(), &api_ticket)?;
>> +            let token = assemble_csrf_prevention_token(auth_context.csrf_secret(), &user_id);
>> +            env.log_auth(user_id.as_str());
>> +
>> +            Ok((user_id, ticket, token))
>> +        })();
>> +
>> +        let (user_id, mut ticket, token) = result.map_err(|err| {
>> +            let msg = err.to_string();
>> +            env.log_failed_auth(tested_username, &msg);
>
> this is copied over from PBS, but isn't this also kinda wrong? not every
> error above is a failed auth.. at least if we compare error handling
> here with the one in the regular ticket flow, this is inconsistent..
>
> might rather be follow-up material after closer thought though, and
> ensuring PBS and PDM behave the same afterwards..


yes this requires some though as to whether will accidentally leak some
information here otherwise. for now i'd keep it in line with pbs.

incorporated the rest of the feedback here into a v2!


_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel