bitwarden / clients

Bitwarden client apps (web, browser extension, desktop, and cli).
https://bitwarden.com
Other
9.34k stars 1.26k forks source link

Subdomain support #77

Closed WardsParadox closed 6 years ago

WardsParadox commented 7 years ago

Hello, Love Bitwarden and have swapped to it from Lastpass. I noticed that there is no support for separating sites based on the full domain. Bitwarden detects tech.example.com and forms.example.com to be the same site and offers both sets of logins for both sites. If a user could setup a URL rule to prevent this, that would be great.

laemm-line commented 6 years ago

Exactly the same problem here: I have a lot of subdomains for different applications, Bitwarden is almost unusable for those.

I would appreciate it very much if you could implement this feature soon.

piejanssens commented 6 years ago

@kspearrin Any update on the plans to implement one of the suggested approaches?

kspearrin commented 6 years ago

The initial plan is to create a string prefix value you can append to the URL field to enable various scenarios described in this issue.

ex:

This negates the need for UI elements to control the functions and we can create many different rules internally. It also allows us to specify rules on individual URIs if/when we add the ability to store multiple URIs per login item.

We then plan to add a UI toggle (checkbox) for the most likely used case such as "Use Full Hostname".

Attoy commented 6 years ago

Thanks for the update Kyle! Is there a beta channel we can subscribe to and test this feature?

kspearrin commented 6 years ago

@Attoy No. You would have to build the extension locally or manually download our real-time dev builds at https://ci.appveyor.com/project/bitwarden/browser/build/artifacts

But this work hasn't been started yet.

SylwesterZarebski commented 6 years ago

Thanks, Will You also provide syntax with wildcards/regexps like: https://git.*.internal.local/ for multiple sites?

kspearrin commented 6 years ago

@SylwesterZarebski I suppose we can just add a regex=something option and you can create whatever wildcard scenario you like. Seems like it could get dangerous though.

SylwesterZarebski commented 6 years ago

True, sharp knives could cut ;-). Thanks a lot.

piejanssens commented 6 years ago

Yes! REGEX 🥇 I'm all for it. regex=/something(/w+)is{0,4}regexinhere/

Couldn't you simply turn the string prefixes into a dropdown as a minimum for best UX? So you choose from the dropdown which is shown right in front of the URL field. Internally the value of the dropdown can be concatenated with the URL text as fullhostname=https://sub.domain.com:123/ in your example. You can then add a âť” icon to go to a wiki/help page where each of the option is explained.

fullhostname fulluri startswith basedomain regex

kriswilk commented 6 years ago

@kspearrin, I like the flexibility you're aiming for, but I feel @piejanssens has the right idea for the UI.

Adding a prefix within the URI field feels kludgy...mixing data with contextual info in a field that should probably only contain a URI. The field becomes a lot less portable with some proprietary prefix on it. For instance exported vault data would not be very friendly-looking to other apps.

But stepping back for a second...

Before you begin implementing more complicated options, maybe it's worth just changing the logic to the very simple "most specific" URI as described best by @wehrdo? Trying that out would not require any UI or structural changes but I think it would address the vast majority of cases immediately.

Keep up the great work!

favadi commented 6 years ago

I also think the regex might be too complicated for the value it will provide to users. The "most specific" strategy is enough for me.

SylwesterZarebski commented 6 years ago

Term "most specific" is too general to make any assumptions. For example, which of these definitions is most specific:

sub.domain.com/path/path2
www.sub.domain.com/path/

for address: www.sub.domain.com/path/path2/path3?

This 'most specifity' looks similar to CSS specificity which is very complex.

PS. It is fine by me, but will need to be documented extensively.

favadi commented 6 years ago

This task is about subdomain, so the latter is most specific in your case. Although, someone posted above that we should treat www as a special case.

Fonant commented 6 years ago

How about: Longest matching hostname or sub-hostname working from right to left. Followed by longest matching path or sub-path working left to right. If port number is specified, that has to match.

Different logins per host name are much more commonly found than different logins per path on the same host name.

Entries:

A: domain.com B: www.domain.com C: shop.domain.com D: local.shop.domain.com E: domain.com/testing F: shop.domain.com/admin G: domain.com/testing/results

Then:

http://www.domain.com/anything/here => B http://www.domain.com/testing => B http://www.domain.com/admin =>B http://shop.domain.com/anything/here => C/F => C http://shop.domain.com/testing => C/F => C http://shop.domain.com/admin => C/F => F http://domain.com/anything/here =>A/E/G => A http://domain.com/testing =>A/E/G => E http://domain.com/testing/anything =>A/E/G => E http://domain.com/testing/results =>A/E/G => G

