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

feat(anta): Add support for command blacklist to secure tests #416

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

titom73
Copy link
Collaborator

@titom73 titom73 commented Sep 28, 2023

Description

Add support of a list of blacklisted regex to secure test execution and avoid
any critical change on devices.

It currently blocked CLI with following regex

  • Reload command: ^reload\s*\w*
  • Configure mode: ^conf\w*\s*(terminal|session)*
  • Write: ^wr\w*\s*\w+

Fixes #413

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have run pre-commit for code linting and typing (pre-commit run)
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes (tox -e testenv)

@titom73 titom73 added the framework-enhancement New feature or request label Sep 28, 2023
@titom73 titom73 marked this pull request as ready for review September 28, 2023 14:47
anta/models.py Show resolved Hide resolved
anta/models.py Outdated Show resolved Hide resolved
docs/api/models.md Show resolved Hide resolved
@gmuloc
Copy link
Collaborator

gmuloc commented Sep 28, 2023

This implementation won't prevent a malicious device implementation (injecting reload / conf t / ... in its own _collect.

Wondering if we should not rather implement this in the AntaDevice(ABC) collect ?

anta/models.py Outdated Show resolved Hide resolved
@gmuloc
Copy link
Collaborator

gmuloc commented Sep 28, 2023

This implementation won't prevent a malicious device implementation (injecting reload / conf t / ... in its own _collect.

Wondering if we should not rather implement this in the AntaDevice(ABC) collect ?

we keep it in test

Co-authored-by: Guillaume Mulocher <gmulocher@arista.com>
anta/models.py Show resolved Hide resolved
anta/models.py Outdated Show resolved Hide resolved
@gmuloc gmuloc added this to the v0.10.0 milestone Sep 28, 2023
Copy link
Contributor

@carl-baillargeon carl-baillargeon left a comment

Choose a reason for hiding this comment

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

Tested a few blocked commands locally with collect of AntaTest and it works.

Should we add this guard to anta debug as well?

(.anta) ➜  anta git:(test_blacklist) anta debug run-cmd -c "configure terminal" -d DC1-LEAF1A --ofmt text
Run command configure terminal on DC1-LEAF1A

(.anta) ➜  anta git:(test_blacklist) anta debug run-cmd -c "write" -d DC1-LEAF1A
Run command write on DC1-LEAF1A
{'messages': ['Copy completed successfully.']}

@titom73
Copy link
Collaborator Author

titom73 commented Oct 3, 2023

Tested a few blocked commands locally with collect of AntaTest and it works.

Should we add this guard to anta debug as well?

(.anta) ➜  anta git:(test_blacklist) anta debug run-cmd -c "configure terminal" -d DC1-LEAF1A --ofmt text
Run command configure terminal on DC1-LEAF1A

(.anta) ➜  anta git:(test_blacklist) anta debug run-cmd -c "write" -d DC1-LEAF1A
Run command write on DC1-LEAF1A
{'messages': ['Copy completed successfully.']}

Ideas was to protect tests to send dangerous command while in debug mode, you do that on purpose. A complete protection of the device should be done via a correct RBAC configuration.

Adding this protection to anta debug can be done but in different manner than process in place for anta nrfu

Copy link
Contributor

@carl-baillargeon carl-baillargeon left a comment

Choose a reason for hiding this comment

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

LGTM

@gmuloc gmuloc requested a review from mtache October 3, 2023 12:26
@titom73 titom73 merged commit c6b1d2a into aristanetworks:main Oct 4, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: Add some security to prevent any "conf" command
4 participants