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

Secure SNMPv3 user creation #46

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

bbilyeu
Copy link

@bbilyeu bbilyeu commented Jan 21, 2022

PR progress checklist (to be filled in by reviewers)

  • Changes to documentation are appropriate (or tick if not required)
  • Changes to tests are appropriate (or tick if not required)
  • Reviews completed

What type of PR is this?

Primary type

  • [build] Changes related to the build system
  • [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
  • [ci] Changes to the continuous integration configuration
  • [feat] A new feature
  • [fix] A bug fix
  • [perf] A code change that improves performance
  • [refactor] A code change that neither fixes a bug nor adds a feature
  • [revert] A change used to revert a previous commit
  • [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

  • [docs] Documentation changes
  • [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

Yes, there are a few breaking changes.

  1. logconnect has been changed to dontLogTCPWrappersConnects which identically matches the snmpd.conf option (instead of forcing a formula specific value). This also corrects a slightly less than intuitive boolean usage.
  2. syscontact changed to sysContact to also match the snmpd.conf option.
  3. location changed to sysLocation to also match the snmpd.conf option.

Related issues and/or pull requests

Describe the changes you're proposing

  • First and foremost, this addresses the SNMP user/pass being dumped into snmpd.conf in plaintext.
    • The specific workflow for Deb/RHEL systems is test SNMP access with credentials -> (on fail) stop snmpd -> add createUser string to the correct config file (/var/lib/____/snmpd.conf) -> start snmpd back up. Doing so will cause the credentials to be "consumed", converted into something no longer human readable.
  • Second, it standardizes a few of the options and moves closer to the Saltstack formula recommendation of "sane default values".

Pillar / config required to test the proposed changes

None, files test/integration/default/controls/config.rb and test/salt/pillar/default.sls were updated to all turnkey testing.

Debug log showing how the proposed changes work

CentOS 7 3003.3 and 3004.0 (both py3) would fail to start up SSH. Skipping those

CentOS 8 3003.3 py3

-----> Verifying <default-centos-8-3003-3-py3>...
       Loaded default

Profile: snmp formula (default)
Version: (not specified)
Target:  ssh://kitchen@localhost:61297

  ✔  snmp.config.file: Verify the configuration file
     ✔  File /etc/snmp/snmpd.conf is expected to be file
     ✔  File /etc/snmp/snmpd.conf is expected to be owned by "root"
     ✔  File /etc/snmp/snmpd.conf is expected to be grouped into "root"
     ✔  File /etc/snmp/snmpd.conf mode is expected to cmp == "0644"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysLocation Right Here"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysContact System Admin"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "dontLogTCPWrappersConnects yes"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "view all included .1 80"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       localhost"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.0.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rwcommunity     private       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rouser myv3user auth -V all"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "createUser string will be added to /var/lib/net-snmp/snmpd.conf"
  ✔  snmp.package.install: The required package should be installed
     ✔  System Package net-snmp is expected to be installed
  ✔  snmp.service.running: The service should be installed, enabled and running
     ✔  Service snmpd is expected to be installed
     ✔  Service snmpd is expected to be enabled
     ✔  Service snmpd is expected to be running

CentOS 8 3004.0 py3

-----> Verifying <default-centos-8-3004-0-py3>...
       Loaded default

Profile: snmp formula (default)
Version: (not specified)
Target:  ssh://kitchen@localhost:60780

  ✔  snmp.config.file: Verify the configuration file
     ✔  File /etc/snmp/snmpd.conf is expected to be file
     ✔  File /etc/snmp/snmpd.conf is expected to be owned by "root"
     ✔  File /etc/snmp/snmpd.conf is expected to be grouped into "root"
     ✔  File /etc/snmp/snmpd.conf mode is expected to cmp == "0644"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysLocation Right Here"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysContact System Admin"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "dontLogTCPWrappersConnects yes"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "view all included .1 80"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       localhost"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.0.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rwcommunity     private       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rouser myv3user auth -V all"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "createUser string will be added to /var/lib/net-snmp/snmpd.conf"
  ✔  snmp.package.install: The required package should be installed
     ✔  System Package net-snmp is expected to be installed
  ✔  snmp.service.running: The service should be installed, enabled and running
     ✔  Service snmpd is expected to be installed
     ✔  Service snmpd is expected to be enabled
     ✔  Service snmpd is expected to be running

Debian 9 3003.3 py3

-----> Verifying <default-debian-9-3003-3-py3>...
       Loaded default

Profile: snmp formula (default)
Version: (not specified)
Target:  ssh://kitchen@localhost:61403

  ✔  snmp.config.file: Verify the configuration file
     ✔  File /etc/snmp/snmpd.conf is expected to be file
     ✔  File /etc/snmp/snmpd.conf is expected to be owned by "root"
     ✔  File /etc/snmp/snmpd.conf is expected to be grouped into "root"
     ✔  File /etc/snmp/snmpd.conf mode is expected to cmp == "0644"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysLocation Right Here"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysContact System Admin"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "dontLogTCPWrappersConnects yes"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "view all included .1 80"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       localhost"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.0.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rwcommunity     private       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rouser myv3user auth -V all"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "createUser string will be added to /var/lib/snmp/snmpd.conf"
  ✔  snmp.package.install: The required package should be installed
     ✔  System Package snmpd is expected to be installed
  ✔  snmp.service.running: The service should be installed, enabled and running
     ✔  Service snmpd is expected to be installed
     ✔  Service snmpd is expected to be enabled
     ✔  Service snmpd is expected to be running

Debian 9 3004.0 py3

-----> Verifying <default-debian-9-3004-0-py3>...
       Loaded default

Profile: snmp formula (default)
Version: (not specified)
Target:  ssh://kitchen@localhost:60961

  ✔  snmp.config.file: Verify the configuration file
     ✔  File /etc/snmp/snmpd.conf is expected to be file
     ✔  File /etc/snmp/snmpd.conf is expected to be owned by "root"
     ✔  File /etc/snmp/snmpd.conf is expected to be grouped into "root"
     ✔  File /etc/snmp/snmpd.conf mode is expected to cmp == "0644"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysLocation Right Here"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysContact System Admin"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "dontLogTCPWrappersConnects yes"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "view all included .1 80"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       localhost"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.0.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rwcommunity     private       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rouser myv3user auth -V all"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "createUser string will be added to /var/lib/snmp/snmpd.conf"
  ✔  snmp.package.install: The required package should be installed
     ✔  System Package snmpd is expected to be installed
  ✔  snmp.service.running: The service should be installed, enabled and running
     ✔  Service snmpd is expected to be installed
     ✔  Service snmpd is expected to be enabled
     ✔  Service snmpd is expected to be running

Debian 10 3003.3 py3

-----> Verifying <default-debian-10-3003-3-py3>...
       Loaded default

Profile: snmp formula (default)
Version: (not specified)
Target:  ssh://kitchen@localhost:61477

  ✔  snmp.config.file: Verify the configuration file
     ✔  File /etc/snmp/snmpd.conf is expected to be file
     ✔  File /etc/snmp/snmpd.conf is expected to be owned by "root"
     ✔  File /etc/snmp/snmpd.conf is expected to be grouped into "root"
     ✔  File /etc/snmp/snmpd.conf mode is expected to cmp == "0644"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysLocation Right Here"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysContact System Admin"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "dontLogTCPWrappersConnects yes"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "view all included .1 80"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       localhost"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.0.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rwcommunity     private       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rouser myv3user auth -V all"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "createUser string will be added to /var/lib/snmp/snmpd.conf"
  ✔  snmp.package.install: The required package should be installed
     ✔  System Package snmpd is expected to be installed
  ✔  snmp.service.running: The service should be installed, enabled and running
     ✔  Service snmpd is expected to be installed
     ✔  Service snmpd is expected to be enabled
     ✔  Service snmpd is expected to be running

Debian 10 3004.0 py3

-----> Verifying <default-debian-10-3004-0-py3>...
       Loaded default

Profile: snmp formula (default)
Version: (not specified)
Target:  ssh://kitchen@localhost:60885

  ✔  snmp.config.file: Verify the configuration file
     ✔  File /etc/snmp/snmpd.conf is expected to be file
     ✔  File /etc/snmp/snmpd.conf is expected to be owned by "root"
     ✔  File /etc/snmp/snmpd.conf is expected to be grouped into "root"
     ✔  File /etc/snmp/snmpd.conf mode is expected to cmp == "0644"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysLocation Right Here"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "sysContact System Admin"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "dontLogTCPWrappersConnects yes"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "view all included .1 80"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       localhost"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.0.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rocommunity     public       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rwcommunity     private       192.168.1.0/24"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "rouser myv3user auth -V all"
     ✔  File /etc/snmp/snmpd.conf content is expected to include "createUser string will be added to /var/lib/snmp/snmpd.conf"
  ✔  snmp.package.install: The required package should be installed
     ✔  System Package snmpd is expected to be installed
  ✔  snmp.service.running: The service should be installed, enabled and running
     ✔  Service snmpd is expected to be installed
     ✔  Service snmpd is expected to be enabled
     ✔  Service snmpd is expected to be running

Documentation checklist

  • Updated the README (e.g. Available states).
  • Updated pillar.example.

Testing checklist

  • Included in Kitchen (i.e. under state_top).
  • Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
  • Updated the relevant test pillar.

Additional context

@bbilyeu
Copy link
Author

bbilyeu commented Feb 1, 2022

Apologies for the radio silence!

4903638765 :: This is a foolish mistake on my part, failing only due to improper casing of the commit subject.

4890361620 :: (EDIT) Resolved

5030042061 :: All the rest are failing due to something specific to Saltstack master branch (3005?), which isn't live yet.

@myii
Copy link
Member

myii commented Feb 3, 2022

@bbilyeu Regarding the failing commitlint job:

⧗   input: style(*): Added vim modelines
Adding simple vim modelines for convenience.
✖   subject must not be sentence-case, start-case, pascal-case, upper-case [subject-case]
✖   found 1 problems, 0 warnings

Please amend the commit title accordingly:

-style(*): Added vim modelines
+style(*): added vim modelines

In terms of the Rendering SLS 'base:snmp.conf' failed: could not find expected ':' failures:

       snmpv3 creating myv3user step 2 of 3:
         file.line:
           - name: /var/lib/snmp/snmpd.conf
           - mode: insert
           - location: end
           - content: 
         
       createUser myv3user SHA myv3password AES v3privpass
           - show_changes: False
           - onchanges:
             - snmpv3 creating myv3user step 1 of 3

This is happening because the whitespace control for createUser ... macro needs to be amended. I've suggested something inline.


Only an initial review, just to get the CI working, hopefully.

@alxwr Will you be able to look over this PR?

snmp/macros.jinja Outdated Show resolved Hide resolved
@myii
Copy link
Member

myii commented Feb 3, 2022

@bbilyeu Actually, when you amend the commit, would you mind rebasing this PR on the latest commit to this repo? That will use the updated CI matrix.

@bbilyeu
Copy link
Author

bbilyeu commented Feb 4, 2022

@bbilyeu Actually, when you amend the commit, would you mind rebasing this PR on the latest commit to this repo? That will use the updated CI matrix.

I apologize, but my rebase knowledge/experience is embarrassingly weak. Would amending the commit and rebasing not bloat the commit history with duplicates?

@myii
Copy link
Member

myii commented Feb 4, 2022

I apologize, but my rebase knowledge/experience is embarrassingly weak. Would amending the commit and rebasing not bloat the commit history with duplicates?

@bbilyeu No, it's an expected (and usually preferred) procedure. Once you've rebased and amended the commit message, you need to force push it back here.

This is useful documentation:

@myii
Copy link
Member

myii commented Feb 4, 2022

@bbilyeu I've just noticed that the very last commit message needs to be updated as well:

-Update snmp/macros.jinja
+fix(macros.jinja): fix macro `v3_createUser_string` whitespace control

Beau Bilyeu and others added 11 commits February 9, 2022 14:28
adding simple vim modelines for convenience.
Gentoo is the only one affected, but this is technically a timebomb.
* `init.sls` - This will now enable the service, restarting on
config changes normally.
* `conf.sls` - Having the watch_in:service on the file.managed state
will cause issues if any additional states follow `snmp_conf`.
Added a generic default message to avoid an awkward blank line.
* `map.jinja` - Adding the file path for persistent configs. These files
 will hold the actual createUser strings before they're consumed.
* `conf.sls` -  New users' createUser strings will be placed in the
`persistentconfig` files, which will consumed on snmp daemon start.
The process is stop snmpd, add createUser to the
`persistentconfig` file, and then
start snmpd back up. This somewhat opinionated choice to stop & start
per user avoided a significantly large and needlessly complex
 alternative.
* `snmpd.conf` - Allow defaults/blanks for access entries, where
appropriate. Changed `location` to `sysLocation` and `syscontact` to
`sysContact` to match snmpd.conf options identically.
Changed `logconnects` to `dontLogTCPWrappersConnects` for the same
reason and readability.
* `snmpd.conf.minimal` - Almost identical changes.
* `macros.jinja` - added macro for the createUser string.

BREAKING CHANGE: `location` is no longer accepted and has been
replaced with `sysLocation` to match the snmpd.conf standard.

BREAKING CHANGE: `syscontact` is no longer accepted and has been
replaced with `sysContact` to match the snmpd.conf standard.

BREAKING CHANGE: `logconnects` is no longer accepted and has been
replaced with `dontLogTCPWrappersConnects` to match the snmpd.conf
standard, as well as removing less than intuitive boolean.
* `pillar.example` - updated to match secure v3 users functionality
* `README.rst` - similarly updated
* `config.rb` - Now aware of the correct createUser state
* `default.sls` - Updated to match new snmpd.conf
This version of `init.sls` should have been in 177f110 and 3f8c6f2.
It should be `onchanges`, not `require` as it triggers if step 1
completes with 'unless condition is true'
* pillar.example - corrected to match yamllint guidelines
* snmp/conf.sls - exploded into variables to stay below 160 char
* snmp/macros.jinja - corrected style issues
* config.rb - fixed to snake_cake
Co-authored-by: Imran Iqbal <myii@users.noreply.github.com>
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.

2 participants