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

Fix login and session issues connecting to zabbix #1380

Conversation

CircuitCipher
Copy link
Contributor

  • zabbix httpapi plugin - if login failed, it would not be attempted again by later tasks. fix now marks the connection as disconnected on failed login so that later tasks can try
  • zabbix_user - if the api user's password was changed, session errors would occur in later tasks. fix will relogin to zabbix after changing password of the api user

SUMMARY

Fixes #1209 because reset_connection will no longer be necessary in that context

Disconnect when a failed login happens

Why do we need this ?

In the case of writing an Ansible role to:

  • install Zabbix from these images
  • then update the Zabbix admin password in a idempotent way,

We may need to use a test login call to see if our new password works or if its still the default

  • We use failed_when: false on the task so our play doesn't fail if we get a failed login
  • If the the call fails, we can assume we have not yet setup the admin password and the default zabbix password is still in place

Where this fails

Currently the codebase doesn't mark the connection as disconnected when a login fails so subsequent attempts will assume we are logged in and provide a bad auth token

The fix

In community/zabbix/plugins/httpapi/zabbix.py we update the login() method so that when the login is failed, the connection is marked as disconnected.

try:
	# Zabbix <= 6.2
	payload = self.payload_builder("user.login", user=username, password=password)
	code, response = self.send_request(data=payload)
except ConnectionError:
	# Zabbix >= 6.4
	try:
		payload = self.payload_builder("user.login", username=username, password=password)
		code, response = self.send_request(data=payload)
	 except ConnectionError:
		# If this task fails to login, we mark the connection as disconnected so that
		# future tasks can try to reauth
		self.connection._connected = False
		raise

Update zabbix_user module to notify on successful password changes

Why do we have to do this?

It would be nice if we could detect changing the current Zabbix API user's password and re-login the user instead of seeing session terminated errors on subsequent calls

The original intent was to do this all from inside the Zabbix HTTPAPI class but it turns out the update user request is actually two separate requests made by the zabbix_user module:

  • Get the user id by searching for the user by the given username using the user.get method
  • Call user.update method to change the password by passing in the user id
    • By the time the zabbix httpapi receives the send_request for the user.update:
      • We only have the user id of the request so its hard to determine if the connection user is the user we are changing the password of!

Zabbix_user module User::update_user() method

  • From within the zabbix_user module we still have access to the username of the user we are changing the password for but we don't have access to the Ansible HTTPAPI options such as the ansible_user which is our API user!
    • So we have to notify the Zabbix Ansible HTTPAPI that a user's password has changed.
    • The Ansible HTTPAPI can then determine if we changed our current user's password and re-login with the new credentials
      • We call the password_changed_for_user() method of the ZabbixApiRequest class
      • community/zabbix/plugins/modules/zabbix_user.py:
      • successfully_changed_password = user_ids and override_passwd
        if successfully_changed_password:
            self._zapi.password_changed_for_user(username=username, password=passwd)
        
        return user_ids

Add the password_changed_for_user method to ZabbixApiRequest

  • We need to communicate with the connection so it makes sense to encapsulate these calls inside the ZabbixApiRequest class that already has the handle.
  • community/zabbix/plugins/module_utils/api_request.py:
def password_changed_for_user(self, username: str, password: str):
    return self.connection.password_changed_for_user(username, password)
  • Theres some voodoo that occurs here that involves JSON-RPC calls to get to the actual Zabbix Ansible HTTPAPI because the connection object we have here is just a dummy from module-utils

Add password_changed_for_user method to Zabbix Ansible/HTTPAPI

  • community/zabbix/plugins/httpapi/zabbix.py
  • This is the method that ends up being called by the module through the ZabbixApiRequest class and the JSON-RPC calls.
  • Here we detect if the user whose password was changed is the same user we are using for the Zabbix API requests.
    • If so, we login with the new credentials

      def password_changed_for_user(self, username: str, password: str):
         '''
         An event handler for when a zabbix user's password has been changed.
         We can then check to see if the API user's password has been changed.
         If so, we re-login so that the next modules are authenticated with the new password.
      
         :param str username: Username of the user whose password was changed
         :param str password: The new password
         '''
         changed_current_user_pass = self.connection.get_option('remote_user') == username
         if changed_current_user_pass:
            self.login(username, password)
         return 

JSON-RPC communication between module and connection

  • JSON-RPC requests are handled by the JsonRpcServer class
  • Modules and their connection usually reside on different machines so their communication goes over RPC
  • JSON-RPC request and responses must have the same ID
  • By returning nothing from the password_changed_for_user()method, the marshaller for the json-rpc requests automatically adds the header data including the correct request id
ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME
  • Zabbix HTTPAPI
  • zabbix_user module
ADDITIONAL INFORMATION

See the integration test for a playbook on how to reproduce the issue: tests/integration/targets/test_zabbix_user/tasks/change_api_user_pass.yml

* zabbix httpapi plugin - if login failed, it would not be attempted again by later tasks. fix now marks the connection as disconnected on failed login so that later tasks can try
* zabbix_user - if the api user's password was changed, session errors would occur in later tasks. fix will relogin to zabbix after changing password of the api user
@CircuitCipher
Copy link
Contributor Author

@pyrodie18 Thanks for running the tests. I will look into the failures and hopefully have a resolution later today.

* Remove "Internal" group from the admin pass modification task
* Don't attempt to reset back to default zabbix password, just update
  the password fact.  (default 'zabbix' is too short to reset to)
@CircuitCipher
Copy link
Contributor Author

So there are a few issues with the build as it stands. It will take me a few days to address these issues but I have a roadmap. Unfortunately its become quite a rabbit hole of bugs and or constraints.

I noted that tests are failing strangely for zabbix version 6.0. I will have to look into this further after I resolve the other issues.

Tests after zabbix_user module changes zabbix password fail

I see the tests are sharing the same zabbix instance and I thought maybe we could try rewriting the test so that its changing the password of another user. Unfortunately, due to how connections are reused, this does not trigger the required code branch.

My current approach is to use the zabbix_authentication module to change the passwd_check_rules so that we can reset the password back to the default "zabbix"

Unfortunately, there is a bug in the zabbix_authentication module when updating the passwd_check_rules and you cannot set it to an empty list to turn off all checks.

As such, I will be putting together the required tests and fixes to to address the sub-bugs and hopefully get this PR into the green

@BGmot
Copy link
Collaborator

BGmot commented Sep 2, 2024

Sorry for coming so late to this discussion (being busy with real life).
While I appreciate your efforts and so detailed description of the issue I don't like what you are proposing in this PR. Let me take a closer look at #1209

@CircuitCipher
Copy link
Contributor Author

Sorry for coming so late to this discussion (being busy with real life). While I appreciate your efforts and so detailed description of the issue I don't like what you are proposing in this PR. Let me take a closer look at #1209

No problem, let me know your preferred direction for solving this issue.

@BGmot
Copy link
Collaborator

BGmot commented Sep 4, 2024

The issue solved by #1386
Closing this one.

@BGmot BGmot closed this Sep 4, 2024
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.

Zabbix httpapi does not honor reset_connection
2 participants