Issue 7468 - RFE - HIBP password breach validation by jchapma · Pull Request #7492 · 389ds/389-ds-base

jchapma · 2026-05-12T13:09:20Z

Description:
Integrate the HIBP HTTP client into password policy. Adds config, schema, and password validation logic to check passwords against the HIBP breach database during password add and modify operations. Depends on the HIBP client PR.

Schema: passwordBreachCheck, passwordBreachDbUrl, passwordBreachDbTimeout
Config: Config setters in libglobs.c
Validation: Check passwords on add/modify for both admin and non-admin users
rootDN: Also validates nsslapd-rootpw changes against breach database

Relates: #7468

Reviewed by:

Summary by Sourcery

Integrate breached password checking into the directory server’s password policy using the HIBP client, including configuration, schema wiring, and runtime enforcement for password operations.

New Features:

Add configurable password breach checking to the global password policy, including enable/disable flag, breach database URL, and timeout settings.
Validate user and rootDN passwords against the HIBP breach database during add and modify operations when the feature is enabled.

Enhancements:

Initialize and wire password breach policy settings into existing password policy handling so per-policy and global defaults are honored.
Hook HIBP initialization into server startup so breached password checks are available when configured.

sourcery-ai

Hey - I've found 2 issues, and left some high level feedback:

In the add and non-root modify paths, hibp_check_password is called on all values without checking slapi_is_encoded, which could lead to hashed or otherwise encoded passwords being sent to the breach service; mirroring the rootpw handling by skipping encoded values would avoid this.
The breach-checking logic is duplicated across op_shared_add, op_shared_allow_pw_change, and the rootpw handling in op_shared_modify; consider extracting a shared helper that takes a value list and policy to reduce repetition and keep future behavior changes consistent.
There is a stray tab-based indentation change in op_shared_allow_pw_change around the "This is an internal operation" comment; aligning this with the existing space-based indentation would keep the file formatting consistent.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In the add and non-root modify paths, `hibp_check_password` is called on all values without checking `slapi_is_encoded`, which could lead to hashed or otherwise encoded passwords being sent to the breach service; mirroring the rootpw handling by skipping encoded values would avoid this.
- The breach-checking logic is duplicated across `op_shared_add`, `op_shared_allow_pw_change`, and the rootpw handling in `op_shared_modify`; consider extracting a shared helper that takes a value list and policy to reduce repetition and keep future behavior changes consistent.
- There is a stray tab-based indentation change in `op_shared_allow_pw_change` around the "This is an internal operation" comment; aligning this with the existing space-based indentation would keep the file formatting consistent.

## Individual Comments

### Comment 1
<location path="ldap/servers/slapd/add.c" line_range="674-677" />
<code_context>
                 present_values = attr_get_present_values(attr);
