aquasecurity / kube-hunter

Hunt for security weaknesses in Kubernetes clusters
Apache License 2.0
4.66k stars 578 forks source link

modify error when cluster resources unreachable #478

Closed DiptoChakrabarty closed 2 years ago

DiptoChakrabarty commented 2 years ago

Description

Please include a summary of the change and which issue is fixed. Also include relevant motivation and context. List any dependencies that are required for this change.

Modifies the Error message when cluster resources are unreachable from kube hunter

Contribution Guidelines

Please Read through the Contribution Guidelines.

Fixed Issues

Please mention any issues fixed in the PR by referencing it properly in the commit message. As per the convention, use appropriate keywords such as fixes, closes, resolves to automatically refer the issue. Please consult official github documentation for details.

Fixes #477

"BEFORE" and "AFTER" output

To verify that the change works as desired, please include an output of terminal before and after the changes under headings "BEFORE" and "AFTER".

BEFORE

Any Terminal Output Before Changes. Kube Hunter couldn't find any clusters

AFTER

Any Terminal Output Before Changes. Cluster Inaccessible from Kube Hunter

Contribution checklist

Notes

Please mention if you have not checked any of the above boxes. As tests are not required for this.

danielsagi commented 2 years ago

Hi @DiptoChakrabarty Thats a really good change, I thought about changing this behavior. However, I believe in order for this error message to be more indicative, we need to add some mechanism to determine why kube hunter failed to discover the cluster.

The most common problems that cause kube hunter to fail are:

  1. Ports are not open from where kube-hunter was ran

  2. Ports are open, but kube-hunter could not identify a kubernetes service behind them

  3. Failed dns resolution

  4. The ip is not reachable (not very easy to distinguish from ports not open, because ping requests might be blocked anyway)

So if we were to change the message to be more indicative I want that to be by adding those 3 messages.

Do you think this is something you want to take on yourself? This would be wonderful :) I would help in the process with questions of course.

The general process of identifying the specific problem could be by checking the port open event thats fired if we found an open port.

DiptoChakrabarty commented 2 years ago

Yes @danielsagi I would like to try it out , I think the ports issue is visible when we run it with debug log flag so maybe can get some pointers from there Ill check it out

DiptoChakrabarty commented 2 years ago

hey @danielsagi I am able to log out the errors could you give me some pointers on how I could display these errors , I know that the final error finally comes from here https://github.com/aquasecurity/kube-hunter/blob/473e4fe2b57d3e91b91777de0ed65d13658ae9cb/kube_hunter/modules/report/plain.py#L16-L46 . How can I propagate the error here

DiptoChakrabarty commented 2 years ago

Selection_138 Hey @danielsagi I am able to pass errors around and getting the port not open message , however that is only because I do a publish_event of HuntFinished in the port discovery class as huntfinished of main does not pickup the PortDiscovery as caller in the publish_event.

I was thinking how I could pass the PortDiscovery to teh HuntFinished of the main function which would give the required output.

Currently if a port or service is not discovered no event is published due to which SendFullReport which picks up from HuntFinished does not have any details of the error , if there was a publish event type we could run which would directly go to Huntfinished I think it could solve the problem.

danielsagi commented 2 years ago

Hi @DiptoChakrabarty Glad to see you working on this! Ill be free at the end of this week to help you, please be patient and well work on this together :)

DiptoChakrabarty commented 2 years ago

Hi @DiptoChakrabarty Glad to see you working on this! Ill be free at the end of this week to help you, please be patient and well work on this together :)

Okay Sure :+1:

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

DiptoChakrabarty commented 2 years ago

@danielsagi can you check the commits once , I am able to print the error and propagate it but stuck on the last step

DiptoChakrabarty commented 2 years ago

Yeah I am looking through it , it sounds good I am trying to understand it a bit more and then working on it

DiptoChakrabarty commented 2 years ago

It is working now I think , getting such output at the end Selection_159

DiptoChakrabarty commented 2 years ago

Hi @DiptoChakrabarty Looking at this for the second time, I think we have been looking at this wrongly. The hunt error basically serves as more advanced logging. (which just spams messages, as the error is triggered for every failed try, which happens alot in a network scan) while we want to make a decision at the end of the hunt, based on what we found.

Want we need is to decide at the end based on already published events, what happend in the scan. sorry to walk through this again from the start, but I tested it myself and the following method works wonderfully

  1. Remove the hunt error completely :(
  2. Collect OpenPortEvent at the collector (like we did last time with the HuntError)
  3. At the reporter, just adding logic to the output, something like this:
with open_ports_lock:
    open_ports_len = len(open_ports)
failed_reasons = list()

if not open_ports_len:
    failed_reasons += ["Could not find any open kubernetes ports"]

output += "\nKube Hunter couldn't find any clusters"
if len(failed_reasons):
    output += "\n\nPossible reasons:"
    for i, reason in enumerate(failed_reasons):
        output += f"\n{i+1}. {reason}"

I think I get the idea , I am working on it , I will publish the OpenPortEvent and catch it in the collector and then report it in a similar fashion in case of error.

DiptoChakrabarty commented 2 years ago

Does this look good , made use of the OpenPortEvent only Selection_167

DiptoChakrabarty commented 2 years ago

Hey @danielsagi can you once review please

DiptoChakrabarty commented 2 years ago

hey can you please check it out once please

danielsagi commented 2 years ago

Hi @DiptoChakrabarty I'm sorry I can't sit with you this one out. you probably didn't understand my comments regarding the Error messages. I meant you should not add published events of open port event.

only add logic on messages at the collector by subscribing. In your PR you basically added publishing of OpenPortEvent s even on non related port scanning hunters.

I'm closing this PR for now, if you want to address my comments as I stated in a new PR. I could help you again. We still would love to see this implemented :)