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

[Endpoints] Change size of protocol + [Nexpose] consolidation of protocols #4696

Merged
merged 4 commits into from
Jun 22, 2021

Conversation

kiblik
Copy link
Contributor

@kiblik kiblik commented Jun 21, 2021

No description provided.

@@ -84,19 +84,19 @@ def test_nexpose_parser_has_many_finding(self):
finding = findings[29]
self.assertIn("Backup Exec Agent Browser", finding.description)
self.assertEqual("backup-exec-agent-browser", finding.unsaved_tags[0])
self.assertEqual("backup-exe", finding.unsaved_endpoints[0].protocol)
self.assertEqual("backup-exec-agent-br", finding.unsaved_endpoints[0].protocol)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@damiencarol, 20 still doesn't cover this use-case. But it is really really edge case

Copy link
Contributor

Choose a reason for hiding this comment

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

Something is wrong with this parser and I'm glad that the new code on endpoints found it.
Take a look at the data:

                           <endpoint protocol="tcp" port="6101" status="open">
					<services>
						<service name="Backup Exec Agent Browser">
							<configuration>
								<config name="sslv3">false</config>
								<config name="tlsv1_0">false</config>
								<config name="tlsv1_1">false</config>
								<config name="tlsv1_2">false</config>
							</configuration>
							<tests>
							</tests>
						</service>
					</services>
				</endpoint>

The protocol value should be tcp and not some fake value backup-exec-agent-browser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, yes and no. What about this case?

<endpoint protocol="tcp" port="80" status="open">
  <services>
    <service name="HTTP">

Or this

<endpoint protocol="tcp" port="22" status="open">
  <services>
    <service name="SSH">

Or you can compare it with this

<endpoint protocol="udp" port="53" status="open">
  <services>
      <service name="DNS">

I like this format: dns://192.168.1.1#tcp and dns://192.168.1.1#udp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use (in Nexpose parser) checking of existence in hyperlink._url.SCHEME_PORT_MAP like here and here
So, it is exist in hyperlink._url.SCHEME_PORT_MAP, use it, otherwise, use tcp or udp

Copy link
Contributor

Choose a reason for hiding this comment

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

Saw your commit, it's good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your comment are valid. Parsing protocol is tricky. I'm not surprised that the module hyperlink have internal mapping.

@damiencarol damiencarol changed the title Change site of Endpoint.protocol [Endpoints] Change size of protocol Jun 21, 2021
@kiblik kiblik changed the title [Endpoints] Change size of protocol [Endpoints] Change size of protocol + [Nexpose] consolidation of protocols Jun 21, 2021
@damiencarol damiencarol added this to the 2.0.0 milestone Jun 21, 2021
@damiencarol damiencarol requested review from valentijnscholten and a team June 21, 2021 21:23
@valentijnscholten valentijnscholten added the New Migration Adding a new migration file. Take care when merging. label Jun 22, 2021
@valentijnscholten
Copy link
Member

@kiblik can you rebase migration?

@kiblik
Copy link
Contributor Author

kiblik commented Jun 22, 2021

@valentijnscholten, rebased and db_mig file moved

@kiblik
Copy link
Contributor Author

kiblik commented Jun 22, 2021

tests failed but not because of my changes

@valentijnscholten valentijnscholten merged commit 128a1b7 into DefectDojo:dev Jun 22, 2021
@kiblik kiblik deleted the endpoint_proto branch August 18, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy-review enhancement New Migration Adding a new migration file. Take care when merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants