Skip to content

fix: Enforce minimum range boundaries for security compliance (fixes #746)#753

Open
ymh1874 wants to merge 1 commit into
openstack-experimental:mainfrom
ymh1874:fix/746-password-expires-range
Open

fix: Enforce minimum range boundaries for security compliance (fixes #746)#753
ymh1874 wants to merge 1 commit into
openstack-experimental:mainfrom
ymh1874:fix/746-password-expires-range

Conversation

@ymh1874
Copy link
Copy Markdown
Collaborator

@ymh1874 ymh1874 commented Jun 4, 2026

This patch introduces runtime configuration validation rules ensuring that specific security compliance settings (like 'password_expires_days', 'disable_user_account_days_inactive', etc.) mirroring openstack/keystone python implementation.

The config crate has been updated to use the validator crate, and the ConfigManager now invokes validation on startup and file reloads.

Closes #746

@ymh1874 ymh1874 changed the title config: enforce minimum range boundaries for security compliance (fixes #746) fix: Enforce minimum range boundaries for security compliance (fixes #746) Jun 4, 2026
@gtema gtema force-pushed the fix/746-password-expires-range branch from cea2bcd to 309938c Compare June 5, 2026 14:27
@ymh1874 ymh1874 force-pushed the fix/746-password-expires-range branch 2 times, most recently from 1c06c23 to 774a1cd Compare June 5, 2026 14:39
Copy link
Copy Markdown
Collaborator

@gtema gtema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Would you mind adding a unittest (on the lib level and not under the security_compliance section) that explicitly tries to parse a config that would fail the validation ensuring the error is actually thrown. This way we can be sure that we prevent for accidental removing of the validation. I would be also interested to just to know whether the error message that the user will see will contain explanation of what is wrong or whether it is just a "failed validation" message with no further details. The scope is not to test validation of every single field, but to ensure the error message contain information useful to the user.

@ymh1874 ymh1874 force-pushed the fix/746-password-expires-range branch from 774a1cd to b9c2de4 Compare June 5, 2026 20:00
@ymh1874
Copy link
Copy Markdown
Collaborator Author

ymh1874 commented Jun 5, 2026

Thanks for the review!

I have added the unit test at the lib level (crates/config/src/lib.rs:test_invalid_security_compliance_validation). It writes an invalid configuration block containing 0 values to ensure that configuration parsing gets rejected as it should.

Regarding the error visibility: yes, the validator error message is implemented and provides clear context to the user.

Note:
During the CI run, you might notice that the integration test test_revoke_user_project_grant_auth_impact fails with a TokenRevoked error.

After looking at the trace logs, I found that this is a preexisting quirk in the test fixture surfaced by my new validation constraints. The test mocks an ApplicationCredential token with an issued_at timestamp hardcoded to the Unix Epoch (1970-01-01T00:00:00Z).

Because the configuration now strictly validates and enforces non-zero security lifetimes, the revocation engine evaluates this token against the RevokeProvider's default expiration_buffer (1800 seconds). Seeing that the token is effectively 56 years old, the system correctly flags it as invalid/revoked against recent database security events before the test can proceed.

Proposed Solution:
This can be easily resolved by updating the test fixture in tests/integration/src/assignment/grant/revoke.rs to use a current timestamp for issued_at instead of the Epoch date.

Let me know if you would like me to include that test update in this PR, or if you prefer to handle it in a separate issue!

@ymh1874 ymh1874 force-pushed the fix/746-password-expires-range branch from b9c2de4 to b11cdc3 Compare June 5, 2026 20:39
This patch introduces runtime configuration validation rules ensuring that
specific security compliance settings (like 'password_expires_days',
'disable_user_account_days_inactive', etc.) are explicitly greater than
zero.

Note: This contribution was developed with the help of AI.

Signed-off-by: Yousef Hussein <ymh1874@gmail.com>
@ymh1874 ymh1874 force-pushed the fix/746-password-expires-range branch from b11cdc3 to 1599ab3 Compare June 5, 2026 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

config.security_compliance.password_expires_days must be >0

2 participants