yardnsm / tmux-1password

:key: Access your 1Password login items within tmux!
MIT License
252 stars 24 forks source link

Feature: Local cache for op items #16

Open delucca opened 4 years ago

delucca commented 4 years ago

☕ 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

camspiers commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

Oh right. Excellent!

delucca commented 4 years ago

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

camspiers commented 4 years ago

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

delucca commented 4 years ago

@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 commented 4 years ago

@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

delucca commented 4 years ago

@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 commented 4 years ago

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

delucca commented 4 years ago

@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 commented 4 years ago

@yardnsm done :) Waiting for you approval

yardnsm commented 4 years ago

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

delucca commented 4 years ago

Great! Thanks.

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

delucca commented 4 years ago

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

delucca commented 4 years ago

@yardnsm will this branch ever be merged? :)

tapayne88 commented 4 years ago

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?