config/cfg_rules.ini: Make regexp's more POSIX compliant by arrowd · Pull Request #8227 · SSSD/sssd

arrowd · 2025-11-23T13:41:55Z

According to section 9.3.2 BRE Ordinary Characters [1], the '+' sequence may work as a plain character or like ERE special character. It is the latter for Linux, but it is the former for FreeBSD.

Instead, use '{1,}' as a portable way of expressing "one or many".

This fixes most of config validation tests on FreeBSD.

[1] https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/basedefs/V1_chap09.html#tag_09_03_02

gemini-code-assist

Code Review

This pull request improves the POSIX compliance of regular expressions in src/config/cfg_rules.ini by replacing the non-standard \+ quantifier with the more portable \{1,\}. This change is well-motivated, as it fixes configuration validation on platforms like FreeBSD where \+ is not supported in Basic Regular Expressions. The changes are correct and have been applied consistently. I've reviewed the modifications and find them to be a good improvement for portability. No issues were found.

arrowd · 2025-11-23T13:46:01Z

One test still fails even after this change:

[rule/allowed_domain_options]
validator = ini_allowed_options
section_re = ^\(domain\|application\)/[^/]\{1,\}$

It uses \| operator, which is also not guaranteed to work in BRE mode. FreeBSD man says:

Obsolete (“basic”) regular expressions differ in several respects.  ‘|’
is an ordinary character and there is no equivalent for its
functionality.

Not sure how to fix this properly given the rule itself is huge.

thalman · 2025-11-27T18:20:02Z

One test still fails even after this change:
[rule/allowed_domain_options]
validator = ini_allowed_options
section_re = ^$domain\|application$/[^/]\{1,\}$
It uses \| operator, which is also not guaranteed to work in BRE mode. FreeBSD man says:
Obsolete (“basic”) regular expressions differ in several respects.  ‘|’
is an ordinary character and there is no equivalent for its
functionality.
Not sure how to fix this properly given the rule itself is huge.

Hi, I did not look into the sssd code but it should be possible to use section_re multiple times like

section_re = ^application/[^/]\{1,\}$
section_re = ^domain/[^/]\{1,\}$

Does that help?

arrowd · 2025-11-27T18:56:29Z

The code that processes these rules is not in SSSD, but in ding-libs. Since it is a just an INI file, I don't think that listing the same key twice will append to its value, but I'll try your suggestion nevertheless.

alexey-tikhonov · 2025-11-27T20:13:40Z

but in ding-libs. Since it is a just an INI file, I don't think that listing the same key twice will append to its value

No, in 'libini_config' "last key wins".

alexey-tikhonov · 2025-11-27T20:15:57Z

Actually, we would like to port SSSD to use 'libeconf' instead of 'libini_config'.

But I don't know yet if 'libeconf' has any support for config validation (and what to do if it doesn't).

thalman · 2025-11-27T20:17:33Z

Actually, we would like to port SSSD to use 'libeconf' instead of 'libini_config'.

But I don't know yet if 'libeconf' has any support for config validation (and what to do if it doesn't).

Why we have this?:

sssd/src/config/cfg_rules.ini

Lines 14 to 20 in 637b7bc

    
           section_re = ^prompting/password/[^/\@]\+$ 
        
           section_re = ^prompting/2fa$ 
        
           section_re = ^prompting/2fa/[^/\@]\+$ 
        
           section_re = ^prompting/passkey$ 
        
           section_re = ^prompting/passkey/[^/\@]\+$ 
        
           section_re = ^domain/[^/\@]\+$ 
        
           section_re = ^domain/[^/\@]\+/[^/\@]\+$

alexey-tikhonov · 2025-11-28T08:25:18Z

Why we have this?:

sssd/src/config/cfg_rules.ini

Lines 14 to 20 in 637b7bc

There is an explanation in commit message of 4526858

arrowd · 2025-11-28T08:31:45Z

I think Tomáš meant "we already have duplicated keys and they seem to work". I'll dive into the code once I get free time.

alexey-tikhonov · 2025-11-28T08:47:31Z

JFTR: feature was added around SSSD/ding-libs@edf8547

thalman · 2025-11-28T08:57:34Z

Yes, exactly - we have duplicit keys and they are working, as Alexey mentioned that is supported by dinglibs.

alexey-tikhonov · 2025-11-28T09:08:30Z

Yes, exactly - we have duplicit keys and they are working, as Alexey mentioned that is supported by dinglibs.

Honestly, I don't know, just searched where feature was added.
But since SSSD uses it this way...

thalman · 2025-11-28T09:25:36Z

@arrowd - I added a label change requested, I assume that you will try to split and push the problematic rule.

arrowd · 2025-11-28T10:43:02Z

So, yes, it is an intended usage of libini: https://github.com/SSSD/ding-libs/blob/9078835b3c30bbd382d80f1b9e7dbc599e1190b5/ini/ini_configobj.h#L385-L393

thalman · 2025-11-28T11:50:10Z

It looks like the config_check_test_bad_appdomain_option_name unit test fails.

arrowd · 2025-11-28T17:38:09Z

Alas, the "ini_allowed_options" validator uses INI_GET_FIRST_VALUE when parsing section_re: https://github.com/SSSD/ding-libs/blob/9078835b3c30bbd382d80f1b9e7dbc599e1190b5/ini/ini_configobj.c#L1398-L1402

