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 support for TELNET EXTENDED-OPTIONS-LIST (RFC861) #50

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

okofish
Copy link

@okofish okofish commented Apr 13, 2019

This adds transparent support for negotiation and subnegotiation of TELNET options on the Extended Options List - see RFC861. This standard extends the maximum possible options number from 255 to 511 by wrapping negotiation/subnegotiation commands for these options inside a subnegotiation command for option 255 (EXOPL). No options utilizing this feature were ever defined, but it is nonetheless an Internet standard, and I happen to have an application that requires it.

The only significant user-facing change is the addition of the short telopt_extended member to event structs negotiate_t and subnegotiate_t. I didn't want to change the type of the existing unsigned char telopt member as this could break existing user code, so I added the new member. Going forward, though, there should be no need for anyone to use telopt - I added a note to that effect in the readme.

A few more notes:

  • I don't really get PROXY mode so I didn't particularly touch any code involving it - feel free to point out any spots that need new conditions or anything like that.
  • I changed the types of several arguments regarding options from unsigned char to short, but I didn't add any checks that the supplied option number is 511 or less. I suppose these are needed in telnet_negotiate, telnet_begin_sb, and telnet_subnegotiation - anywhere else? Should they just call _error and return?
  • I tried to prevent VS Code from having its way with the code formatting but I may have missed a spot or two - sorry. Feel free to correct it if so.

@mhei
Copy link
Contributor

mhei commented Apr 13, 2019

I did not completed with a review, but one point I don't like is the introduction of another usage of "short" - it has a "slightly antiquated flavor". Why not just use ''int''?

@okofish
Copy link
Author

okofish commented Apr 13, 2019

I did not completed with a review, but one point I don't like is the introduction of another usage of "short" - it has a "slightly antiquated flavor". Why not just use ''int''?

Changed. I was sort of imitating the telnet_telopt_t struct in that regard, but it really doesn't matter.

@seanmiddleditch
Copy link
Owner

The CI Windows failure looks like a Travis problem, I need to move this all to Azure Pipelines anyway.

@okofish:

I don't really get PROXY mode

The key gist of PROXY mode is to act as a transparent passthru. It can be useful for Wireshark-like tools, for example. Basically, PROXY won't automatically respond to negotiation, and assumes some other layer (e.g. another remote end) is doing the actual negotiation, and that libtelnet is effectively just working as a protocol parser only.

Everything else looks good to me.

Did you want to add yourself to the AUTHORS file, especially as this is a significant feature?

@okofish
Copy link
Author

okofish commented May 27, 2019

Did you want to add yourself to the AUTHORS file, especially as this is a significant feature?

Done, thanks for the reminder.

I added input validation on the three functions that take a telopt as an argument to make sure it's within the valid range.

@thefallentree
Copy link
Contributor

This is awesome, are we getting this merged soon?

@seanmiddleditch
Copy link
Owner

Ack, sorry, I'm terrible at paying attention to GH notifications! :/ I'll figure out what's up with the Travis CI and then merge this.

@thefallentree
Copy link
Contributor

thefallentree commented Mar 3, 2020

merged as fluffos@e9c2128

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