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 Monero protocol classification. #2196

Merged
merged 1 commit into from
Dec 13, 2023

Conversation

utoni
Copy link
Collaborator

@utoni utoni commented Dec 5, 2023

Not sure if it makes sense to unify monero.c detection patterns with the one in mining.c. Seems like the JSON protocol for ZCash/Monero look pretty similar.

@utoni utoni force-pushed the add/monero-protocol-dissector branch from e64aad6 to b132c02 Compare December 5, 2023 06:47
@IvanNardi
Copy link
Collaborator

Not sure if it makes sense to unify monero.c detection patterns with the one in mining.c. Seems like the JSON protocol for ZCash/Monero look pretty similar.

@mmaatuq, any thoughts?

@mmaatuq
Copy link
Contributor

mmaatuq commented Dec 5, 2023

Not sure if it makes sense to unify monero.c detection patterns with the one in mining.c. Seems like the JSON protocol for ZCash/Monero look pretty similar.

@mmaatuq, any thoughts?

yes, it's safe to do that, it was in my pipeline as most of the RPC calls are related to blockchain exploration not mining, also the pattern inside mining.c for ZCash/Monero is too generic it doesn't specify any rpc methode.

@utoni
Copy link
Collaborator Author

utoni commented Dec 5, 2023

@mmaatuq
So it is safe to remove the whole mining.c file?

@mmaatuq
Copy link
Contributor

mmaatuq commented Dec 5, 2023

@mmaatuq So it is safe to remove the whole mining.c file?

I think so.

@utoni utoni force-pushed the add/monero-protocol-dissector branch 3 times, most recently from 021f146 to 0f79e95 Compare December 5, 2023 09:43
@IvanNardi
Copy link
Collaborator

@mmaatuq, is it fine for you?

@mmaatuq
Copy link
Contributor

mmaatuq commented Dec 5, 2023

@mmaatuq, is it fine for you?

yes, looks good.

@lucaderi
Copy link
Member

lucaderi commented Dec 5, 2023

This PR removed the currency that is used by some people, so under the hood, you can change the code but the output must stay rich, hence I think you should preserve the currency name.

@IvanNardi
Copy link
Collaborator

Let me try to understand better... Let start with two simple questions
AFAIK from previous discussions, we have NDPI_PROTOCOL_MINING for the generic "mining" stuff and specific protocols (ETHEREUM, BITCOIN,...) for the "transactions".
First question: is that correct? @mmaatuq
If so, does this new dissector aim to identify Monero mining or Monero transaction, @utoni ?

@utoni
Copy link
Collaborator Author

utoni commented Dec 7, 2023

I think the dissector (and pcap) files are related to the node discovery thingy. I did not record any pcaps for mining/transactions yet.

@IvanNardi
Copy link
Collaborator

I think the dissector (and pcap) files are related to the node discovery thingy. I did not record any pcaps for mining/transactions yet.

Ok, so it makes sense to have a new/distinct dissector.
But, and that is my main issue, if the existing mining dissector still identifies mining stuff (even if it is not able to tell zcash from monero) I think that we should keep it (with its metadata) and keep having different logic for generic mining and specific cryptocurrency transaction/discovery/...

@mmaatuq
Copy link
Contributor

mmaatuq commented Dec 7, 2023

Let me try to understand better... Let start with two simple questions AFAIK from previous discussions, we have NDPI_PROTOCOL_MINING for the generic "mining" stuff and specific protocols (ETHEREUM, BITCOIN,...) for the "transactions". First question: is that correct? @mmaatuq If so, does this new dissector aim to identify Monero mining or Monero transaction, @utoni ?
yes correct,
to elaborate more on this

  • patterns that were initially inside mining.c and moved to Bitcoin and Ethereum are related to the generic work of any blockchain node software that tries to connect to the blockchain network(discovery) and publish, and receive transactions, no pattern is specific to mining. The same goes for Monero/Zcash pattern it's generic and a lot of Monero rpc calls will have this pattern.

  • detect mining needs new dissectors/sub dissectors for each coin e.g (by looking for rpc calls that specifically are sent from a mining software like getblocktemplate or mining protocols like stratum in case of bitcoin.

@IvanNardi
Copy link
Collaborator

IvanNardi commented Dec 9, 2023

Proposal: add the new Monero dissector and keep the existing mining code. This way we add dissection capabilities without loosing anything.
Then, later, we might take a closer look at the mining stuff

@utoni utoni force-pushed the add/monero-protocol-dissector branch 2 times, most recently from 58de133 to ca3247f Compare December 13, 2023 16:58
@utoni utoni requested a review from IvanNardi December 13, 2023 17:00
Copy link
Collaborator

@IvanNardi IvanNardi left a comment

Choose a reason for hiding this comment

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

Could you:

  • update /doc/protocols.rst
  • restore the zcash.pcap test

please?

@utoni utoni force-pushed the add/monero-protocol-dissector branch from ca3247f to 76792d8 Compare December 13, 2023 18:06
@utoni utoni force-pushed the add/monero-protocol-dissector branch from 76792d8 to d210338 Compare December 13, 2023 18:10
Copy link

sonarcloud bot commented Dec 13, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.2% Duplication on New Code

See analysis details on SonarCloud

@IvanNardi IvanNardi merged commit ef62391 into ntop:dev Dec 13, 2023
33 checks passed
@IvanNardi
Copy link
Collaborator

Thanks everyone!

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.

4 participants