Credentials storage XDG #109

xfbs · 2023-10-10T09:38:09Z

This PR switches the code used for reading and writing credentials to being XDG-compatible, but also backwards-compatible.

It does that by having a list of candidate paths for the configuration that it checks, preferring the XDG-compatible path but falling back to the previous path. When writing, it will only write to the XDG-compatible path.

This PR is still a draft.

This makes the intent of the function clear: when you read it being used, it is obvious that it might create a default credentials file rather than being a read-only load operation, without needing to read the documentation.

asmello · 2023-10-10T19:21:43Z

Overall comment: while resolving the appropriate config directory in a portable way is a great idea, we currently don't have support for actual configuration in Buffrs (see aws/aws-sdk#94 for a discussion on how that might look like). The credentials file is technically data, not configuration, so I think dirs::data_dir() would be a more appropriate choice. Though I believe the current approach mimics Cargo, so it'd be interesting to do some research into why they went that way rather than use the OS defaults.

From another side, I actually think that we should reconsider using keychains for credential management (this feature was removed in aws/aws-sdk#8). Even though stable Cargo uses a plaintext file, nightly has already implemented keychains as the default credential storage (rust-lang/cargo#8933), so there must be a way to make this work sensibly.

mara-schulke · 2023-10-10T21:47:17Z

@asmello i think, this could be sensible. But the host machine should be ultimately seen as secure to store a token. Thus I would not prioritize it. Maybe create a discussion?

asmello · 2023-10-11T08:02:18Z

But the host machine should be ultimately seen as secure to store a token

I disagree with this statement, any sensitive data should be encrypted at rest, that's just regular AppSec. Any program that does otherwise has weakness CWE-256 (or CWE-312) and cannot be considered secure by any official standard. But we don't need to block Patrick's contribution based on that (I just figured I'd point out that the file approach may be superseded in the future).

Created aws/aws-sdk#119

xfbs · 2023-10-11T09:02:06Z

The credentials file is technically data, not configuration, so I think dirs::data_dir() would be a more appropriate choice.

I don't know if I would agree with that! In my understanding, credentials is something that is explicitly configured by the user, and thus not data. In my understanding, data would be more static things that are needed at runtime. Anything that you need to back up when you migrate to a new machine and want to have your tools work should be configuration.

Though I believe the current approach mimics Cargo, so it'd be interesting to do some research into why they went that way rather than use the OS defaults.

There is a tracking issue on this. I think that the tl;dr is: XDG standard adoption is spotty, most UNIXy tools default to just putting things in dotfiles, and this works. I think that for new tools, the XDG paths should be respected, just like for new web development it should use ARIA labels to be accessible, but one should expect to see a lot of projects that do not prioritize it.

I actually think that we should reconsider using keychains for credential management.

This is not a terrible idea. Although, maybe it should be done in a separate step. To have it where we check:

System-local keychain (macOS keychain, GNOME keychain, whatever Windows does)
XDG-path ($XDG_CONFIG_DIR/buffrs/credentials.toml)
Dotfile location (~/.buffrs/credentials.toml)
System-wide config (/etc/buffrs/credentials.toml)

And then you can basically use whatever you want, but we obviously prefer the keychain approach for saving new credentials, if your system happens to have a standardized keychain store.

It might be a nightmare to support it however, if there is not a crate with a nice abstraction.

asmello · 2023-10-11T09:21:56Z

My main concern with using the config path, other than semantics, is security — this directory is often backed up and synced across devices, and implicitly sharing credentials this way is potentially not intended. A quick search revealed similar open issues on similar projects on github:

aws/aws-cli#9031
mislav/hub#1912

Of course, the XDG spec doesn't explicitly mention what category credentials fall into, because their recommendation is to use the keychain instead, but "user-specific data files" describes the credentials file better than "user-specific configuration files". The distinction being that configuration is applied to the tool itself, while data is merely manipulated by the tool (but doesn't affect its behaviour).

As for implementing keychain support at a later time, I'm ok with that, but I'll also flag that since we are pre-1.0 it doesn't necessarily make sense to continue supporting the current location ($HOME/.buffers) as well — this can be a breaking change.

asmello

Didn't comment on error messages as the way Buffrs handles that is due to change.

asmello · 2023-10-11T09:24:58Z

src/credentials.rs

