fix nit in #130 #166
fix nit in #130 #166
Conversation
|
I made it like that but I think we need to decide what should be the final call for this case -p should overwrite the existing port or not? |
|
@agaurav77 Your fix will overwrite this github.com:33 -p 333,3333 to github.com:333 and github.com:3333 |
|
@jinankjain You're right, but there is a side case as well. When the user has specified a port via -p and not specified it via ip:port, then I think he expects the port provided via -p to override the default value (22). |
|
I think we should append extra ports specified via -p (as it is right now), but also handle the case when ssh_scan makes ip = ip:22 when the user didn't specify the port via the colon notation, and specified it instead via -p. So, this might need more change. |
|
We should handle this case too github.com:22 -p 33,333 |
ashishgaurav13
force-pushed
the
130nit
branch
3 times, most recently
from
1410ce4 to
af93eb0
Compare
|
@jinankjain How about this? This gives
|
|
@agaurav77 We could keep all these sockets in a set instead of array so that we don't have to worry about duplicate sockets entries. |
|
@jinankjain Good point, I'll use Set for the IP Range then :) EDIT : Never mind the previous edit, I guess I'll keep everything Set just to be consistent. |
ashishgaurav13
force-pushed
the
130nit
branch
2 times, most recently
from
2b38921 to
696be6d
Compare
ashishgaurav13
force-pushed
the
130nit
branch
from
696be6d to
fa0b063
Compare
|
@jinankjain Done :) |
| # Remove instances of ip, where ip:port already exist. | ||
| # This would mean that port was somehow passed. Otherwise, | ||
| # make ip = ip:22 | ||
| def sanitizeIPRange(ip_range) |
There was a problem hiding this comment.
@agaurav77 Do we need this after introducing set?
There was a problem hiding this comment.
@jinankjain yes
Maybe I got you confused. Let me clarify once more.
Right now, when you pass github.com -p 99, it checks github.com:22, github.com:99, which is a side effect of appending ports in -p to the targets given in -t. Since the default port is 22, the first time you were doing enumerateIPRange, it would convert github.com to github.com:22, and then simply append 99 to the host github.com again. But then this 22 is kind of redundant.
The need for my logic stems from the fact that there is (maybe) no way to check whether -p was passed within -t block in OptionParser's code. Thus, enumerate in my PR converts github.com -p 99 to github.com, github.com:99, and then sanitizes it to remove github.com since a github.com:port argument is present. Maybe there are shorter ways to do this, and please feel free to indicate them, I'd be more than happy to change my logic to something shorter.
Sorry for the long explanation :)
|
@jinankjain @agaurav77 glad you guys are tracking this down. I think the easiest way to handle this could be to make socket specifications via -t incompatible with -p to make it easier. I think the scenarios are getting a little unwieldy and it becomes hard at a point for a new user to understand the implications of joining socket specifications. The other option, which seems to make sense to me is that when you specify a target with an explicit socket specification, it would fully override the port specification of that target. I also agree that a -p 22222 should not result in 22 and 22222. If you choose -p 22222 it should only scan 22222. If you want to scan both, you should have to specify -p 22,22222. In the end, I'm really happy that we went the target parser route, I think this helps make expectations more clear. Perhaps what we should do is make a direct mapping from command-line switches to target parser options so we don't loose any context when target parser goes to interpret? Then we can easily spec in the behavior and if we ever want to change it we can do so explicitly. |
|
@claudijd So then do you want something like (in pseudocode) this? Parse all passed command line switches I'm sorry if I misunderstood your idea. |
|
@agaurav77 some like that...
|
|
@agaurav77 I had fixed this up in #171 |
|
#171 is now landed, closing this for now, please reopen if needed. |
@claudijd
I tried ssh_scan -t ip -p 9999, and upon inspecting options[:sockets] it looked something like ["ip:22", "ip:9999"], when it should've been ["ip:9999"].