The logic could perhaps ignore "www." by stripping it off both the test URL and the stored hostnames, for the purposes of this logic?

kriswilk commented 6 years ago

Let's not make this seem more complicated than it needs to be...

1) Compare the visited HOSTNAME (x.y.z.tld) against each vault entry. Keep only the one(s) which match most closely, working from RIGHT TO LEFT (tld, then domain, then subdomains, ...)

2) If the visited site includes a specific PORT (80, 23, ...), reduce the matching pool further if any vault entries share the same port.

2) If multiple matches still exist, examine the PATH (/path1/path2/file.ext?blah=blah...) and determine which entr(y/ies) match by working LEFT to RIGHT, i.e. most matching characters.

Is this not enough??

Fonant commented 6 years ago

That was what I was getting at, although I got my "shortest" and "longest" the wrong way round... and I forgot about port numbers...

I've edited my post above to correct the wording.

phynias commented 6 years ago

just decide on a way and go with it. it's been a year. not trying to be rude, but even a basic option could have been done by now. i switched to bitwarden from lastpass but have to switch back. WAY to many subdomains at home and work to ever make this useable.

Attoy commented 6 years ago

@kspearrin do you have an ETA to share with us?

psylenced commented 6 years ago

The initial plan is to create a string prefix value you can append to the URL field to enable various scenarios described in this issue.

fullhostname, fulluri, startswith, basedomain

Just one further thing. I think there needs to be the ability to add certain domains / string matches to be "a domain that needs a prefix".

If you auto-add (via the standard dropdown bar) the site's name/description will default to the shortened version. You then will need to go search for it (often with dozens of matches), go into the entry and then manually add the prefix.

If I add workdomain.com to the "domain that needs a prefix" list - then when I auto-add, the UI will know to automatically open the "edit" version of that site, so I can add in the prefix.

I guess also the name value needs to change based on the prefix too, so every entry is not just workdomain.com, as there will by default be many matches all with the same name showing on the UI.

mikemeier commented 6 years ago

Where can we donate money to have this feature implemented? 🤔

Attoy commented 6 years ago

@mikemeier I think the best way to support BitWarden is subscribe to premium membership. I hope @kspearrin will give us an ETA sooner than later :)

samuelebistoletti commented 6 years ago

Any news?

dominicpratt commented 6 years ago

I really hope this feature get's implemented soon... stuck on LastPass until then. :(

kspearrin commented 6 years ago

Now that the desktop app is released, this is being looked at again.

Attoy commented 6 years ago

Awesome!

kspearrin commented 6 years ago

Here's a quick screenshot of what's being built for this (also multi-uri support being added too). Does calling this "MatchAuto-fill Detection" make sense?

image

psylenced commented 6 years ago

Now this is being looked at again - just a comment.

I can see this being needed in 2 areas:

1) Editing an already saved login so it only shows as a candidate when the match passes. 2) Adding a global set of values, for a new added domain so it defaults correctly. 2b) Adding a global set of values so existing logins will only exactly match.

Also when adding a new login which falls under number 2 above - the name of the login should default to the matching segment (sub.domain.com:88/path) and not just domain.com.

SylwesterZarebski commented 6 years ago

For me, proposition looks great, just what i need.

Attoy commented 6 years ago

@kspearrin as "Auto-fill detection" I'd think something related to the detection of fields on the page to be auto-filled. I'd say "URL match detection" is a better name. Just my 2 cent.

talha131 commented 6 years ago

@kspearrin feature wise it looks great. Can't wait to have it.

But from end user POV, it is complicated. It's very technical. An end user is unaware of terms like base domain, hostname, and regular expression, much less understand them to make a choice.

Perhaps, hide these options under "Advanced URL match detection". Or highlight the matching part of the url when users selection changes. Like if "Base domain" is selected, then the base domain domain part of the url gets a different color. This will offer visual cue to the user.

kspearrin commented 6 years ago

@talha131 I agree but I am not sure how I can properly present that in the UI. Where would this advanced uri options button be? I don’t think it’s possible to selectively highlight uri text in an text input field.

kspearrin commented 6 years ago

@talha131 How about something like this?

image

talha131 commented 6 years ago

@kspearrin Much better. And thanks for listening and responding to feedback so quickly. More power to you!

kriswilk commented 6 years ago

This looks like it will handle just about anyone's use pattern. I like it.

Perhaps instead of the gear icon you can put a "help" link next to the drop-down box which goes directly to a help page describing the various options. That would allow users to self-educate when they are curious. Personally I'd prefer not to have to click the gear just to see the option I had set for each URI.

