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

Disable SSH server by default on client side and add the flag --allow-server-ssh to enable it #1508

Merged
merged 18 commits into from
Feb 20, 2024

Conversation

charnesp
Copy link
Contributor

@charnesp charnesp commented Jan 30, 2024

Describe your changes

I add in the configuration of the Netbird client the option to allow (or not) the launch of an SSH server on the peer.

The client-side control of the SSH server option on the client side of Netbird create a root access to all peers by Netbird administrators, thus an important security issue.

This changes the default behavior for new peers, by requiring the agent to be executed with allow-server-ssh set to true in order for the management configuration to take effect. See example commands below:

To enable SSH

netbird down
netbird up --allow-server-ssh

To disable SSH

netbird down
netbird up --allow-server-ssh=false

Existing peers will have SSH enabled in the same way as the previous version. You can disable it with the commands above.

Issue ticket number and link

#509 and #683

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)
  • Extended the README / documentation, if necessary

@CLAassistant
Copy link

CLAassistant commented Jan 30, 2024

CLA assistant check
All committers have signed the CLA.

@charnesp
Copy link
Contributor Author

Hi @mlsmaycon I see you tried to relaunch the test pipeline. By looking at the logs, it appears it fail because it tried to test the SSH server connectivity. As in this pull request the SSH server is disabled by default (to ensure a zero-trust behavior of the agent) it will always fail.
Thus 2 options :

  • Enabling the SSH server by default (with the potential zero-trust issue if the agent is not correctly configured)
  • Modyfing the test pipeline to handle this configuration.

What do you recommend ?

@mlsmaycon
Copy link
Collaborator

Hi @mlsmaycon I see you tried to relaunch the test pipeline. By looking at the logs, it appears it fail because it tried to test the SSH server connectivity. As in this pull request the SSH server is disabled by default (to ensure a zero-trust behavior of the agent) it will always fail. Thus 2 options :

  • Enabling the SSH server by default (with the potential zero-trust issue if the agent is not correctly configured)
  • Modyfing the test pipeline to handle this configuration.

What do you recommend ?

Hello @charnesp, first of all, thanks for submitting the PR;

I have two points to add about the PR and the tests:

  1. about the PR: We need to consider the case when existing users are using the client and require SSH, so for retro-compatibility, we should use a *bool type for Config.ServerSSHAllowed and when there is an existing configuration file with Config.ServerSSHAllowed == nil we set the value to true.
  2. for the tests: We should modify them to validate both cases.

What do you think? do you need some help there?

@charnesp
Copy link
Contributor Author

Hi @mlsmaycon , it seems indeed logical for existing users.

I pushed a modification of the code following your advise using pointers and tested it. It should cover your case.

Thank you

client/internal/config.go Outdated Show resolved Hide resolved
@bcmmbaga
Copy link
Contributor

bcmmbaga commented Feb 16, 2024

@charnesp we need to fix these tests TestEngine_SSH

client/internal/config.go Show resolved Hide resolved
client/internal/config.go Outdated Show resolved Hide resolved
client/internal/config.go Outdated Show resolved Hide resolved
@charnesp
Copy link
Contributor Author

@bcmmbaga I guess the last pull should be ok concerning the TestEngine_SSH.

bcmmbaga
bcmmbaga previously approved these changes Feb 20, 2024
@charnesp
Copy link
Contributor Author

Hi @bcmmbaga do you need my help for the branch conflicts?

@bcmmbaga
Copy link
Contributor

Hi @bcmmbaga do you need my help for the branch conflicts?

Yes, please resolve them.

# Conflicts:
#	client/cmd/root.go
#	client/cmd/up.go
#	client/internal/config.go
#	client/proto/daemon.pb.go
#	client/proto/daemon.proto
#	client/server/server.go
@mlsmaycon mlsmaycon changed the title Add the option to allow launch of SSH server on client side Disable SSH server by default on client side and add a flag to enable it Feb 20, 2024
@mlsmaycon mlsmaycon changed the title Disable SSH server by default on client side and add a flag to enable it Disable SSH server by default on client side and add the flag --allow-server-ssh to enable it Feb 20, 2024
@mlsmaycon mlsmaycon merged commit d5338c0 into netbirdio:main Feb 20, 2024
15 checks passed
@mlsmaycon
Copy link
Collaborator

Thanks, @charnesp, for the awesome contribution!

@charnesp
Copy link
Contributor Author

@mlsmaycon my pleasure. Thank you for your amazing work and solution!

@dillfrescott
Copy link

Thank you so much for doing this!!!

Foosec pushed a commit to Foosec/netbird that referenced this pull request May 8, 2024
…-server-ssh to enable it (netbirdio#1508)

This changes the default behavior for new peers, by requiring the agent to be executed with allow-server-ssh set to true in order for the management configuration to take effect.
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

6 participants