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

Adds/corrects more SNMP sysDescr fingerprints #58

Merged
merged 7 commits into from
May 29, 2015

Conversation

jhart-r7
Copy link
Contributor

@jhart-r7 jhart-r7 assigned jhart-r7 and hdm and unassigned jhart-r7 and hdm May 22, 2015
<description>Digital/Compaq/HP Tru64 Unix</description>
<example>clima.igpsdfpe COMPAQ AlphaServer DS20E 500 MHz Digital UNIX V4.0F (Rev. 1229); Mon Apr 12 13:30:41 EDT 1999 TCP/IP</example>
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the driver for removing these examples?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference is to have as many examples as possible that test what the regular expression is designed to handle but as few as necessary. All of these are basically the same in that they don't each test something particular in the fingerprint that others don't. For example, if all that differs between two examples it the version and the hostname, and the regular expression captures both fields the same way in both examples, there is little point in having the second example.

There is also a performance hit here. Part of the unit tests test that any given example only matches the fingerprint in which it was specified, which means trying to match every example against every fingerprint pattern specified before its enclosing fingerprint.

I don't believe we are losing any test coverage here but feel free to point out any instances that you see.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it thanks, will eyeball them once more for unique test cases that might have been lost

Copy link
Contributor

Choose a reason for hiding this comment

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

A few unique examples are lost, but they don't make a difference to the pattern.

hdm pushed a commit that referenced this pull request May 29, 2015
Adds/corrects more SNMP sysDescr fingerprints
@hdm hdm merged commit 63e0d6f into rapid7:master May 29, 2015
@jhart-r7 jhart-r7 deleted the bug/snmp-stuff branch August 21, 2015 16:13
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.

None yet

2 participants