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.

Sign up for GitHub

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

mode/password: filename-based usernames for pass #3429

Closed
wants to merge 2 commits into from
Closed

mode/password: filename-based usernames for pass #3429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ffda32
mode/password: filename-based usernames for pass
chaorace Jul 7, 2024
beaf3e6
changelog: Mention change to pass interface
chaorace Jul 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 51 additions & 17 deletions libraries/password-manager/password-pass.lisp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,20 @@
(define-class password-store-interface (password-interface)
((executable (pathname->string (sera:resolve-executable "pass")))
(sleep-timer (or (uiop:getenv "PASSWORD_STORE_CLIP_TIME") 45))
(password-directory (or (uiop:getenv "PASSWORD_STORE_DIR")
(format nil "~a/.password-store" (uiop:getenv "HOME")))
:type string
:reader password-directory))
(password-directory
(or (uiop:getenv "PASSWORD_STORE_DIR")
(format nil "~a/.password-store" (uiop:getenv "HOME")))
:type string
:reader password-directory)
(scan-for-username-entries
t
:type boolean
:documentation "When non-nil, a two-step process determines the username:
1. Scan credential content for an entry with a key matching `*username-keys*'.
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."))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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:

  1. The filename & path relative to the root of the store
  2. 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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

@chaorace chaorace Jul 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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 😉

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to let the implementation handle :credential-content-and-path and :credential-path exclusively. Thanks!

(:export-class-name-p t)
(:export-accessor-names-p t))

Expand Down Expand Up @@ -69,22 +79,46 @@ 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)
Copy link
Member

Choose a reason for hiding this comment

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

Can we come up with a better name for the function and argument? It seems a bit misleading.

Copy link
Author

@chaorace chaorace Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@aadcg Here's some candidates, do any stand out to you?

  • username-from-filename
  • username-from-path
  • username-fallback

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

"Select a username using the path of the credential file.
The strategy for deriving the username is context-dependent:
- If credential exists in a subdirectory (e.g.: x.example/[email protected]),
username is taken as-is from the filename ([email protected])
- Otherwise, if no subdirectory is used (e.g.: [email protected]@y.example),
username is taken from the first half of the filename ([email protected])"
(multiple-value-bind (_ parent-dirs credential-name)
(uiop/pathname:split-unix-namestring-directory-components password-name)
(declare (ignore _))
(if parent-dirs
credential-name
(subseq credential-name 0 (position #\@ credential-name :from-end t)))))

(defun username-from-content (password-interface password-name)
"Select username from first entry in credential matching `*username-keys*'"
(let* ((content (execute password-interface (list "show" password-name)
:output '(:string :stripped t)))
(entries (parse-multiline content))
(username-entry (when entries
(some (lambda (key)
(find key entries :test #'string-equal :key #'first))
*username-keys*))))
(when username-entry (second username-entry))))

(defmethod clip-username ((password-interface password-store-interface)
&key password-name service)
"Save username of the `password-name' credential to clipboard.
See the `scan-for-usename-entries' slot for details.
Case is ignored.
The prefix is discarded from the result and returned."
The resulting username is also returned."
(declare (ignore service))
(when password-name
(let* ((content (execute password-interface (list "show" password-name)
:output '(:string :stripped t)))
(entries (parse-multiline content))
(username-entry (when entries
(some (lambda (key)
(find key entries :test #'string-equal :key #'first))
*username-keys*))))
(when username-entry
(trivial-clipboard:text (second username-entry))
(second username-entry)))))
(let ((username
(or (when (scan-for-username-entries password-interface)
(username-from-content password-interface password-name))
(username-from-name password-name))))
(when username
(trivial-clipboard:text username)
username))))

(defmethod save-password ((password-interface password-store-interface)
&key password-name username password service)
Expand Down
14 changes: 14 additions & 0 deletions source/changelog.lisp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,20 @@ elements are scaled accordingly.")
(:li "When on pre-release, push " (:code "X-pre-release")
" feature in addition to " (:code "X-pre-release-N") "one."))

(define-version "3.12.0"
(:nsection :title "Features"
(:ul
(:li "Added new filename-based fallback strategy to "
(:nxref :command 'nyxt/mode/password:copy-username)
" when using the default password-store password interface."
(:li "Allow skipping the primary key-based username strategy utilized by "
(:nxref :command 'nyxt/mode/password:copy-username)
" via the new slot "
(:nxref :class-name 'password:password-store-interface
:slot 'scan-for-username-entries)
". By default, it is set to "
(:code "t") " (do not skip).")))))

(define-version "3.11.7"
(:nsection :title "Bug fixes"
(:ul
Expand Down