browserpass / browserpass-legacy

Legacy Browserpass repo, development is now happening at:
https://github.com/browserpass/browserpass-extension
MIT License
999 stars 80 forks source link

Document how precisely the matching of domains - credentials works and why #272

Closed ipundit closed 5 years ago

ipundit commented 6 years ago

General information Operating system + version: Windows 10 64 bit Browser + version: Chrome 67.0.3396.99 64 bit Information about the host app: How did you install it: Prebuilt binary If installed an official release, put a version: 2.0.21 Information about the browser extension: Chrome webstore

Exact steps to reproduce the problem

  1. Create the following structure in pass: password storage structure
    • Note that the google.com folder is necessary for the google.com filter mode to pick up these logins on the accounts.google.com page
    • Note that the alternative way for the google.com filter mode to pick up these logins is to name the logins google.com instead of Anderson contract, Davidson contract, Alibe, Bob, or Charlie. This works when there is only one google.com login per folder/badge, but not in this demonstration case where a user has multiple logins (Anderson contract, Davidson contract, Alibe, Bob, Charlie) under multiple password stores (Consulting, Family) for one site
  2. Replicate that Custom store/badge structure in browserpass options: set up custom store locations
  3. Browse to https://accounts.google.com/ServiceLogin/signinchooser
  4. Ctrl+Shift+L remove google com prefix
    • Bug: remove the extraneous google.com\ prefix from all the logins
erayd commented 6 years ago

Could you clarify why you want this removed?

It doesn't look like a bug to me - this is deliberately intended behavior, because there can be logins from other domains / subdomains shown alongside what your screenshot illustrates, and the domain is necessary in such a case to disambiguate the search results. It's not just there for matching.

ipundit commented 6 years ago

In step 3, the user has browsed to a .google.com domain, and browserpass has helpfully filtered out all logins except the .google.com domains. Therefore, there is nothing to disambiguate and we may as well suppress the google.com prefix as it's already implied that all return values are under *.google.com by the grey accounts.google.com parent header.

Now if the user backspaces through the accounts.google.com filter to do a search on her entire passwords-store, then the google.com prefix is necessary for disambiguation between the logins for multiple sites. But if she's browsed to *.google.com and wants to choose only among the google.com logins, then the google.com\ prefix is redundant and should be suppressed.

Furthermore, I think the grey header should say "google.com" rather than "accounts.google.com" since you're matching *.google.com. This is especially important for logging into www.example.com vs example.com - the user (and often, the website too!) expects the same login to work for the naked url and the www subdomain because it's the same site to them. If so, ignore subdomains for matching (which I think browserpass already does), but also suppress the subdomain in the grey header.

maximbaz commented 6 years ago

"accounts.google.com" in the grey header and "google.com\" in entry names are two conceptually different things, the first one is "what you searched for", the second one is "what we found". Now, "what we found" does not necessarily correspond to "what you searched", you can have these four entries in your password store and browserpass will discover all 4 of them for "accounts.google.com":

.password-store/
    accounts.google.com/
        Alice.gpg
    google.com/
        Bob.gpg
    accounts.google.com.gpg
    google.com.gpg

The matching algorithm will search for exact matches of a given search request as well as all parent child levels, the assumption is that if you create an entry on a parent level, its credentials may be valid on multiple subdomains of this website. So search for "accounts.google.com" and it will find entries for both "accounts.google.com" and "google.com", but not for "calendar.google.com"; search for "google.com" and it will only find entries for "google.com". So search for "accounts.google.com" and it will find entry only for "accounts.google.com", but not for "google.com" or "calendar.google.com"; search for "google.com" and it will find entries for "google.com" as well as all child subdomains.

In summary, I wouldn't do any magic to the result entry names, this may have a benefit of saving a bit of visual space, but will lead to many other issues.

erayd commented 6 years ago

I agree with @maximbaz on this one.

Additionally, in v3, it will also display logins from other domains you may have used on that particular site previously - it keeps track of where you used things so that you can use them there again without having to search every time.

While suppression might be cosmetically nicer in the specific single case of many logins using the same domain as the site, and nothing else matching - it seems to me that such suppression would overall do more harm than good.

ipundit commented 6 years ago

Thanks @maximbaz for the clarification of why the searching algorithm was not working as I expect. Can you document this somewhere for our benefit? It would have saved me some confusion.

Here's a case where the

search for "google.com" and it will only find entries for "google.com".

