pijul_org / pijul

#339 Reset generated key files' permissions to 600 on Unix

Opened by mpevnev, on November 24, 2018
Closed
FlorentBecker added a patch:
Reset key files' permissions to 600 on Unix by mpevnev, created on November 24, 2018

885EePVNrfjZrKozEmf2pYsDyqiFKQ5yLk31rdetHft4VxgE6MGQaqRfJNiG8cEk8LnAx31DGYa6rWSSj6iYRFLp
latest
master
testing
mpevnev commented on November 24, 2018

It isn't hard to do this manually, but since Pijul allows per-repo keys, forgetting to do so is all too easy. Doesn't really matter for the public keys, but private keys with default 644 permissions are kind of useless.

I have opted to display a warning instead of panicking on error in the reset_permissions function, but I'm not sure if eprintln is the right tool for it. If Pijul uses something different for warnings, please let me know, I'll change it.

pmeunier commented on November 25, 2018

Thanks a lot for this patch, but I believe you could write it in a slightly more concise way. How about something like:

#[cfg(unix)]
fn set_key_permissions(f: &File) -> Result<(), Error> {
    use std::os::unix::fs::PermissionsExt;
    f.set_permissions(std::fs::Permissions::from_mode(0o600))?;
    Ok(())
}
#[cfg(not(unix))]
fn set_key_permissions(_: &File) -> Result<(), Error>  {
    Ok(())
}
mpevnev commented on November 25, 2018

I seriously need to read up on #[cfg]. I was not aware of not, I think I like the dummy version with it for non-Unixes (Unices?) better than conditional blocks in the body of generate_key.

I have one question about your version. My intent, originally, was to write a warning on an error instead of propagating it (and if I were a smarter person, I would have probably made reset_permissions not return anything in the first place. If you read it carefully, you'll notice that it can't returning anything but Ok(())). Do you find erroring out here acceptable? After all, a valid key file is already successfully generated if we got to the reset_permissions\set_key_permissions call. I'm kind of worried about weird filesystems that don't support file permissions. Erroring out would in their case mean never getting to generate the public key.

pmeunier commented on November 25, 2018

I think it's ok to issue a warning indeed.

FlorentBecker added a patch:
Do key files' permissions smarter by mpevnev, created on December 1, 2018

7Z4kLYUhXKTTyEzxyBzCZHcghGskqYgcthBQ1FP8EByjES7Zi1womQpQQT8KWGz9kJqJJmQNupC3XZCdEuDnVWdZ
latest
master
testing
mpevnev commented on December 1, 2018

Ok, here's a cleaner version. BTW, my concerns about 'weird file systems' have been proven empty: I've tried it on a FAT filesystem, and apparently set_permissions silently does nothing instead of returning an error if the filesystem doesn't support the operation. Weird design choice, honestly (EDIT: just checked, and it's call to chmod that returns 0 in such circumstances. In fact, the relevant manpage doesn't seem to indicate that the call should care about the capabilities of the filesystem, there's just no such error there). Should I go an extra mile and check if the permissions have actually changed, and then issue a warning if they didn't?

And please, if you have a quick\easy way to check this on a non-Unix platform, test that this doesn't produce compiler warnings, I'd hate to be a person to introduce them to the code base.

pmeunier commented on December 13, 2018

Thanks! I'll soon compile and test this on Windows (when preparing our next release)