+#ifdef ENABLE_HIBP
+                /* Check all passwords against breach database */
+                if (pwpolicy->pw_check_breach) {
+                    for (size_t i = 0; present_values[i] != NULL; i++) {
+                        const char *pwd = slapi_value_get_string(present_values[i]);
+                        int breach_count = hibp_check_password(pwd, pwpolicy);
+                        if (breach_count > 0) {
+                            slapi_log_err(SLAPI_LOG_WARNING, "op_shared_add",
</code_context>
<issue_to_address>
**🚨 issue (security):** Avoid sending hashed passwords to the breach checker in add operations

In `op_shared_add`, the breach check runs over all present values without skipping already-encoded passwords (e.g. pre-hashed `userPassword`). This can leak reusable hashed secrets to the HIBP backend and may misbehave if `hibp_check_password` expects cleartext. Please align with the rootpw/modify logic and skip values where `slapi_is_encoded(pwd)` is true before calling `hibp_check_password`.
</issue_to_address>

### Comment 2
<location path="ldap/servers/slapd/modify.c" line_range="1311-1316" />
<code_context>

+#ifdef ENABLE_HIBP
+    /* Check all passwords against breach database before any other checks */
+    if (pwpolicy->pw_check_breach && mod->mod_bvalues) {
+        Slapi_Value **breach_vals = NULL;
+        valuearray_init_bervalarray(mod->mod_bvalues, &breach_vals);
+        if (breach_vals) {
+            for (size_t i = 0; breach_vals[i] != NULL; i++) {
+                const char *pwd = slapi_value_get_string(breach_vals[i]);
+                int breach_count = hibp_check_password(pwd, pwpolicy);
+                if (breach_count > 0) {
</code_context>
<issue_to_address>
**🚨 issue (security):** Skip encoded passwords when running breach checks during modify operations

In `op_shared_allow_pw_change`, the breach check runs on all `mod->mod_bvalues` without skipping encoded values. This can send pre-hashed `userPassword` values to `hibp_check_password`, which both misuses the API (it expects cleartext) and risks leaking reusable hash material to the external service. Please add a `!slapi_is_encoded(pwd)` guard before calling `hibp_check_password`, consistent with the rootpw handling.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

mreynolds389 · 2026-05-20T20:45:02Z

@jchapma I think this needs to be rebased. It's not building.

packit-as-a-service · 2026-05-20T22:05:34Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo dnf install -y 'dnf*-command(copr)'
dnf copr enable packit/389ds-389-ds-base-7492
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

droideck · 2026-05-29T23:13:24Z

+#ifndef ENABLE_HIBP
+    if (apply && value && strcasecmp(value, "on") == 0) {
+        slapi_log_err(SLAPI_LOG_WARNING, "config_set_pw_breach_check",
+                      "HIBP breached password checking not enabled - passwordBreachCheck has no effect\n");


I think we should reject on with an LDAP error when HIBP support is unavailable.
An admin would see the setting as enabled and reasonably believe breached-password protection is active. I don't think logging the warning is visible enough.

droideck · 2026-05-29T23:15:55Z

+            for (size_t i = 0; breach_vals[i] != NULL; i++) {
+                const char *pwd = slapi_value_get_string(breach_vals[i]);
+                if (pwd && !slapi_is_encoded((char *)pwd)) {
+                    int breach_count = hibp_check_password(pwd, pwpolicy);


If we call HIBP before the ACI/password-change checks, I think, an unauthorized user can trigger outbound HIBP requests for chosen values before the server rejects the modify. The add path already checks ACI first; modify should do the same, IMO

You are correct, we should do the HIBP check after ACI checks, thanks

droideck · 2026-05-29T23:17:34Z


+#ifdef ENABLE_HIBP
+    /* Check all passwords against breach database before any other checks */
+    if (pwpolicy->pw_check_breach && mod->mod_bvalues) {


IIUC, it will be checked even for LDAP_MOD_DELETE of the userPassword attribute and that seems wrong. We should be able to delete breached password

droideck · 2026-05-29T23:24:00Z

+                } else if (!strcasecmp(attr_name, "passwordBreachDbUrl")) {
+                    if ((sval = attr_get_present_values(attr))) {
+                        pwdpolicy->pw_breach_db_url =
+                            slapi_ch_strdup(slapi_value_get_string(*sval));


Do we free it? I'm not fully sure who owns it (I wasn't able to find)

We/I do not :(

droideck · 2026-05-29T23:27:00Z

+    if (config_value_is_null(attrname, value, errorbuf, 0)) {
+        value = NULL;
+    }
+


I wonder if we should validate passwordBreachDbUrl to require https://, normalize/require a trailing / and ideally also set libcurl allowed protocols to HTTPS only with CURLOPT_PROTOCOLS_STR. CURLOPT_DEFAULT_PROTOCOL can make scheme-less URLs default to HTTPS, but for this security feature I’d still reject scheme-less input rather than guess. WDYT?

Thats a good idea, currently there is no validation. Thanks

droideck · 2026-06-24T00:24:27Z

                    pwdpolicy->pw_check_dict = g_pwdpolicy->pw_check_dict;
                    pwdpolicy->pw_dict_path = g_pwdpolicy->pw_dict_path;
+                    pwdpolicy->pw_check_breach = g_pwdpolicy->pw_check_breach;
+                    pwdpolicy->pw_breach_db_url = g_pwdpolicy->pw_breach_db_url;


I think this might accidentally hand ownership of the global breach URL to the local policy. Since delete_passwdPolicy() frees pw_breach_db_url for local policies, could we duplicate this value here or otherwise avoid freeing a borrowed global pointer?

Ah yes, this is a bug. The global pw_breach_db_url is asigned directly and will be freed, possibly causing a double free. Duplicating pw_breach_db_url.

droideck · 2026-06-24T00:25:17Z

+    assert value.lower() == 'off'
+
+    # Verify setting passwordBreachCheck to 'on'
+    inst.config.set('passwordBreachCheck', 'on')


I think this may fail when the server is built without HIBP support, which is the default.

I just tried it and this test does fail when HIBP is not enabled, I will add a try/except to skip the test the HIBP assertions when the server returned UNWILLING_TO_PERFORM. Apologies I though I had tested this

droideck · 2026-06-24T00:28:45Z

+         * Check password against breach database after ACI validation.
+         */
+        if (!SLAPI_IS_MOD_DELETE(mod->mod_op) &&
+            !pw_is_pwp_admin(pb, pwpolicy, PWP_ADMIN_OR_ROOTDN) &&


The add path checks all users, and the commit message says admin and non-admin password changes are validated, but this condition lets admins bypass HIBP during modify. I think we might have an inconsistancy unless I miss something...

Good catch, there was an inconsistency. The modify path allowed admins to bypass HIBP, but add did not. Added the admin bypass check to add.c

droideck · 2026-06-24T00:29:48Z

+    {CONFIG_PW_BREACH_TIMEOUT_ATTRIBUTE, config_set_pw_breach_timeout,
+     NULL, 0,
+     (void **)&global_slapdFrontendConfig.pw_policy.pw_breach_db_timeout,
+     CONFIG_INT, NULL, "5", NULL},


This default is 5... While the policy/client defaults are 10. Should they be align?

Updated to match HIBO default

Description: Integrate the HIBP HTTP client into password policy. Adds config, schema, and password validation logic to check passwords against the HIBP breach database during password add and modify operations. Depends on the HIBP client PR. - Schema: passwordBreachCheck, passwordBreachDbUrl, passwordBreachDbTimeout - Config: Config setters in libglobs.c - Validation: Check passwords on add/modify for both admin and non-admin users - rootDN: Also validates nsslapd-rootpw changes against breach database Dependencies: - Issue 7468 - RFE - Add HIBP HTTP client Relates: 389ds#7468 Reviewed by:

sourcery-ai Bot reviewed May 12, 2026

View reviewed changes

Comment thread ldap/servers/slapd/add.c Outdated

Comment thread ldap/servers/slapd/modify.c Outdated

jchapma force-pushed the hibp-pwd-validation branch from f2b947f to 7d8cfd2 Compare May 20, 2026 21:54

jchapma force-pushed the hibp-pwd-validation branch from 7d8cfd2 to 39f3bd5 Compare May 21, 2026 09:10

mreynolds389 approved these changes May 29, 2026

View reviewed changes

droideck requested changes May 29, 2026

View reviewed changes

jchapma force-pushed the hibp-pwd-validation branch from 39f3bd5 to 5e676b3 Compare June 2, 2026 15:46

droideck requested changes Jun 24, 2026

View reviewed changes

jchapma added 6 commits June 24, 2026 12:07

sourcery comments

812a5ab

config CI test

7c0cf88

fix aci bypass, mem leak, url validation

359995e

updated logging

cb6481d

mem ownership, HIBP buld check, admin bypass, default timeout

677d7bb

jchapma force-pushed the hibp-pwd-validation branch from c6815c1 to 677d7bb Compare June 24, 2026 12:42

Uh oh!

Conversation

jchapma commented May 12, 2026 • edited by sourcery-ai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by Sourcery

Uh oh!

sourcery-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mreynolds389 commented May 20, 2026

Uh oh!

packit-as-a-service Bot commented May 20, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jchapma commented May 12, 2026 •

edited by sourcery-ai Bot

Loading