Also, maybe I'm missing something but what's the logic behind the "do not enter" icon? I presume it's to delete an entry, but if so, I'd say you should use the same "X" icon that is used for custom fields...at least that's what I see in the chrome extension.

kriswilk commented 6 years ago

One other comment/question: how will this new functionality interact with the existing global equivalent domains feature?

kspearrin commented 6 years ago

@kriswilk Haven't got that far yet.

kspearrin commented 6 years ago

@kriswilk I made it so that the dropdown options are only hidden by default if there is no value set on them yet (default option). Once you set a value for them it will always show by default.

kriswilk commented 6 years ago

I made it so that the dropdown options are only hidden by default if there is no value set on them yet (default option). Once you set a value for them it will always show by default.

@kspearrin Now I understand the screenshot. Thanks for the clarification, that sounds good.

kobelobster commented 6 years ago

Thank you for the feedback, the support and the communication @kspearrin. Now my part is due as promised:

image

kspearrin commented 6 years ago

"Full Hostname" has been renamed to just "Host", which is the URL property that we are actually checking against. Defined here: https://developer.mozilla.org/en-US/docs/Web/API/HTMLHyperlinkElementUtils/host

Hostname doesn't include ports.

zestysoft commented 6 years ago

That URL doesn't contain "Full Hostname" anywhere in the text. Is that even a thing? Did you mean FQDN? I couldn't find "rename" either.

The URL you provided has examples that clearly show ports being used:

anchor.href = "https://developer.mozilla.org:4097/en-US/HTMLHyperlinkElementUtils.host" anchor.host == "developer.mozilla.org:4097"

Did you mean to give this URL instead? https://developer.mozilla.org/en-US/docs/Web/API/HTMLHyperlinkElementUtils/hostname

The "HTML Living Standard" specification link in that URL you provided, states this:

hyperlink . host Returns the hyperlink's URL's host and port (if different from the default port for the scheme).

Can be set, to change the URL's host and port.

hyperlink . hostname Returns the hyperlink's URL's host.

Can be set, to change the URL's host.

So which URL property is Bitwarden checking against? Host or hostname? Can't it choose one or the other?

kspearrin commented 6 years ago

As mentioned in the previous comment, we are using host, which is what I linked and why it was renamed from full hostname.

zestysoft commented 6 years ago

So if it's using host, then it can check for ports. I guess I'm confused why this is even being discussed -- you said:

Hostname doesn't include ports.

Yet you aren't using hostname. Did someone else here want to use that instead?

kspearrin commented 6 years ago

No, we were just trying to figure out what the proper name for the different options were. I didn’t originally know what the name for hostname + port was so I made up a name called “full hostname”.

danh-fissara commented 6 years ago

I'm not convinced the document linked above is definitive as to whether 'host' includes the port, as it's documenting browser behaviour, rather than the RFC that describes URIs.

These both specify 'host' as not including the port: https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax https://developer.mozilla.org/en-US/docs/Learn/Common_questions/What_is_a_URL

Either way, as this is a browser security extension I don't think it makes sense to compare the port - the relevant bits for TLS are the protocol and host (not including the port).

kspearrin commented 6 years ago

The behavior of the option is to match a resource where the URI's host[ + ':' + port] are the same. Browser APIs define this as the host of a URL (spec linked above, hostname[ + ':' + port]), so we are using that property here.

I don't know if there is an official term for this value, but since the browser APIs call it the "host" that is what I am currently going with. If someone has a better alternative term for this option in the dropdown menu I'm open for feedback.

isobit commented 6 years ago

FWIW the Web Location interface calls hostname+port "host": https://developer.mozilla.org/en-US/docs/Web/API/Location

It's unfortunate RFC 3986 doesn't give a name for hostname+port.

kspearrin commented 6 years ago

Also, is there a proper term for the "Base Domain" (what I currently call the default option)? This is another term that I just made up. Ex:

https://google.com => google.com https://sub.google.com => google.com https://sub.sub2.google.com => google.com https://sub.google.com:4000 => google.com https://localhost => localhost

zestysoft commented 6 years ago

Either way, as this is a browser security extension I don't think it makes sense to compare the port - the relevant bits for TLS are the protocol and host (not including the port).

In the world of Docker and virtual machines, port redirection is a thing. Matching based on port is required in these circumstances.

zestysoft commented 6 years ago

Also, is there a proper name for the "Base Domain" (what I currently call the default option)?

That's how I've always understood it. The TLD (Top Level Domain) + a name.

I think giving examples is a great idea though -- I know that in some of the forms I've filled out, instead of the field being a blank white box, it has got some grey text examples that are immediately overwritten when typing into the box.