mode/password: filename-based usernames for pass #3429
Conversation
|
Some checklist-related notes:
|
|
Oh, and fair warning: I've never worked on a Common Lisp project before. Please assume that any fishy code you see is the product of inexperience and nothing particularly avant garde. All critique is welcome and highly useful. |
chaorace
force-pushed
the
password-store-filename-usernames
branch
2 times, most recently
from
3b3272a
to
c66c7a9
Compare
|
@aadcg All pending feedback should now be addressed. Please take a look when you can, thanks! |
| @@ -69,22 +79,48 @@ The first line (the password) is skipped." | |||
| (defvar *username-keys* '("login" "user" "username") | |||
| "A list of string keys used to find the `pass' username in `clip-username'.") | |||
|
|
|||
| (defmethod clip-username ((password-interface password-store-interface) &key password-name service) | |||
| "Save the multiline entry that's prefixed with on of the `*username-keys*' to clipboard. | |||
| (defun username-from-name (password-name) | |||
There was a problem hiding this comment.
Can we come up with a better name for the function and argument? It seems a bit misleading.
There was a problem hiding this comment.
@aadcg Here's some candidates, do any stand out to you?
- username-from-filename
- username-from-path
- username-fallback
There was a problem hiding this comment.
As for the argument name... I don't know what to say. It's identical to the name given to the exact same value via the pre-existing clip-username, clip-password functions. I don't really have a horse in this race but I think choosing a new name for it is beyond the scope of my responsibilities and the work I'm doing for this PR.
There was a problem hiding this comment.
Since it takes a path as a string I'd suggest calling it username-from-path and renaming the argument to path.
Indeed, as you correctly note, it is called in clip-username with an argument called password-name (which I find odd), but that is beyond the scope of this PR. My goal is to guarantee that the added code is maintainable and easily understood in the future. Note that I am not the author of the password-manager library.
| 2. If no match could be found, use the credential filename as a fallback. | ||
|
|
||
| When nil, Nyxt always immediately uses the fallback strategy. | ||
| If your store doesn't utilize username keys, this skips credential decryption.")) |
There was a problem hiding this comment.
I gave it a second thought.
The slot name needs to be named as a predicate, i.e. it must have the suffic -p.
Since I am not a pass user please help me figure out an adequate default behavior. Does it make sense to scan for both the content and the directory tree structure? Why is that better than scanning the content only, as before this PR? Am I right that you'd use this mode with this slot set to nil, such that the username is computed exclusively via username-from-name?
Upon answering, we can decide the slot name.
There was a problem hiding this comment.
pass is -- in what might be described as typical GNU fashion -- highly unprescriptive. As far as the software is concerned, there are exactly two distinct types of data:
- The filename & path relative to the root of the store
- The password text
Higher-level concepts, such as URLs and usernames, exist at a level of abstraction that pass is unconcerned with -- note how the manual entry contains zero mentions of the word "username". The original author's website does touch on the author's personally preferred schema for storing extra metadata...
One approach is to use the multi-line functionality of pass (--multiline or -m in insert), and store the password itself on the first line of the file, and the additional information on subsequent lines
... though it should be noted that this statement is immediately preceded by the disclaimer that the software itself prescribes no notion of schema whatsoever:
The password store does not impose any particular schema or type of organization of your data, as it is simply a flat text file, which can contain arbitrary data.
I lay this all out so that I can assert a very important point: it is impossible to use pass as an internet password manager without making opinionated assumptions about how the user chooses to utilize pass. At best, all one can do is study the conventions invented by the surrounding pass-based tooling ecosystem and accomodate the most common approaches.
The most common convention is to place the website URL & username metadata directly in the path of the credential. The case for storing these like so are, respectively:
- URL: If the URL metadata were instead stored within the password text, then password managers would be required to decrypt every single password in the store when attempting to produce an exhausting list of credentials for any given website. This scales very badly with larger password stores.
- Username: There exist several common use-cases where users may have multiple credentials associated with the same website (e.g.: "Single Sign On" portals). For this reason, some additional disambiguating value needs to exist in order for the pattern to accomodate the concept of multiple credential files being applicable to the same URL. True, the disambiguating value doesn't strictly need to be the username... but something needs to be used and usernames make for good unique identifiers.
The main point of contention here among implementations has historically been how these two metadata elements are organized in the credential path. Some implementations used flat directory structures while others used nested structures. Luckily, these two approaches are not mutually exclusive, so the tooling ecosystem eventually settled on supporting use of either of these patterns (even both simultaneously!). This dual-support convention is most canonically codified in the Emacs documentation, though it should be noted that many password management tools in the pass ecosystem expect the same patterns (example, example).
As you may have noticed, both of the previously linked examples also support getting the username via a "field value". These "field values" are yet another invented convention with various competing implementations. The general consensus for how such username keys should be named is relatively weak, so implementations which support extracting usernames via field value generally try to pack in a customizable list of commonly used key variants.
Right, so with all of that baggage out of the way, let's circle back and address your questions...
Does it make sense to scan for both the content and the directory tree structure?
Yes, the reason for this being that it's the easiest way to support both styles without making the user do additional configuration. Indeed, there are many common GPG configurations out there (some of them even secure!) which do not require additional input from the user in order to decrypt credentials once the keyring (and/or hardware key) has been unlocked. For these users, simply attempting both approaches is painless and not something they would be overly concerned about.
Why is that better than scanning the content only, as before this PR?
Because storing usernames as "field values" is no more official or correct than storing usernames as part of the file path. More importantly, I use the pathname-based approach and I want Nyxt to support my (quite common!) password store schema. The only way to logically square excluding support for this on the grounds of canonicity would be removing support for both schemas and always returning "" for the username.
Am I right that you'd use this mode with this slot set to nil, such that the username is computed exclusively via username-from-name?
Correct. This is because I use hardware-based encryption which actively withholds decryption from GPG until I manually reach across my desk and press a physical button. In other words: without this feature, I'd have to engage in this awkward ritual twice as often as would be otherwise necessary -- once (unnecessarily!) for the username and once again for the password. Indeed, this is one of the better justifications for not storing the username as a credential field value.
There was a problem hiding this comment.
Thanks for the detailed explanation.
I suggest the following.
Name the slot username-scan-method. It takes the following keywords as values: :credential-content-and-path, :credential-content and :credential-path. By default it is set to :credential-content-and-path and it follows the logic you have implemented (checks the content and fallbacks on the path approach). For your use-case, you'd set it to credential-path.
Does that make sense? I wonder if it makes sense to allow setting it to :credential-content.
There was a problem hiding this comment.
@aadcg Hmm... I can't imagine any hypothetical reasons anyone would want such a :credential-content option other than offering strict reverse-compatibility.
If you ask me, I don't personally think the added level of technical completeness really justifies the (admitedly small) amount of additional code complexity... but I'll happily ignore that instinct and implement the option should you deem it important -- it's your codebase after all 😉
There was a problem hiding this comment.
Feel free to let the implementation handle :credential-content-and-path and :credential-path exclusively. Thanks!
Fixes atlas-engineer#3293 The new filename-based username check is used as a fallback to the current key-based username behavior. Users who only want the filename-based behavior can disable scanning the credential for username keys via the new scan-for-username-entries slot. Disabling scanning for a key-based username is particularly useful in use-cases where credential files are encrypted and require touch verification to open.
chaorace
force-pushed
the
password-store-filename-usernames
branch
from
d5f99b0
to
beaf3e6
Compare
There was a problem hiding this comment.
@chaorace Sorry for the late reply.
Thanks for the information shared and check my relies.
Note that your first commit isn't using the correct pathname in the commit message. Suggestion:
libraries/password-manager/password-pass: Refactor username scan.
|
@chaorace would you like to finish the PR or should I take over it? Thanks. |
Description
Introduces a new filename-based code pathway for determining the username of a GNU password-store credential. This new pathway simply uses the credential file name (not including extension) as the username.
By default, the new pathway is only used as a fallback strategy for when no matching username keys could be found using the current strategy, though users who only want the new filename-based behavior can entirely skip using the original strategy via the new
scan-for-username-entriesslot. Disabling scanning credential files for a key-based username is particularly useful in use-cases where the store credentials are encrypted and would require touch verification to open.In the special case that a credential is on the root directory of the password store, we also trim off the domain signifier to match the behavior of other password-store based utilities (e.g.: "[email protected]" becomes "me", "me@[email protected]" becomes "[email protected]")
Fixes #3293
Checklist:
(asdf:test-system :nyxt/<renderer>))