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 option to enable/disable netapi clients #63050

Merged

Conversation

barneysowood
Copy link
Contributor

@barneysowood barneysowood commented Nov 9, 2022

What does this PR do?

Adds a new config option "netapi_enable_clients" that takes a list of clients to enable in the netapi.

Checks the list early in handling the the request before authentication occurs. Should be useful where certain clients (eg ssh or wheel) aren't required and should be disabled to reduce attack surface in the salt-api.

This PR is an alternative to #59622 which implements @dwoz's suggestion in #59622 (review) to make client disabled by default and require them to be specifically enabled in the master config.

New Behavior

Adds "netapi_enable_clients" list option to the config which is checked when a request is passed to NetApiClient.run() and an exception raised if the requested client is not in the list.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

Commits signed with GPG?

No

@barneysowood barneysowood requested a review from a team as a code owner November 9, 2022 17:08
@barneysowood barneysowood requested review from whytewolf and removed request for a team November 9, 2022 17:08
doc/ref/configuration/master.rst Outdated Show resolved Hide resolved
doc/ref/configuration/master.rst Show resolved Hide resolved
salt/netapi/__init__.py Show resolved Hide resolved
@Ch3LL
Copy link
Contributor

Ch3LL commented Nov 9, 2022

@barneysowood can we close #59622 in favor of this one?

@Ch3LL Ch3LL requested a review from dwoz November 9, 2022 19:25
@Ch3LL
Copy link
Contributor

Ch3LL commented Nov 9, 2022

I'm thinking we should also add some docs in the main salt-api docs since this will be required for all users to use any client

@Ch3LL Ch3LL added Sulfur v3006.0 release code name and version has-failing-test labels Nov 9, 2022
@barneysowood
Copy link
Contributor Author

@barneysowood can we close #59622 in favor of this one?

Done.

@barneysowood barneysowood force-pushed the default-disable-salt-api-clients branch from 7d78804 to 5119c57 Compare November 11, 2022 13:16
@barneysowood
Copy link
Contributor Author

I'm thinking we should also add some docs in the main salt-api docs since this will be required for all users to use any client

Added some docs in the netapi reference

@barneysowood barneysowood force-pushed the default-disable-salt-api-clients branch from 14a7642 to e25f612 Compare November 14, 2022 18:49
@barneysowood
Copy link
Contributor Author

re-run pr-macosx-catalina-x86_64-py3-pytest

Copy link
Contributor

@Ch3LL Ch3LL left a comment

Choose a reason for hiding this comment

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

Can you also highlight this change in the file: doc/topics/releases/3006.rst

And emphasize that its a breaking change and will require users to update their configs if they are using the salt-api. Also point to hte new docs for more information.

changelog/63050.added Outdated Show resolved Hide resolved
salt/netapi/__init__.py Outdated Show resolved Hide resolved
@barbaricyawps
Copy link
Contributor

I'm also wondering if we should spin off 1-2 new issues to capture some chores related to this PR:

  • One chore would be to make sure this is specifically called out as one of the first things in the release notes and spell out that action is needed.
  • A second chore would be to create some more in-depth docs that explain the process for adding clients to their config.

I can spin up those issues if needed tomorrow.

@barneysowood
Copy link
Contributor Author

I'm also wondering if we should spin off 1-2 new issues to capture some chores related to this PR:

  • One chore would be to make sure this is specifically called out as one of the first things in the release notes and spell out that action is needed.
  • A second chore would be to create some more in-depth docs that explain the process for adding clients to their config.

I can spin up those issues if needed tomorrow.

@barbaricyawps - that sounds like a good idea. Feel free to pass either or both of those to me. I'll probably need to discuss with you where best to create the more in-depth docs (in the general docs or perhaps the userguide, or somewhere else)

barneysowood and others added 12 commits December 2, 2022 11:47
Adds an option to allow you to enable clients (eg ssh, wheel) in the
netapi. By default all clients will be disabled. Does the check before
any attempts to authenticate.
Adds pytest based integration tests as the old non-pytest
netapi/test_client.py tests have been removed.
Adds documentation for netapi_enable_clients to the netapi reference
documentation.
Enables local and runner clients for rest_cherrypy tests
Enables local, local_async, runner and runner_async clients for
rest_tornado tests
Updates changelog entry for netapi_enable_clients options to be a
"changed" rather than "added" changelog entry as it will require users
to update their master config.
Expands on explanation of change to make clear what will happen.
Updates the error message returned to explain how to enable a disabled
netapi client.
Adds information on the netapi_enable_clients changes to the 3006
release document.
@barneysowood barneysowood force-pushed the default-disable-salt-api-clients branch from e25f612 to 896a7ad Compare December 2, 2022 12:49
@barneysowood
Copy link
Contributor Author

Can you also highlight this change in the file: doc/topics/releases/3006.rst

And emphasize that its a breaking change and will require users to update their configs if they are using the salt-api. Also point to hte new docs for more information.

@Ch3LL - I've done that and added the other requested changes. Let me know if you're happy with those.

@Ch3LL
Copy link
Contributor

Ch3LL commented Dec 2, 2022

@barbaricyawps looks like @barneysowood added docs to the release notes and docs have been added for salt-api.

Are you okay with the changes @barbaricyawps ?

@garethgreenaway garethgreenaway merged commit e714c21 into saltstack:master Dec 2, 2022
@barneysowood barneysowood deleted the default-disable-salt-api-clients branch December 3, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-failing-test Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants