user: simplify ssh key provisioning and drop unsafe usage #95
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jeremycline
force-pushed
the
jcline-drop-unsafe-ssh
branch
from
783afcd
to
ffe6d54
Compare
dongsupark
reviewed
Jul 3, 2024
| let new_uid = Uid::from_raw(uid); | ||
|
|
||
| let gid_groupname = CString::new(username)?; | ||
| let gid_group = unsafe { libc::getgrnam(gid_groupname.as_ptr()) }; |
There was a problem hiding this comment.
Removing the libc-related code like that, it becomes now unnecessary to keep libc any more.
Can you please remove libc in libazureinit/Cargo.toml?
To catch such issues, we should actually regularly run cargo machete or so. And there are other unused crates in this repo, but I will soon create another PR to remove those.
There was a problem hiding this comment.
Good catch, I've dropped it
In order to correctly set the ownership of the authorized_keys file, the function was using libc directly with some unsafe blocks. It's tricky to correctly use the C APIs and in this case, both getpwnam() and getgrnam() return NULL if a matching entry isn't found or if an error occurred. Since we weren't checking the return value this would result in a null pointer dereference. `nix` provides safe APIs to retrieve the user and group ids so rather than implementing it ourselves, just use those. This also refactors the two separate APIs for creating the directory and writing the keys to a single call that handles it all.
jeremycline
force-pushed
the
jcline-drop-unsafe-ssh
branch
from
ffe6d54
to
3bc3915
Compare
dongsupark
approved these changes
Jul 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In order to correctly set the ownership of the authorized_keys file, the function was using libc directly with some unsafe blocks. It's tricky to correctly use the C APIs and in this case, both getpwnam() and getgrnam() return NULL if a matching entry isn't found or if an error occurred. Since we weren't checking the return value this would result in a null pointer dereference.
nixprovides safe APIs to retrieve the user and group ids so rather than implementing it ourselves, just use those. This also refactors the two separate APIs for creating the directory and writing the keys to a single call that handles it all.This is extracted from #91 as it's not really related to the API design.