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

Add missing scanner params #293

Merged
merged 7 commits into from
Jul 24, 2020

Conversation

ArnoStiefvater
Copy link
Member

  • Add all openvas scanner parameters which are not openvas only parameters.
  • Add flag for for parameters so we have the possibility of not showing all parameters to clients.
  • Delete scanner only param drop_privileges from params dict.

Related PR:
greenbone/ospd#301

@codecov
Copy link

codecov bot commented Jul 17, 2020

Codecov Report

Merging #293 into ospd-openvas-20.08 will decrease coverage by 0.84%.
The diff coverage is 48.78%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           ospd-openvas-20.08     #293      +/-   ##
======================================================
- Coverage               82.71%   81.86%   -0.85%     
======================================================
  Files                       9        9              
  Lines                    1481     1511      +30     
======================================================
+ Hits                     1225     1237      +12     
- Misses                    256      274      +18     
Impacted Files Coverage Δ
ospd_openvas/preferencehandler.py 86.85% <33.33%> (-0.26%) ⬇️
ospd_openvas/daemon.py 62.24% <44.44%> (-0.67%) ⬇️
ospd_openvas/db.py 96.24% <55.00%> (-3.36%) ⬇️

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 1f9d6aa...86508a5. Read the comment docs.

Copy link
Member

@cfi-gb cfi-gb left a comment

Choose a reason for hiding this comment

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

Generally there seems to be an inconsistency between using spaces in front of the next lines, using spaces at the line and and no spaces at all.

In addition OpenVAS is used once where openvas is used in other parts.

Both should be made consistent.

CHANGELOG.md Outdated
@@ -22,6 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
managing the version information in ospd-openvas [#238](https://github.com/greenbone/ospd-openvas/pull/238)
- Pass store directory to OSPDaemon init [#266](https://github.com/greenbone/ospd-openvas/pull/266)
- Add URI field to results for file path or webservice URL [#271](https://github.com/greenbone/ospd-openvas/pull/271)
- Add elemtn to OSPD_PARAMS entries to indicate visiblitiy for client. [#293](https://github.com/greenbone/ospd-openvas/pull/293)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Add elemtn to OSPD_PARAMS entries to indicate visiblitiy for client. [#293](https://github.com/greenbone/ospd-openvas/pull/293)
- Add an element to OSPD_PARAMS entries to indicate visibility for client. [#293](https://github.com/greenbone/ospd-openvas/pull/293)

'mandatory': 0,
'visible_for_client': 0,
'description': (
'Is maximum number of hosts to test at the same time which'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'Is maximum number of hosts to test at the same time which'
'The maximum number of hosts to test at the same time which'

'description': (
'Is maximum number of hosts to test at the same time which'
+ 'should be given to the client (which can override it).'
+ 'This value must be computed given your bandwidth,'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ 'This value must be computed given your bandwidth,'
+ 'This value must be computed given your bandwidth,'

+ 'should be given to the client (which can override it).'
+ 'This value must be computed given your bandwidth,'
+ 'the number of hosts you want to test, your amount of'
+ 'memory and the horsepower of your processor(s).'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ 'memory and the horsepower of your processor(s).'
+ ' memory and the performance of your processor(s).'

'Is maximum number of hosts to test at the same time which'
+ 'should be given to the client (which can override it).'
+ 'This value must be computed given your bandwidth,'
+ 'the number of hosts you want to test, your amount of'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ 'the number of hosts you want to test, your amount of'
+ ' the number of hosts you want to test, your amount of'

'visible_for_client': 0,
'description': (
'Is maximum number of hosts to test at the same time which'
+ 'should be given to the client (which can override it).'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ 'should be given to the client (which can override it).'
+ ' should be given to the client (which can override it).'

'description': (
'Is maximum number of hosts to test at the same time which'
+ 'should be given to the client (which can override it).'
+ 'This value must be computed given your bandwidth,'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ 'This value must be computed given your bandwidth,'
+ ' This value must be computed given your bandwidth,'

'mandatory': 0,
'visible_for_client': 0,
'description': (
'is the number of plugins that will run against each host being'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'is the number of plugins that will run against each host being'
'The number of plugins that will run against each host being'

'visible_for_client': 0,
'description': (
'Name of the network interface that will be used as the source '
+ 'of connections established by OpenVAS. The scan won\'t be '
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ 'of connections established by OpenVAS. The scan won\'t be '
+ 'of connections established by openvas. The scan won\'t be '

As an alternative update the other occurrences of openvas to use OpenVAS

@ArnoStiefvater ArnoStiefvater force-pushed the scanner-params branch 3 times, most recently from e5b72aa to 6e5eb50 Compare July 20, 2020 08:47
@ArnoStiefvater ArnoStiefvater marked this pull request as ready for review July 20, 2020 08:51
jjnicola
jjnicola previously approved these changes Jul 21, 2020
drop_privileges preference should only be
settable via openvas config file.
Support all openvas settings which are not
strict openvas only settings.
We may run into problems on openvas side otherwise.
For example adding the empty string for the
ifaces_allow option would basically mean that we
do not allow any iface.
Copy link
Member

@jjnicola jjnicola left a comment

Choose a reason for hiding this comment

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

This requires still a fix to avoid sending all the preferences in the dictionary. Only the preferences received in the xml object as options + preferences return by openvas -s must be sent. This fix will be done with further PR.

@jjnicola jjnicola merged commit 73cc4e3 into greenbone:ospd-openvas-20.08 Jul 24, 2020
ArnoStiefvater pushed a commit to ArnoStiefvater/ospd-openvas that referenced this pull request Oct 25, 2021
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.

3 participants