was unexpected for me; Google is actually a good example of this: When I want to login to google, I just search for google.com. I didn't even realize until now that I was actually logging into https://accounts.google.com because Google transparently redirects me to a subdomain when I click login. In my mental map, *.google.com is all just google.com, and I let Google decide to redirect me to whatever subdomain they wish as it's all one site to me.

Lastpass handles this by treating *.google.com as all one domain for matching purposes. It resolves the visual namespace collision by showing the user name of the login on the second line as shown in the following screenshot, which I consider to be a much more intuitive interface than browserpass' current implementation as shown on the right: lastpass vs browserpass2

Can we have the convenience of seeing only the websites that match github.com while on github.com, coupled with a way of distinguishing between different logins to that site (preferably by user name as per Lastpass' implementation) without having everything concatenated in duplicate in one long line per login?

maximbaz commented 6 years ago

Of course I managed to screw up and wrote a completely wrong example. When you have an entry called "google.com", it is assumed that these credentials work on "google.com" as well as all of it's domains. But when you have an entry "calendar.google.com", it will not be shown for "accounts.google.com" or "google.com". Sorry, and I agree we should absolutely document this.

maximbaz commented 6 years ago

As for showing the usernames in the popup, this can be achieved if you choose not to encrypt the user name and put it as a file name, e.g.:

.password-store/
    google.com/
        john.doe.gpg

That way you will see the entry with the username google.com/john.doe.

But, if you choose to encrypt the username and so put it inside the password entry, browserpass will not decrypt your entire password store just to show you the popup, it will take very long (I measured once, ~2 minutes to decrypt my entire password store when using hardware key such as yubikey).

ipundit commented 6 years ago

Yes, and that's what motivated me to open this issue in the first place; I wanted two things: 1) Domain-level filtering so that I only see the logins specific to the domain that I'm on (this requires the google.com folder) 2) Meaningful names to distinguish between the different logins I have for google.com (john.doe.gpg)

The side effect of the two combined together is to have the google.com/ prefix in all the search results. But with @maximbaz's explanation, it makes more sense - and yes, it should be documented to explain why. I still don't like the repeated prefix, but perhaps Lastpass' two line solution is best?

Instead of what we have now on the left, I would much prefer the interface of the right: one line interface

The first line is the directory path to the key in the store. It could be google.com, or accounts.google.com, and is what browserpass uses to filter search results to a particular domain.

The second line is the name of the key, and would be used to disambiguate multiple logins for a site. It could be whatever the use chooses, whether it be google.com or fred@example.com or Fred - leave the choice to the user. Since the user already knows he's on google.com, I would make the font of the key name larger than its location in the password-store directory as shown in the above example.

For the somewhat contrived example of:

Personal password-store

you would get: contrived example

maximbaz commented 6 years ago

Interesting concept, I will think about it and all the edge cases and decide before we release v3, for now I created #275 so that I don't forget about it.

stefanct commented 5 years ago

The main problem (unclear matching of entries) also bit me. I stored the key as github.gpg w/o the TLD. Since the url field contains it anyway I was expecting it to be found. Please document this clearly! Maybe in some getting started section that explains a basic approach how to lay out the files and their contents.

I would also welcome an extended search within all gpg files when no match can be found as is. This might be slow in some instances (I seriously doubt that the 2 minutes are representative at all for most users TBH) but it renders the freedom of choosing file names useless if not harmful.

maximbaz commented 5 years ago

Please document this clearly! Maybe in some getting started section that explains a basic approach how to lay out the files and their contents.

Definitely, that's what this issue is for πŸ˜‰

I would also welcome an extended search within all gpg files when no match can be found as is. This might be slow in some instances (I seriously doubt that the 2 minutes are representative at all for most users TBH) but it renders the freedom of choosing file names useless if not harmful.

That's a definitive no-go, however your specific use-case has been solved differently in the next major version of browserpass β€” it will remember which passwords you used on which domains and show them next time in the popup even if they don't match the domain.

So if you have a password entry called github.gpg and you navigate to github.com, at first just like today browserpass will not show github among the list of passwords (to protect you from phishing attacks), however if you specifically search for github and use it to login, next time you open github.com the entry github will be present in the popup.

We are a bit delayed with the v3 release, but I really hope we get it released soon. Wink wink @erayd 😜

maximbaz commented 5 years ago

Documented in this section of Browserpass v3 README: https://github.com/browserpass/browserpass-extension#password-matching-and-sorting

By the way, if you can test Browserpass v3 now, please share your feedback here: https://github.com/browserpass/browserpass/issues/331