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

Implement snmpv3_user fact and snmp::snmpv3_usm_hash function #157

Merged
merged 1 commit into from
Nov 14, 2018
Merged

Implement snmpv3_user fact and snmp::snmpv3_usm_hash function #157

merged 1 commit into from
Nov 14, 2018

Conversation

smoeding
Copy link
Contributor

@smoeding smoeding commented Nov 2, 2018

Pull Request (PR) description

This PR contains the code for the new snmpv3_user fact and snmp::snmpv3_usm_hash function.

The fact is a structured fact that contains the configured SNMPv3 users and the the engine plus the passphrase hash that has been generated (e.g. by using snmp::snmpv3_user). Example:

snmpv3_user => {
  icinga => {
    engine => "0x80001f8880e0bd8a70cbf84f5800000000",
    authhash => "0x80001f8880e0bd8a70cbf84f5800000000"
  }
}

The function can be used to calculate the SHA1 or MD5 hashes for a given engine and passphrase. The generated hash can be compared with the value returned by the fact to determine if the user actually has the password that Puppet tries to enforce.

The function is a ruby version of the code published in RFC-3414 (sections A.2.1 and A.2.2).

With this fact and function the snmp::snmpv3_user defined type could be extended to actually check the existing user and trigger an update if the hashes do not match. Something like this has also been discussed in issue #4.

Maybe a custom type/provider implementation for snmpv3_user would be better. But I'm not sure how good restarting the SNMP daemon would fit into the type/provider model.

@bastelfreak
Copy link
Member

Hi @smoeding, thanks for the PR. Can you take a look at the failed travis job?

@bastelfreak
Copy link
Member

This looks good to me. Can somebody else please take a look as well?

@vStone
Copy link
Contributor

vStone commented Nov 5, 2018

Would we dare to put the MD5 and SHA values in a
type SNMP::AuthType = Enum['MD5','SHA']
?

dispatch :snmpv3_usm_hash do
required_param 'String', :authtype
required_param 'String', :engine
required_param 'String', :passphrase
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure but can we use required_param 'String[8]', :passphrase ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be sufficient to implement the type restriction in snmp::snmpv3_user?

@smoeding
Copy link
Contributor Author

smoeding commented Nov 6, 2018

I have update the fact to include more details about the snmpv3 users that are currently configured. This includes details about the authtype and privtype used and also the privhash (together with that authhash already available).

This should be enough to use the cleartext authpass and privpass parameters in snmp::snmpv3_user to calculate the expected hashes and compare them. This can be used to trigger the exec resource to (re)create the user.

@alexjfisher alexjfisher added this to the 4.1.0 milestone Nov 8, 2018
@alexjfisher alexjfisher added the enhancement New feature or request label Nov 8, 2018
@smoeding
Copy link
Contributor Author

I will prepare a rebase & squash for the PR.

This rewrite of `snmp::snmpv3_user` (re)creates the user if it does not
exist or the stored passwords do not match the expected values. It uses the
`snmpv3_user` fact to retrieve the currently configured SNMPv3 users and
calculates the expected password hashes by calling the
`snmp::snmpv3_usm_hash` function using the cleartext password.

Updating the passwords in Puppet will now correctly update the passwords in
the SNMP configuration file. (closes #4).
manifests/snmpv3_user.pp Show resolved Hide resolved
manifests/snmpv3_user.pp Show resolved Hide resolved
lib/puppet/functions/snmp/snmpv3_usm_hash.rb Show resolved Hide resolved
@Dan33l
Copy link
Member

Dan33l commented Nov 13, 2018

Hi @smoeding . Thank you for the PR. I written some questions with in line comments.

Copy link
Member

@Dan33l Dan33l left a comment

Choose a reason for hiding this comment

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

The current acceptance tests are already validating that function is working well to create a user.
But are you able to check that a modification is working as expected and in an idempotent way ?

@smoeding
Copy link
Contributor Author

I'm not sure if I understand your question correctly.

The Puppet code tries to figure out if the hash in the config file is the same that is generated by executing the documented function using the engine id and the cleartext password as input. If they are equal, nothing is done. Otherwise Puppet adds a createUser line to the config and starts the SNMP daemon. The SNMP daemon replaces the createUser line with a different config item that contains hashed password. If all goes well (and the implementation is correct) then this will be the same hashes that Puppet calculates and therefore will cause the "do nothing" described above.

The unit tests already contain tests for different scenarios: user missing, user defined but with different hashes, ... and checks that the exec resource is only triggered when it should be.

@Dan33l
Copy link
Member

Dan33l commented Nov 14, 2018

The unit tests test the catalog. The acceptance tests test the result of catalog. I saw you proposed good range of unit tests. If you know how to do, IMO it is interesting to add acceptance tests about modifying user settings. If you are interested by learning how to do, we can provide some guidance on #voxpupuli channel (IRC or Slack). And otherwise we can merge the PR like this and i'll update acceptance in a new PR. Don't worry about this. Simply tell me your choice.

@smoeding
Copy link
Contributor Author

I never looked into setting up acceptance tests. One reason is that I don't have a Vagrant setup available. And unfortunately I won't have the time to look into something like that in the near future.

So please feel free to add any acceptance tests yourself! Thanks!

@Dan33l
Copy link
Member

Dan33l commented Nov 14, 2018

Ok. Thank you for this PR.

@alexjfisher alexjfisher merged commit bc03aa2 into voxpupuli:master Nov 14, 2018
@alexjfisher
Copy link
Member

@smoeding Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants