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

refactor: Remove file system bindings #516

Merged
merged 68 commits into from Oct 9, 2023
Merged

Conversation

XavierChanth
Copy link
Member

@XavierChanth XavierChanth commented Oct 4, 2023

- What I did

  • Updated key arguments:
    • Added -i just like ssh, -s is now a bool which sends the public key associated with -i
  • Renamed SSHNPImpl to SSHNPCore since it is abstract, it isn't really an implementation
  • A major overhaul to local file and ssh key management
    • Added AtSSHKeyPair which holds a set of ssh keys
    • Added LocalSSHKeyUtil and DartSSHKeyUtil to manage keys depending on what is available to store / create SSH Keys
    • Added SSHNPDLocalSSHKeyHandler (a mixin on SSHNPCore) which exposes LocalSSHKeyUtil safely
    • Added SSHNPDDartSSHKeyHandler (a mixin on SSHNPForwardDart) which exposes DartSSHKeyUtil safely
  • Split the ForwardDart implementation into two:
    • one which expects that the local file system is still available, so it can output an ssh command
    • another which expects that there is no local file system available and just passes a handler to the dartssh2 SSHClient
    • The reason for this design decision is that I expect we will want to run the first in a separate isolate so that we can exit the main program safely, but then we can no longer access the SSHClient so it is not suitable for use in sshnp_gui
    • I have not implemented these design decisions as I am leaving them open ended for an architecture discussion
  • Deferred payload handling from sshnpd to mixins, since it is implementation specific
    • The default payload handler mixin exposes the ephemeralPrivateKey safely
  • Added type-safety to
    • DefaultArgs
    • String names in SSHNPArg/SSHNPParams
  • Replaced the rsa boolean with an enum of sshAlgorithms
    • currently just ed25519 (default) and rsa

Several Action items come out of these changes:

  • Write new unit tests for arg parsing which validate against the new argument changes to:
    • -i, -s, -r
  • Modify the GUI to be able to input the new arguments
    • Need to write UI for handling SSH key management
  • Architecture discussion
    • How to handle the ForwardDart Implementations

- How to verify it

NB - Ignore the failed client-trunk test, it is being provided the -i arg and doesn't know how to handle it.

- Description for the changelog
refactor: Remove file system bindings

@XavierChanth XavierChanth changed the title DRAFT: Remove file system bindings chore(DRAFT): Remove file system bindings Oct 4, 2023
@XavierChanth
Copy link
Member Author

Trunk test is failing because of the breaking change caused by adding the -i option, this makes me wonder if we even need the trunk test, or if it should be disabled by default like the backward tests.

final String rootDomain;
final int localSshdPort;
final String ephemeralPermissions;
final SupportedSSHAlgorithm sshAlgorithm;
Copy link
Member Author

Choose a reason for hiding this comment

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

Remove late from all of these, which means I had to change how they get set below

@@ -4,8 +4,6 @@ import 'package:noports_core/src/sshrv/sshrv_impl.dart';
import 'package:socket_connector/socket_connector.dart';
import 'package:noports_core/src/common/default_args.dart';

typedef SSHRVGenerator = SSHRV Function(String, int, {int localSshdPort});
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved this to src/common/types.dart

@@ -0,0 +1,3 @@
library noports_core_sshnp_core;

export 'src/sshnp/sshnp_core.dart';
Copy link
Member Author

Choose a reason for hiding this comment

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

Exported this so it can implemented in a flutter library later

Copy link
Member Author

Choose a reason for hiding this comment

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

If we want to optimize down the road

at_utils: ^3.0.15
crypton: ^2.1.0
cryptography: ^2.7.0
Copy link
Member Author

Choose a reason for hiding this comment

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

Cryptography is used for generating ed25519 keys in pure dart

meta: ^1.9.1
openssh_ed25519: ^1.1.0
Copy link
Member Author

Choose a reason for hiding this comment

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

openssh_ed25519 is used for converting the generated ed25519 keys to openssh format

path: ^1.8.3
posix: ^5.0.0
Copy link
Member Author

Choose a reason for hiding this comment

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

posix is used for calling chmod to modify the written ephemeral keys

Copy link
Member Author

Choose a reason for hiding this comment

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

We will have to assess if this method works on windows, I have yet to test it

@XavierChanth XavierChanth changed the title refactor(DRAFT): Remove file system bindings refactor: Remove file system bindings Oct 6, 2023
@XavierChanth XavierChanth self-assigned this Oct 6, 2023
@XavierChanth XavierChanth marked this pull request as ready for review October 6, 2023 23:50
@gkc gkc merged commit 982bb5a into trunk Oct 9, 2023
13 of 14 checks passed
@gkc gkc deleted the remove-file-system-bindings branch October 9, 2023 11:40
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.

None yet

2 participants