c0sco / ansible-modules-bitwarden

Bitwarden integrations for Ansible
GNU General Public License v3.0
109 stars 45 forks source link

Invalidation of sessions to due frequency of calls #23

Open jakubgs opened 2 years ago

jakubgs commented 2 years ago

I've been using this plugin for a while and I appreciate it being freely available. I've been seeing issues with sessions invalidating.

Specifically it happens when I run multiple playbooks on big numbers of hosts, which causes the API to hit some kind of rate llimit:

An unhandled exception occurred while templating '{{lookup("bitwarden", "example", field="username")}}'.
Error was a <class 'ansible.errors.AnsibleError'>,
original message: An unhandled exception occurred while running the lookup plugin 'bitwarden'.
Error was a <class 'ansible.errors.AnsibleError'>,
original message: Error accessing Bitwarden vault. Run 'bw login' to login.

This becomes quite an issue when trying to make big changes rapidly.

jakubgs commented 2 years ago

One issue appears to be the fact that the plugin calls both bw --version and bw status for each credential required:

image

Which is pretty absurd if you ask me.

jakubgs commented 2 years ago

The first issue can be easily fixed by using shutil.which to check if the tool is available:

diff --git a/ansible/lookup_plugins/bitwarden.py b/ansible/lookup_plugins/bitwarden.py
index 48c3c1f..e78aa63 100755
--- a/ansible/lookup_plugins/bitwarden.py
+++ b/ansible/lookup_plugins/bitwarden.py
@@ -19,6 +19,7 @@ import json
 import os
 import sys

+from shutil import which
 from subprocess import Popen, PIPE, STDOUT, check_output

 from ansible.errors import AnsibleError
@@ -70,9 +71,7 @@ class Bitwarden(object):
     def __init__(self, path):
         self._cli_path = path
         self._bw_session = ""
-        try:
-            check_output([self._cli_path, "--version"])
-        except OSError:
+        if which("bw") is None:
             raise AnsibleError("Command not found: {0}".format(self._cli_path))

     @property

Which gets rid of the first batch of pointless mass bw --version calls.

jakubgs commented 2 years ago

If we look at BitWarden CLI source we can see how checking status works:

  private async status(): Promise<string> {
    const authed = await this.stateService.getIsAuthenticated();
    if (!authed) {
      return "unauthenticated";
    }

    const isLocked = await this.vaultTimeoutService.isLocked();
    return isLocked ? "locked" : "unlocked";
  }

https://github.com/bitwarden/cli/blob/2ae2fdfd/src/commands/status.command.ts#L44-L52

Which calls their jslib:

  async getIsAuthenticated(options?: StorageOptions): Promise<boolean> {
    return (await this.getAccessToken(options)) != null && (await this.getUserId(options)) != null;
  }

https://github.com/bitwarden/jslib/blob/2ae2fdfd/common/src/services/state.service.ts#L1519-L1521

So it tries to get an access token from the API and check user ID to verify state of session.

jakubgs commented 2 years ago

The thing is, I don't see a reason to check bw status if calls like bw get item fail with the same unauthenticated error anyway.

What's the point of the bw status call? Am I missing something? It seems to bring no value and just adds unnecessary calls.

jakubgs commented 2 years ago

I've made my own fork, and applied the two fixes recommended here(among others):

It works well for me. Maybe it is useful to someone else.