So we're stuck with \| again.

arrowd · 2025-11-29T05:37:13Z

We can modify ding-libs and teach validator to use multiple re's when matching a rule.

Or we can modify ding-libs to use ERE's instead of BRE's. Unfortunately, either way requires modifications to ding-libs, making a new release and then waiting for it to be packaged downstreams.

thalman · 2025-12-01T10:36:40Z

We can modify ding-libs and teach validator to use multiple re's when matching a rule.

Or we can modify ding-libs to use ERE's instead of BRE's. Unfortunately, either way requires modifications to ding-libs, making a new release and then waiting for it to be packaged downstreams.

As Alexey mentioned, we are moving away from ding-libs. It does not make much sense to invest into it.

alexey-tikhonov · 2025-12-01T10:38:51Z

Is there anything "merge-able" in this PR without ding-libs update? (Sorry I didn't look into patch details)

thalman · 2025-12-01T10:44:07Z

@alexey-tikhonov yes, all the updates are fine. I will check why splitting the one with (|) is not working. IMO it should since we have that in other sections.

thalman · 2025-12-01T13:52:30Z

For the sake of future development I spent few minutes investigating this. Multiple section_re are allowed where validator = ini_allowed_sections. For allowed_domain_options we use validator = ini_allowed_options which indeed does not support multiple section_re.

The only solution available at the moment for you (@arrowd) is to duplicate whole section into [rule/allowed_domain_options] and [rule/allowed_application_options] with two individual section_re. But that is something I do not want upstream, maybe some packaging script can do it until we move away from ding-lib.

Otherwise it looks good, ACK

aplopez · 2025-12-02T12:57:49Z

@arrowd Are you OK with the proposed solution?

arrowd · 2025-12-02T12:59:10Z

Yes, I will arrange the CI appropriately.

aplopez

LGTM

According to section 9.3.2 BRE Ordinary Characters [1], the '\+' sequence may work as a plain character or like ERE special character. It is the latter for Linux, but it is the former for FreeBSD. Instead, use '{1,}' as a portable way of expressing "one or many". This fixes most of config validation tests on FreeBSD. [1] https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/basedefs/V1_chap09.html#tag_09_03_02 Reviewed-by: Alejandro López <[email protected]> Reviewed-by: Tomáš Halman <[email protected]>

sssd-bot · 2025-12-02T13:09:16Z

The pull request was accepted by @aplopez with the following PR CI status:

There are unsuccessful or unfinished checks. Make sure that the failures are not related to this pull request before merging.

gemini-code-assist Bot reviewed Nov 23, 2025

View reviewed changes

alexey-tikhonov added the no-backport This should go to target branch only. label Nov 24, 2025

alexey-tikhonov requested a review from thalman November 27, 2025 13:46

alexey-tikhonov assigned thalman Nov 27, 2025

alexey-tikhonov added the Trivial A single reviewer is sufficient to review the Pull Request label Nov 27, 2025

alexey-tikhonov requested a review from aplopez November 27, 2025 13:46

alexey-tikhonov assigned aplopez Nov 27, 2025

thalman added the Changes requested label Nov 28, 2025

thalman added Waiting for review and removed Changes requested labels Nov 28, 2025

arrowd force-pushed the posix-regexp-rules branch from 3e95c81 to b39e87c Compare November 28, 2025 17:38

thalman approved these changes Dec 1, 2025

View reviewed changes

aplopez approved these changes Dec 2, 2025

View reviewed changes

aplopez added Accepted and removed Waiting for review labels Dec 2, 2025

sssd-bot force-pushed the posix-regexp-rules branch from b39e87c to 8465579 Compare December 2, 2025 13:09

aplopez merged commit b0af250 into SSSD:master Dec 2, 2025
18 checks passed

arrowd deleted the posix-regexp-rules branch December 26, 2025 15:03

Uh oh!

Conversation

arrowd commented Nov 23, 2025

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

arrowd commented Nov 23, 2025

Uh oh!

thalman commented Nov 27, 2025

Uh oh!

arrowd commented Nov 27, 2025

Uh oh!

alexey-tikhonov commented Nov 27, 2025

Uh oh!

alexey-tikhonov commented Nov 27, 2025

Uh oh!

thalman commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-tikhonov commented Nov 28, 2025

Uh oh!

arrowd commented Nov 28, 2025

Uh oh!

alexey-tikhonov commented Nov 28, 2025

Uh oh!

thalman commented Nov 28, 2025

Uh oh!

alexey-tikhonov commented Nov 28, 2025

Uh oh!

thalman commented Nov 28, 2025

Uh oh!

arrowd commented Nov 28, 2025

Uh oh!

thalman commented Nov 28, 2025

Uh oh!

arrowd commented Nov 28, 2025

Uh oh!

arrowd commented Nov 29, 2025

Uh oh!

thalman commented Dec 1, 2025

Uh oh!

alexey-tikhonov commented Dec 1, 2025

Uh oh!

thalman commented Dec 1, 2025

Uh oh!

thalman commented Dec 1, 2025

Uh oh!

aplopez commented Dec 2, 2025

Uh oh!

arrowd commented Dec 2, 2025

Uh oh!

aplopez left a comment

Choose a reason for hiding this comment

Uh oh!

sssd-bot commented Dec 2, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thalman commented Nov 27, 2025 •

edited

Loading