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

fix: Correct handling of mixed-case device names #856

Merged
merged 9 commits into from Mar 1, 2024
Merged

fix: Correct handling of mixed-case device names #856

merged 9 commits into from Mar 1, 2024

Conversation

gkc
Copy link
Contributor

@gkc gkc commented Feb 27, 2024

- What I did

- How I did it

  • modified regex in validation_utils.dart to be alphanumeric snake case, max length 36
  • modified various programs' args parsing to validate device names using the same validation function
  • modified various programs' args help to use the same language for describing the device name format
  • Added unit tests for invalid device names
  • Modified other unit tests to use valid device names

- How to verify it
Tests pass

- Modify ClientParamsBase constructor to force `device` to lowercase
- Modify SshnpdImpl constructor to force `device` to lowercase
- Modify SshnpdParams constructor to force `device` to lowercase
- Modify tests to expect device to be lowercase of whatever was supplied
cconstab
cconstab previously approved these changes Feb 27, 2024
- modified regex in validation_utils.dart to be alphanumeric snake case, max length 30
- modified various programs' args parsing to validate device names using the same validation function
- modified various programs' args help to use the same language for describing the device name format
- Added unit tests for invalid device names
- Modified other unit tests to use valid device names
…lizers now that all of the relevant constructors are doing an `invalidDeviceName` check
@gkc
Copy link
Contributor Author

gkc commented Feb 28, 2024

  • Changed validation regex for device name to alphanumeric snake case, max length 30
  • Have all relevant constructors assert valid device name using the same validation function
  • Modified installation docs to include device name format info
  • Modified various program args to include consistent device name format help

XavierChanth
XavierChanth previously approved these changes Feb 28, 2024
Copy link
Member

@XavierChanth XavierChanth left a comment

Choose a reason for hiding this comment

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

LGTM.
I had mentioned making it 36 characters so that someone using uuid v4s to manage their devices could do:

sshnpd -d "$(echo $uuid | sed 's/-/_/g')" ...

But this is still better than what we had before. There are some people who may be using uppercase device names as well, so it might be good if sshnpd automatically converts the device name input to lower case and emits a warning. This way upgrading is non-breaking

@gkc
Copy link
Contributor Author

gkc commented Feb 29, 2024

LGTM. I had mentioned making it 36 characters so that someone using uuid v4s to manage their devices could do:

sshnpd -d "$(echo $uuid | sed 's/-/_/g')" ...

But this is still better than what we had before. There are some people who may be using uppercase device names as well, so it might be good if sshnpd automatically converts the device name input to lower case and emits a warning. This way upgrading is non-breaking

Updated to 36 length

Given that the daemon does not currently work properly when using uppercase device names, I'd rather avoid any potential future confusion / bugs and have strict validation requiring lowercase

Copy link
Member

@cconstab cconstab left a comment

Choose a reason for hiding this comment

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

Observation: Connected appears in the command line then an error message. I would have expected syntax to be checked before connection was made. Does not break anything just felt unexpected..
e.g.

╰$ ./npt -f @cconstab -t @ssh_1 -d ser-valan -l 9000 -r @rv_am -p 3389                            
Connecting ... Connected
Version : 5.0.3 (core: 6.0.4)
-f, --from (mandatory)                       This client's atSign
-t, --to (mandatory)                         The device daemon's atSign
-r, --srvd (mandatory)                       The socket rendezvous's atSign
-d, --device (mandatory)                     Receiving device name. Alphanumeric snake case, max length 36.
-l, --local-port,--lp                        client-side local port for the socket tunnel. If not supplied, we will
                                             ask the o/s for a spare port
                                             (defaults to "0")
-p, --remote-port,--rp (mandatory)           The remote port required
-h, --remote-host,--rh                       The remote host required
                                             (defaults to "localhost")
-k, --key-file,--keyFile                     Path to this client's atSign's keyFile, if not in ~/.atsign/keys/
    --root-domain                            atDirectory domain
                                             (defaults to "root.atsign.org")
    --[no-]per-session-storage,--[no-]pss    Use ephemeral local storage for each session. Defaults to true,
                                             enabling you to run multiple local clients concurrently. However: if
                                             you wish to run just one client at a time, then you will get a
                                             performance boost if you negate this flag.
                                             (defaults to on)
-v, --verbose                                More logging
    --help                                   Print usage
-x, --output-execution-command               Instead of running the srv in the same process, fork the srv, print
                                             the local port to stdout, and exit this program.

Invalid argument(s): Device name must be alphanumeric snake case, max length 36
2024-02-29 15:52:31.423678 : Cleaning up temporary files

@gkc
Copy link
Contributor Author

gkc commented Mar 1, 2024

Yeah that's naff. Will fix

@gkc
Copy link
Contributor Author

gkc commented Mar 1, 2024

[cconstab] Observation: Connected appears in the command line then an error message. I would have expected syntax to be checked before connection was made. Does not break anything just felt unexpected

[gkc] Yeah that's naff. Will fix

@cconstab fixed

@gkc gkc requested a review from cconstab March 1, 2024 13:33
@gkc gkc merged commit e7b51ba into trunk Mar 1, 2024
6 checks passed
@gkc gkc deleted the fix/839 branch March 1, 2024 19:49
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.

sshnp/sshnpd -d option does not handle capitalisation of device names
3 participants