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

Fix how MAC addresses are handled by the rules parser #2176

Closed

Conversation

lel-amri
Copy link

@lel-amri lel-amri commented Nov 14, 2023

Fixes #2175.

It wasn't ignoring separator characters such as the colon and hyphen. The rules compiler automatically add a colon to separate bytes, which is not compatible with how they are parsed.


This is currently a draft, as I'm not particularly versed in C++, and I think I couldn't fit well with the style used at Zerotier Inc.

@lel-amri lel-amri force-pushed the fix-mac-handling-in-rules-parser branch from 0d2a956 to ae6aef9 Compare November 15, 2023 15:50
It wasn't ignoring separator characters such as the colon and hyphen.
The rules compiler automatically add a colon to separate bytes, which is
not compatible with how they are parsed.
@lel-amri lel-amri force-pushed the fix-mac-handling-in-rules-parser branch from ae6aef9 to 1aa31e0 Compare November 15, 2023 15:51
@laduke
Copy link
Contributor

laduke commented Nov 15, 2023

Thanks for doing this. We will try to test it soon. Do you have a quick sample ruleset that shows the problem?

@lel-amri
Copy link
Author

I think a direct and minimal test would be the following ruleset:

drop macsrc AA:AA:AA:AA:AA;
accept;

which compiles to the following JSON:

{
 "rules": [
  {
   "type": "MATCH_MAC_SOURCE",
   "not": false,
   "or": false,
   "mac": "aa:aa:aa:aa:aa"
  },
  {
   "type": "ACTION_DROP"
  },
  {
   "type": "ACTION_ACCEPT"
  }
 ],
 "capabilities": [],
 "tags": []
}

Because of the bug, host AA:AA:AA:AA:AA is not filtered.

The following JSON should work though:

{
 "rules": [
  {
   "type": "MATCH_MAC_SOURCE",
   "not": false,
   "or": false,
   "mac": "aaaaaaaaaa"
  },
  {
   "type": "ACTION_DROP"
  },
  {
   "type": "ACTION_ACCEPT"
  }
 ],
 "capabilities": [],
 "tags": []
}

@joseph-henry
Copy link
Contributor

Unfortunately I don't think I can merge this because the result of cleanMac (which is void) was being passed to the constructor of std::string which cannot work. I've pulled your changes from your fork and made a new branch with additional fixes. We still need to do a proper end-to-end test.

Closing this PR.

@lel-amri
Copy link
Author

lel-amri commented Mar 5, 2024

Thanks for working on this :)
Indeed I overlooked that, and I did not run the code.
Also, maybe doing the operation in-place is not a good design, I'm not sure.

@joseph-henry
Copy link
Contributor

Thanks for the effort and we can still work on merging in your change with the extra fix on the other branch. Feel free to test it.

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.

macsrc and macdest matches values are improperly parsed
3 participants