Skip to content
This repository has been archived by the owner on Nov 29, 2021. It is now read-only.

Add new interface for adding alive test methods #329

Merged
merged 7 commits into from
Oct 14, 2020

Conversation

ArnoStiefvater
Copy link
Member

@ArnoStiefvater ArnoStiefvater commented Aug 31, 2020

What:

Instead of using a bit field which entails all alive tests we can now also use xml elements for every alive test method.

Depends on:
greenbone/ospd-openvas#331

Why:

Better usability.

How:

Tested with python-gvm and following target description with different configurations.

<target>
    <alive_test_methods>
        <icmp>0</icmp>
        <tcp_ack>0</tcp_ack>
        <tcp_syn>0</tcp_syn>
        <arp>0</arp>
        <consider_alive>0</consider_alive>
    </alive_test_methods>
    <hosts>127.0.0.1</hosts>
</target>

Checklist:

Instead of using a bit flag which entails all
alive tests we can now also use xml elements for
every alive test method.
@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #329 into master will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
+ Coverage   73.52%   73.74%   +0.21%     
==========================================
  Files          23       23              
  Lines        2531     2552      +21     
==========================================
+ Hits         1861     1882      +21     
  Misses        670      670              
Impacted Files Coverage Δ
ospd/protocol.py 89.56% <100.00%> (+2.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 532169f...eaa4f32. Read the comment docs.

@ArnoStiefvater ArnoStiefvater marked this pull request as ready for review October 13, 2020 11:17
ospd/protocol.py Outdated
@@ -146,6 +146,32 @@ def process_credentials_elements(cred_tree: Element) -> Dict:

return credentials

@staticmethod
def process_alive_test_methods(cred_tree: Element, options: Dict) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a more descriptive param name.

Suggested change
def process_alive_test_methods(cred_tree: Element, options: Dict) -> None:
def process_alive_test_methods(alive_test_tree: Element, options: Dict) -> None:

</alive_test_methods>
"""
for child in cred_tree:
if child.tag == 'icmp':
Copy link
Member

Choose a reason for hiding this comment

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

Are these methods optional? shouldn't be added only if they are set? Not sure if it make sense to add and empty element to the options dict.

Copy link
Member Author

Choose a reason for hiding this comment

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

All the options (imcp, tcp_ack ...) are optional. For example if you do not want icmp then you can either leave it out completely, set it to '0' or leave it empty. Everything not '1' is treated as not wanted.

Should I check if child.text is None and not include it in the options dict?

Just an additional remark. If <alive_test> and <alive_test_methods> is provided, <alive_test> takes precedence over <alive_test_methods>.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have now added a test for None. So if there is no text it will not be included options dict.
I also changed the argument description in the function.

@jjnicola
Copy link
Member

Could you add a test ? in tests/test_scan_and_results.py there are already some tests for target options.

@jjnicola jjnicola merged commit df823da into greenbone:master Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants