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

Optional TCP MSS option (--tcpmss) #1

Merged
merged 2 commits into from
Sep 3, 2022
Merged

Optional TCP MSS option (--tcpmss) #1

merged 2 commits into from
Sep 3, 2022

Conversation

mzpqnxow
Copy link

There was a discussion about this a while back (robertdavidgraham#326) and I ended up needing to add this for a few specific networks that appear to be deliberately dropping packets without TCP options. This is open upstream as robertdavidgraham#646

Please feel free to rework it into something a little bit neater if you do want to merge it

@mzpqnxow
Copy link
Author

mzpqnxow commented Feb 11, 2022

Also, any thoughts on making it an option to set the MSS to an arbitrary value?

I figure the objective is to blend to make surveys more accurate rather than to allow a user to fully customize the packet (and that can become a very slippery slope ...) so I decided not to make the MSS value customizable

@p-l-
Copy link
Member

p-l- commented Aug 19, 2022

@mzpqnxow I'm so sorry I wasn't, for some reason, watching this repository :-/

I'll look at your PR ASAP.

@p-l-
Copy link
Member

p-l- commented Aug 19, 2022

I have a request (I can take care of it if you want): could you amend your commits to remove unneeded changes (like spaces / newlines in src/templ-pkt.c)? It will be much easier for me to keep this repository synchronized with its upstream.

@p-l- p-l- force-pushed the master branch 2 times, most recently from 1a3cabb to 4bc76eb Compare August 23, 2022 14:53
@mzpqnxow
Copy link
Author

mzpqnxow commented Sep 3, 2022

Hey, sorry for the slow response- not sure if you already made the changes yourself but I think I cleaned up what you were referring to

@p-l- p-l- merged commit bdbf49a into ivre:master Sep 3, 2022
@p-l-
Copy link
Member

p-l- commented Sep 3, 2022

I did some more fixes in src/templ-pkt.c because you had added / removed whitespaces while it was not needed, and that makes it harder for me to maintain the patch. Your commit has now also been rebased so that the README.md is updated in the last commit.

Thanks for this contribution!

@mzpqnxow
Copy link
Author

mzpqnxow commented Sep 4, 2022

Great, thanks- and apologies, that whitespace wasn't deliberate- I actually had a hard time spotting it at first, even after you pointed it out- good eye, though I guess it stands out when you try to merge/sync with upstream

Also, thanks for merging, you've become my official upstream, though I'm not (yet) a user of IVRE

@p-l-
Copy link
Member

p-l- commented Sep 5, 2022

If you are a user of the --banners option, you may like @Frky's PR (robertdavidgraham#681) that is also integrated here (with a fix that @Frky needs to add in his PR to the official project).

If you do not use IVRE to parse the results, you may have useless values, for example, with SSH servers. Having users of this project who don't use IVRE was not my target, but if you tell me what your issues are, we'll do our best to help you.

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.

2 participants