Enable cargo:libsecret on all Unix platforms except macOS and mobile#17197
Enable cargo:libsecret on all Unix platforms except macOS and mobile#17197quinnjr wants to merge 3 commits into
Conversation
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @weihanglo (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
…ernative Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
I don't know how to test this on BSD, and I have no machine running on BDS. Do you have any guide and step to follow so I can at least make sure it work?
BTW, this seems reasonable separately from the RFC.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks. I ends up adapting your scripts with qemu (which I used to be familiar with). And it worked!
|
@weihanglo would it be reasonable to add a CI check for BSD compatibility in this PR or leave it as something for later? |
That should be in a separate PR. It would be great if anyone has resources to help do that. BSD family platforms are mostly tier 2 with host tools or tier 3. We haven't done anything like that in Cargo's CI. Personally I'd like to see more CI validation for more platforms. Feel free to cut an issue first and contribute to that :) |
This comment has been minimized.
This comment has been minimized.
… macOS and mobile Widen the libsecret credential provider's cfg gate from `target_os = "linux"` to `all(unix, not(any(target_os = "macos", "ios", "tvos", "watchos", "visionos", "android")))`, making the provider available on the BSDs and other Unix-like systems. libsecret is loaded dynamically at runtime via libloading (which supports these platforms), and the library plus Secret Service implementations are packaged on FreeBSD, OpenBSD, and NetBSD, so the previous Linux-only gate was an artificial restriction rather than a technical one. macOS keeps its OS-specific keyring provider (cargo:macos-keychain), and the mobile Unix targets (iOS-family, Android) are excluded since libsecret does not exist there. Also document the provider's platform availability in registry-authentication.md and the crate README. This is pre-work for RFC 3981 (store registry tokens in the OS credential store by default), which flips secure token storage from opt-in to the default and notes the BSD gap in Cargo's built-in providers: rust-lang/rfcs#3981 Tested on a FreeBSD 15.1-RELEASE amd64 VirtualBox VM with libsecret and gnome-keyring over a D-Bus session bus: LibSecretCredential::new() loads libsecret via dlopen, and login/get/logout round-trips through the Secret Service correctly (token stored, retrieved matching, removed, and reported absent after logout). cargo check -p cargo and cargo fmt pass on x86_64-unknown-linux-gnu. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Weihang Lo <weihanglo@users.noreply.github.com>
…ADME Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
614843a to
bd36129
Compare
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@weihanglo Now that the branch is rebased on latest master and conflicts are resolved — do you need any help setting up or running the verification Gist I shared (https://gist.github.com/quinnjr/f9d0bea41b21c61dbed373911b93ba71)? It spins up a headless FreeBSD VM in VirtualBox and verifies the |
|
This changes the behavior for |
|
Reminder, once the PR becomes ready for a review, use |
What does this PR try to resolve?
The
cargo:libsecretbuilt-in credential provider is currently gated ontarget_os = "linux", with every other platform receiving anUnsupportedCredentialstub. That gate is an artificial restriction rather than a technical one: libsecret and Secret Service implementations (gnome-keyring, KWallet) are packaged on FreeBSD, OpenBSD, and NetBSD, the provider loadslibsecret-1.so.0dynamically at runtime vialibloading(which supports these platforms), and no code inside the provider is Linux-specific. As a result, BSD users have no built-in secure token storage option at all.This PR widens the cfg gate at all five sites (three in
cargo-credential-libsecret, two insrc/cargo/util/auth/mod.rs) to:macOS keeps its OS-specific
cargo:macos-keychainprovider, and the mobile Unix targets are excluded since libsecret does not exist there. Platform availability is now documented inregistry-authentication.mdand the crate README.This is pre-work for the in-progress RFC 3981: Store registry tokens in the OS credential store by default (rust-lang/rfcs#3981). That RFC proposes flipping secure token storage from opt-in to the default and calls out the BSD gap in Cargo's built-in providers as an adoption blocker; this change closes that gap independently of the RFC's outcome. Since
cargo:libsecretis never in the default provider list, nothing changes for users who haven't explicitly configured it.How to test and review this PR?
The diff is a mechanical cfg widening plus docs — the Linux behavior is unchanged (the new predicate is true exactly where the old one was, plus the newly enabled targets).
Tested on a FreeBSD 15.1-RELEASE amd64 VM with
libsecretandgnome-keyringover a D-Bus session bus, using a small harness that drives theCredentialtrait implementation directly:LibSecretCredential::new()loadslibsecret-1.so.0via dlopenLoginstores the token in the Secret ServiceGetretrieves a matching tokenLogoutremoves it, and a subsequentGetcorrectly returnsError::NotFoundOn Linux (x86_64-unknown-linux-gnu):
cargo check -p cargo,cargo check -p cargo-credential-libsecret, andcargo fmt --checkpass.One reviewer question worth settling: on the newly enabled platforms (as on Linux today), a libsecret load failure in
get_credential_libsecret()propagates out ofcredential_actionrather than falling through to the next provider in a configured list, unlike theUrlNotSupportedthe stub used to return atperform()time. This is pre-existing chain semantics extended to more platforms — happy to adjust if fall-through on construction failure is preferred.🤖 Generated with Claude Code