Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Namespace generically named LDAP attributes #249

Merged
merged 3 commits into from
Oct 25, 2018
Merged

Namespace generically named LDAP attributes #249

merged 3 commits into from
Oct 25, 2018

Conversation

grawity
Copy link
Contributor

@grawity grawity commented Sep 13, 2018

I noticed that recently LDAP support was added to USBGuard, which adds some attributes including the generically named RuleTarget and RuleOrder.

Since attribute names have to be unique across the LDAP server, even between schemas (and no, the OIDs are mostly just for show), this has somewhat greater chance of colliding with someone's existing schema in the future.

So while the code is still warm, I would recommend prefixing those two attributes with USBGuard… or at least USB…, like the remaining ones. With namespacing, name collisions are much less likely.

@coveralls
Copy link

coveralls commented Sep 13, 2018

Coverage Status

Coverage remained the same at 57.411% when pulling 4e4f419 on grawity:ldap-namespace into 000123d on USBGuard:master.

Copy link
Member

@dkopecek dkopecek left a comment

Choose a reason for hiding this comment

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

Hello and thanks! Could you please rename it using "USBGuard" prefix rather than "USB". I think the "target" term should be USBGuard specific.

I'll add note to myself to review and correct all the names before the next release according to the following rules:

  1. Terms introduced by USBGuard shoud be prefixed with "USBGuard". (e.g. target, order, host...)
  2. Terms related to the USB specification / devices itself should be prefixed USB (id, serial, name, ....)

Feel free to correct any other occurrences of names that don't conform to these rules.

@grawity
Copy link
Contributor Author

grawity commented Sep 13, 2018

I noticed that there already is a USBGuardOrder attribute, which the source code doesn't use for anything. Was that supposed to be the same as RuleOrder?

@radosroka
Copy link
Member

It seems that USBGuardOrder was first and then there was an intention to rename it as RuleOrder. It is so for src/{Tests|Daemon|Common}/* files and scripts/ldap/* files seem to be not updated.

I think that it would be nice to have scripts/ldap/* files as primary location for ldap scripts and then we can have symlinks in tests to these scripts. Because now we have these scripts on two places and even not updated.

@radosroka
Copy link
Member

 std::vector<std::string> LDAPUtil::_rule_keys = {
    "RuleTarget", /* just for indexing */
    "USBGuardHost", /* just for indexing */
    "USBGuardOrder", /* just for indexing */
    "id",
    "serial",

In this case USBGuardOrder is just placeholder, there is no Order in regular (file based) rule.

To avoid possible collisions with other schemas loaded into the same
server, these attributes are namespaced:

    RuleCondition => USBGuardRuleCondition
    RuleOrder     => USBGuardRuleOrder
    RuleTarget    => USBGuardRuleTarget
./schema2ldif.pl usbguard < usbguard.schema > usbguard.ldif
@dkopecek dkopecek added this to the 0.7.x milestone Oct 25, 2018
@dkopecek
Copy link
Member

Thanks!

@dkopecek dkopecek merged commit 8c513bf into USBGuard:master Oct 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants