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

Feature: Local cache for op items #16

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

Conversation

delucca
Copy link

@delucca delucca commented Feb 1, 2020

☕ Purpose

After I started using #13, I've noticed that it takes too long if every time I fetch passwords my computer needs to go to the 1Password server to fetch them.

To fix this, I've created this PR, which saves a local cache containing the result of op list-items. The cache is available for 6 hours :)

This PR is based on #13, so some of the code here may looks different but it is actually the implementation of #13. As soon as that PR is merged, it would be more clear to see the diffs.

🧐 Checklist

  • Adds a function to save the cache
  • Adds a condition to use cache instead of fetch, if available
  • Adds a condition to remove the cache if it is older than 6hours

Cam Spiers and others added 6 commits February 1, 2020 04:08
…q-filter to disable and customize filtering
New tmux options @1password-enabled-url-filter and @1password-items-j…
To cache op list-items results I must first create a file everytime I get any
return from the command itself.

On this commit, I am creating the logic to save the return from op list-items
into a given file for further usage.

Now, I just need to add the logic to fetch from this file instead, if the file
is new enought.
Now that I can cache the op list-items result, I need to use the cache instead
of getting it online, if it exists. It is important to have a TTL on the cache
file too.

On this commit I'm going to add the logic to use the cache file (if applicable)
and also create a rule to use it only when the cache is recent.

Now, I've finished the feature and can start using it.
@camspiers
Copy link

While I love the idea of caching, putting all of my incredibly sensitive passwords in an unencrypted file in a directory that every program running on my computer has read access to seems like a really bad idea, and not something I would be comfortable with from a security perspective.

@delucca
Copy link
Author

delucca commented Feb 1, 2020

@camspiers the file contains only the title and ID of the account :) it does not contain the password itself.

The password is fetched by the plug-in afterwards, when you select which account you want to get

@delucca
Copy link
Author

delucca commented Feb 1, 2020

Just to give more details, the data I'm caching is the return of op list-items after being filtered by jq, which results only in tuples containing: (Account title, Account ID)

This is not sensitive data, so there is no need to encrypt it.

Even if someone stole your file, they would need your 1Password password to fetch the actual data for your account. So, in my opinion, there is no need for any extra security step here

@camspiers
Copy link

Oh right. Excellent!

@delucca delucca requested a review from yardnsm February 1, 2020 16:48
@delucca
Copy link
Author

delucca commented Feb 1, 2020

@yardnsm can you take a look at this please? :D thanks!

@camspiers
Copy link

btw, apologies for the false assumption above. This feature is going to be really valuable. Thanks!

Copy link
Owner

@yardnsm yardnsm left a comment

Choose a reason for hiding this comment

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

Nice! But what will happen when:

  • The user changes a subdomain (using @1password-subdomain)?
  • The user changes a vault (using @1password-vault)?
  • The user updates his custom JQ filter?
  • The user adds passwords to his vault?

To solve all of these problems, I suggest adding a new tmux keybinding that will clear that cache file and run the normal flow. I would like to have prefix + U, but it's already taken by tpm 😔 Any ideas?

Also, I was originally meant to add a 1pass support (which have it's own secure caching mechanism even for the passwords). It's documented (very roughly) in #6, but in the meantime I think your solution is awesome!

scripts/main.sh Outdated Show resolved Hide resolved
scripts/main.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scripts/main.sh Outdated Show resolved Hide resolved
scripts/main.sh Outdated Show resolved Hide resolved
@delucca
Copy link
Author

delucca commented Feb 1, 2020

@yardnsm nice idea! I'm going to search for a proper keybinding to use!

About 1pass, in my opinion it is an awesome tool, but too hard to setup (I tried to setup it many times and there is a bunch of issues with it) and it doesn't adds too much value to op (in my opinion).

So, this could be a good alternative to those who won't use 1pass

@delucca
Copy link
Author

delucca commented Feb 2, 2020

@yardnsm I'm going to take a look on your suggestions on the following days.

Also, I've noticed another problem. Since we're just asking for user password during the fetch_items, if the user session has expired after selecting a cached item the user would not be able to fetch the password itself.

I must also fix this by asking for the password during afterwards if the session has expired

odelucca added 3 commits February 7, 2020 02:24
During the PR review, some contributors of tmux-1password suggested to bind a
key to clear the cache from the local caching.

On this commit I've created all the required scripts and configurations to
create that binding.

Now we should be able to clear our cache with prefix + R
During the PR, I've received some suggestion to improve the code.

On this commit, I've added those suggestions.
During the PR, I've received some suggestion to improve the code.

On this commit, I've added those suggestions.
@delucca
Copy link
Author

delucca commented Feb 7, 2020

@yardnsm I've updated the code :) can you please review it and merge with master?

I've also updated the tmux-token path (from $HOME to /tmp), to match the pattern of storing temporary data on the right place ($HOME should not be used for tmp data)

The only thing that I wasn't able to do was add a condition to login the user if the session expired. op doesn't provide a command to validate the current session, so I reduced the CACHE_TTL to 30 minutos in order to match the session token TTL.

@delucca delucca requested a review from yardnsm February 7, 2020 14:10
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
plugin.tmux Outdated Show resolved Hide resolved
scripts/main.sh Outdated Show resolved Hide resolved
@delucca
Copy link
Author

delucca commented Feb 11, 2020

@yardnsm thanks for the review, I'm going to take a look on your comments.

@delucca
Copy link
Author

delucca commented Feb 11, 2020

@yardnsm I've fixed your comments, and also fixed a bug.

The cache was being automatically cleared before fetch_items. Now it automatically clears the cache, before fetching items, if the cache is old enought. :)

Waiting for you review!

@delucca delucca requested a review from yardnsm February 11, 2020 00:50
Copy link
Owner

@yardnsm yardnsm left a comment

Choose a reason for hiding this comment

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

Looking good!

plugin.tmux Outdated Show resolved Hide resolved
scripts/main.sh Outdated Show resolved Hide resolved
@delucca
Copy link
Author

delucca commented Feb 12, 2020

@yardnsm done :) Waiting for you approval

@delucca delucca requested a review from yardnsm February 12, 2020 01:36
Copy link
Owner

@yardnsm yardnsm left a comment

Choose a reason for hiding this comment

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

Amazing!

As this PR branches from #13, I'll wait for it to be merged first. However, you can revert the changes from #13 and I'll merge it anyway.

@yardnsm
Copy link
Owner

yardnsm commented Feb 24, 2020

I've merged #18, which continues #13. You can revert the changes from #13 and it should be fine 👌

@delucca
Copy link
Author

delucca commented Feb 25, 2020

Great! Thanks.

I'm going to revert it in the next few days and get in touch with you ;)

@delucca
Copy link
Author

delucca commented Mar 19, 2020

@yardnsm sorry for the late reply. I've just finished it and you can merge right now ;) Thanks!

@delucca
Copy link
Author

delucca commented Apr 17, 2020

@yardnsm will this branch ever be merged? :)

@tapayne88
Copy link
Contributor

This looks good, great work @odelucca! Any chance of this being merged @yardnsm?

Also, do you think it's worth updating the permissions of the temporary files so they can't be read by other system users?

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.

4 participants