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.

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

Add credential loader helper #98

Closed
wants to merge 1 commit into from
Closed

Add credential loader helper #98

wants to merge 1 commit into from

Conversation

vilgotf
Copy link
Contributor

@vilgotf vilgotf commented Dec 11, 2021

Not a part of the C libsystemd library but I don't know if being strictly 1-1 is a goal.

Although the code is simple I do feel that it'd be a waste for every application to reimplement it.

@lucab
Copy link
Owner

lucab commented Dec 12, 2021

Strict 1:1 is indeed not a goal, if an helper makes sense to have I'll gladly take it :)
Thanks for the PR, I'll see the patch is small so I'll try to review it next week.
But it is the first time I encounter this systemd feature, so I first need to get myself familiar with that (which may take a bit longer).

@swsnr
Copy link
Collaborator

swsnr commented Dec 12, 2021

Essentially, this is std::fs::read(std::env::var_os("CREDENTIALS_DIRECTORY")?.into().join(id))? isn't it? I'm not sure whether this is worth abstracting over to be honest…

@vilgotf
Copy link
Contributor Author

vilgotf commented Dec 12, 2021

Currently it only abstracts away CREDENTIALS_DIRECTORY and provides an iterator.

It could be extended by get() returning Option<Vec<u8>> since the only possible IO error should be NotFound (panicking or discarding other errors).

/// }
/// }
/// ```
pub fn get<K: AsRef<OsStr>>(&self, id: K) -> io::Result<Vec<u8>> {
Copy link
Owner

Choose a reason for hiding this comment

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

I'd be happier if we return a std::fs::File instead of a vector of bytes.
The rationale is that it will allow the caller to read the content at a later time, not having to keep the secret in memory since the beginning.

self._get(id.as_ref())
}

fn _get(&self, id: &OsStr) -> io::Result<Vec<u8>> {
Copy link
Owner

Choose a reason for hiding this comment

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

Please note that the systemd docs says ASCII string suitable as filename in the filesystem.
Thus I think we should have some basic sanity check to detect /, empty string, null bytes and such.
This is to avoid ending up with ../../../../etc/passwd or similar.

/// }
/// # Ok::<(), std::io::Error>(())
pub fn iter(&self) -> fs::ReadDir {
self.dir.read_dir().expect("path exists")
Copy link
Owner

Choose a reason for hiding this comment

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

If you need this kind of listing primitive, then let's store a https://docs.rs/nix/latest/nix/dir/struct.Dir.html internally and directly wire our method to Dir::iter().


impl CredentialLoader {
/// Attempt to initiate a loader, returning [`None`] if no credentials are available.
pub fn new() -> Option<Self> {
Copy link
Owner

Choose a reason for hiding this comment

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

This should probably be called open() and return a Result<Self> instead.

To check whether the credentials store is available or not, let's add a dedicated CredentialLoader::credentials_directory() -> Option<PathBuf> on the side.

@@ -23,6 +23,7 @@

/// Interfaces for socket-activated services.
pub mod activation;
mod credential;
Copy link
Owner

Choose a reason for hiding this comment

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

Let's call this credentials (plural) and do a pub mod credentials here.

@lucab
Copy link
Owner

lucab commented Feb 28, 2022

@vilgotf are you still pursuing this? If you have other things to handle at the moment, I'd be happy to push some changes on top of your branch and land it.

@lucab lucab reopened this Jun 20, 2022
@lucab
Copy link
Owner

lucab commented Jun 20, 2022

(I'm not sure how this PR got closed, but even if it got stale by now I'll be happy to throw some changes on top and eventually merge it)

@lucab
Copy link
Owner

lucab commented Oct 21, 2022

Ah, the repository behind this PR does not exist anymore so I can't push there to update the PR.
I'll close this one and recreate it as a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants