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

Feat: Introduce net-localgroup net-user and route-print parsers #602

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

joehacksalot
Copy link
Contributor

@joehacksalot joehacksalot commented Oct 1, 2024

This PR introduces a new parsers for net-localgroup, net-user and route-print[1] windows commands.

Tested on:

Windows XP
Windows 7
Windows 10
Windows 11
Windows Server 2008
Windows Server 2016

Note:
[1] - The 'metric' fields are generally integers, and I was processing them as such but found one instance where the value was the string "Default", which after some research, would require looking up the OS default metric currently set to identify the numerical value. For this reason, I changed the schema type to string and left the original value for the end user to interpret.

This closes #600

@joehacksalot joehacksalot changed the base branch from master to dev October 1, 2024 19:48
@kellyjonbrazil
Copy link
Owner

This PR seems to have other unrelated files in it (e.g. ipconfig.py and associated test files). Could you re-open this PR with a clean set of files?

@kellyjonbrazil kellyjonbrazil added the question Further information is requested label Oct 20, 2024
@joehacksalot
Copy link
Contributor Author

I'll get those removed. I branched off of the wrong branch when I started. It'll be up soon

@joehacksalot
Copy link
Contributor Author

ok. should be ready now

Copy link
Owner

@kellyjonbrazil kellyjonbrazil left a comment

Choose a reason for hiding this comment

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

Thanks for the contributions! Good stuff - just a few changes and suggestions.

author = 'joehacksalot'
author_email = 'joehacksalot@gmail.com'
compatible = ['windows']
magic_commands = ['net-localgroup']
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be magic_commands = ['net localgroup']

Comment on lines +33 to +39
Notes:
[0] - 'lease_expires' and 'lease_obtained' are parsed to ISO8601 format date strings. if the value was unable
to be parsed by datetime, the fields will be in their raw form
[1] - 'autoconfigured' under 'ipv4_address' is only providing indication if the ipv4 address was labeled as
"Autoconfiguration IPv4 Address" vs "IPv4 Address". It does not infer any information from other fields
[2] - Windows XP uses 'IP Address' instead of 'IPv4 Address'. Both values are parsed to the 'ipv4_address'
object for consistency
Copy link
Owner

Choose a reason for hiding this comment

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

I think this section can be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the information might be helpful to some users. I can delete it if you think that's best.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I thought this was left over from the ipconfig parser. I didn't see where these footnotes were referenced in the Schema, so it didn't seem to belong here.

Comment on lines 60 to 61
$ net localgroup Administrators | jc --net-localgroup -p | jq
$ net localgroup /domain | jc --net-localgroup -p | jq
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like these examples are missing. no need to pipe through jq when using the -p option.

Usage (module):

import jc
result = jc.parse('net-localgroup', net_localgroup_command_output)
Copy link
Owner

Choose a reason for hiding this comment

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

Though 'net-localgroup' will work here, it is best to use 'net_localgroup' for python module examples.

jc/parsers/net_user.py Outdated Show resolved Hide resolved
}

Notes:
- The `metric` field is typically an integer but can sometimes be set to "Default"
Copy link
Owner

Choose a reason for hiding this comment

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

Wonder if there is another way of handling this? Could a metric of -1 or even null/None be the "Default"? Or another field called metric_is_default set to True or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When "Default" is present, the interface is using the value currently set as operating system's default metric value. I believe this value is stored somewhere in the registry.

I'm not opposed to a -1 for default. I think I'd prefer that over using null/None since I usually assume if its null/None that it was not present in the original output.

Copy link
Contributor Author

@joehacksalot joehacksalot Nov 25, 2024

Choose a reason for hiding this comment

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

I started looking into doing this and I found an example where a metric was already set to -1.

          {
            "interface": 0,
            "metric": "4294967295",
            "network_destination": "2001:db8::/64",
            "gateway": "fe80::1"
          }

I only googled quickly, but it looks like it may be used for Windows to automatically optimize routing.

Leaving it as a string for now. Open to discuss further.

Copy link
Owner

Choose a reason for hiding this comment

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

Just seems a shame to have this be a string field because of that one possible value. I sorta like the idea of having it be null/None if it is default and then set the metric_is_default field to True. A bit more work, but fixes this type override issue.

@@ -0,0 +1,565 @@
r"""jc - JSON Convert `route-print` command output parser
Copy link
Owner

Choose a reason for hiding this comment

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

should be route print

author = 'joehacksalot'
author_email = 'joehacksalot@gmail.com'
compatible = ['windows']
magic_commands = ['route-print']
Copy link
Owner

Choose a reason for hiding this comment

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

should be 'route print'

return raw_output if raw else _process(raw_output)
except Exception as e:
if not quiet:
jc.utils.warning_message(['Could not parse data due to unexpected format.'])
Copy link
Owner

Choose a reason for hiding this comment

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

This type of exception handling can make it difficult to troubleshoot. Doing this means the user cannot use jc --route-print -dd to get debug info. Without the exception handling jc should just do the right thing. If you want a custom exception message, it would be better to use jc.exceptions.ParseError:

from jc.exceptions import ParseError
raise ParseError('My custom error here.')

You can find examples in the following parsers: cef.py, csv.py, uname.py, etc.

return raw_output if raw else _process(raw_output)
except Exception as e:
if not quiet:
jc.utils.warning_message(['Could not parse data due to unexpected format.'])
Copy link
Owner

Choose a reason for hiding this comment

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

This type of exception handling can make it difficult to troubleshoot. Doing this means the user cannot use jc --route-print -dd to get debug info. Without the exception handling jc should just do the right thing. If you want a custom exception message, it would be better to use jc.exceptions.ParseError:

from jc.exceptions import ParseError
raise ParseError('My custom error here.')

You can find examples in the following parsers: cef.py, csv.py, uname.py, etc.

@kellyjonbrazil kellyjonbrazil added reviewing and removed question Further information is requested labels Nov 19, 2024
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be deleted? why do you need that file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the unit test

Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need that file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is part of the unit test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Parser: net localgroup and net user on windows
3 participants