+    /// XDG path: uses whichever config directory is appropriate for the platform.
+    ///
+    /// For example, on Linux this might be `~/.config/buffrs/credentials.toml`.
+    fn xdg_path() -> PathBuf {


I'd call this "config_path" or something similar. Knowledge of the XDG spec shouldn't be required to understand what this is about.

asmello · 2023-10-11T09:27:14Z

src/credentials.rs

-        fs::try_exists(Self::location()?)
-            .await
-            .wrap_err("Failed to detect credentials")
+        for location in &Self::possible_locations() {


I think this may be cleaner in functional style: Self::possible_locations().iter().map(...).all().

asmello · 2023-10-11T09:30:31Z

src/credentials.rs

-            .await
-            .wrap_err("Failed to detect credentials")
+        for location in &Self::possible_locations() {
+            if fs::try_exists(&location).await? {


Do we really want to terminate on the first error? Imagine we fail to access the system-wide credentials because we lack permission, shouldn't we keep going and try user locations too?

BTW fs::try_exists is only in nightly so I'm surprised this compiles in CI (probably something worth looking into, will file an issue for it).

This refers to tokio::fs::try_exists, which is probably not restricted to nightly?

Womp, you're right! This is why I don't like tokio shadowing std stuff... 😅 Ignore that part (and I'll close the issue).

asmello · 2023-10-11T09:30:46Z

src/credentials.rs

+            }
+        }
+
+        return Ok(false);


Flagged by clippy, this return is superfluous.

xfbs · 2023-10-11T09:42:52Z

Something you can look into @asmello: https://docs.rs/keyring/latest/keyring/. This crate purports to be a cross-platform keychain API, might do just what we need.

My main concern with using the config path, other than semantics, is security — this directory is often backed up and synced across devices, and implicitly sharing credentials this way is potentially not intended.

There are people doing strange stuff, but I think that generally, anything under $HOME is security sensitive. I have lots of secrets in ~/.config, ~/.ssh, ~/.mozilla, ~/.gpg, some of which are unencrypted (LUKS ftw). If people back this up and store it unencrypted or sync it willy-nilly, then that is potentially an error on their part rather than on ours.

The only scheme of syncing dotfiles that I know (and practise) is putting them into a git repo and cloning it on new machines. This is explicitly a one-way sync kind of thing. You have to explicitly add, commit and push things you want to sync, and never all of ~ or all of ~/.config. That would be a terrible idea, regardless of where we put our credentials!

asmello · 2023-10-11T10:10:53Z

There are people doing strange stuff, but I think that generally, anything under $HOME is security sensitive. I have lots of secrets in ~/.config, ~/.ssh, ~/.mozilla, ~/.gpg, some of which are unencrypted (LUKS ftw). If people back this up and store it unencrypted or sync it willy-nilly, then that is potentially an error on their part rather than on ours.

Let's make a distinction between dot-dirs like ~/.ssh and ~/.gpg, the entire $HOME directory and the XDG config path. We're discussing the last one, which by definition shouldn't contain any sensitive information. Arguably the data directory shouldn't either, because this wasn't explicitly intended as per the spec, although one can reasonably extend the definition of sensitive to cover all user data (as has been the trend with GDPR and other regulatory policies).

So to be clear:

Yes, we can consider $HOME, ~/.ssh, ~/.mozilla, ~/.gpg (and ~/.buffrs) all to be sensitive and expect users not to sync them across devices they deem insecure.
The same cannot be said about ~/.config, as it's supposed to only have configuration data.
- Although I agree a cautious user will still avoid syncing this directory since there's nothing enforcing that apps follow best practices.

BTW this is not just my opinion, the issues I linked (especially the AWS one) show that this is a common interpretation.

xfbs · 2023-10-24T08:59:35Z

Closing due to offline discussion, long-term buffrs will need to add a cache anyways and likely require a ~/.buffrs folder.

xfbs added 2 commits October 9, 2023 15:52

Rename Credentials::load() to load_or_init()

8730cfb

This makes the intent of the function clear: when you read it being used, it is obvious that it might create a default credentials file rather than being a read-only load operation, without needing to read the documentation.

Implements XDG config paths

88df4fc

xfbs self-assigned this Oct 10, 2023

mara-schulke added this to the Stabilization milestone Oct 10, 2023

mara-schulke requested review from mara-schulke and asmello October 10, 2023 09:44

mara-schulke changed the title ~~[DRAFT] Credentials storage XDG~~ Credentials storage XDG Oct 10, 2023

mara-schulke marked this pull request as draft October 10, 2023 09:44

mara-schulke added type::refactoring Changing the inner-workings of buffrs complexity::low Issues or ideas with a low implementation cost labels Oct 10, 2023

asmello reviewed Oct 11, 2023

View reviewed changes

asmello mentioned this pull request Oct 11, 2023

Use of nightly API doesn't fail CI checks #121

Closed

xfbs closed this Oct 24, 2023

mara-schulke deleted the pe/credentials branch November 3, 2023 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Credentials storage XDG #109

Credentials storage XDG #109

xfbs commented Oct 10, 2023

asmello commented Oct 10, 2023

mara-schulke commented Oct 10, 2023 •

edited

Loading

asmello commented Oct 11, 2023

xfbs commented Oct 11, 2023

asmello commented Oct 11, 2023

asmello left a comment

asmello Oct 11, 2023

asmello Oct 11, 2023

asmello Oct 11, 2023

xfbs Oct 11, 2023

asmello Oct 11, 2023

asmello Oct 11, 2023

xfbs commented Oct 11, 2023

asmello commented Oct 11, 2023

xfbs commented Oct 24, 2023

Credentials storage XDG #109

Credentials storage XDG #109

Conversation

xfbs commented Oct 10, 2023

asmello commented Oct 10, 2023

mara-schulke commented Oct 10, 2023 • edited Loading

asmello commented Oct 11, 2023

xfbs commented Oct 11, 2023

asmello commented Oct 11, 2023

asmello left a comment

Choose a reason for hiding this comment

asmello Oct 11, 2023

Choose a reason for hiding this comment

asmello Oct 11, 2023

Choose a reason for hiding this comment

asmello Oct 11, 2023

Choose a reason for hiding this comment

xfbs Oct 11, 2023

Choose a reason for hiding this comment

asmello Oct 11, 2023

Choose a reason for hiding this comment

asmello Oct 11, 2023

Choose a reason for hiding this comment

xfbs commented Oct 11, 2023

asmello commented Oct 11, 2023

xfbs commented Oct 24, 2023

mara-schulke commented Oct 10, 2023 •

edited

Loading