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

Feature-parity with age: identity files via - #384

Closed
plaidfinch opened this issue May 10, 2023 · 7 comments
Closed

Feature-parity with age: identity files via - #384

plaidfinch opened this issue May 10, 2023 · 7 comments
Labels
impl divergence A divergence between this implementation and others

Comments

@plaidfinch
Copy link

plaidfinch commented May 10, 2023

I encountered a similar situation as in #379 when attempting to use the - placeholder for stdin and pbpaste (on macOS) to get my private key off the clipboard:

$ echo test | rage -r $MY_PUBLIC_KEY > test.txt
$ pbpaste | age -d -i - test.txt
test
$ pbpaste | rage -d -i - test.txt
Error: No such file or directory (os error 2)

[ Did rage not do what you expected? Could an error be more useful? ]
[ Tell us: https://str4d.xyz/rage/report                            ]

Taking the identity file on stdin is useful for me because it allows using a password manager to do custody of the age private key.

Doing a bit of digging, it looks like here is where there should be a little extra magic:

for filename in identity_strings {
// Try parsing as an encrypted age identity.
if let Ok(identity) = age::encrypted::Identity::from_buffer(
ArmoredReader::new(BufReader::new(File::open(&filename)?)),
Some(filename.clone()),
UiCallbacks,
max_work_factor,
) {
if let Some(identity) = identity {
recipients.extend(identity.recipients()?);
continue;
} else {
return Err(error::EncryptError::IdentityEncryptedWithoutPassphrase(
filename,
));
}
}
// Try parsing as a single multi-line SSH identity.
#[cfg(feature = "ssh")]
match age::ssh::Identity::from_buffer(
BufReader::new(File::open(&filename)?),
Some(filename.clone()),
) {
Ok(age::ssh::Identity::Unsupported(k)) => {
return Err(error::EncryptError::UnsupportedKey(filename, k))
}
Ok(identity) => {
if let Ok(recipient) = age::ssh::Recipient::try_from(identity) {
recipients.push(Box::new(recipient));
continue;
}
}
Err(_) => (),
}
// Try parsing as multiple single-line age identities.
let identity_file =
IdentityFile::from_file(filename.clone()).map_err(|e| match e.kind() {
io::ErrorKind::NotFound => error::EncryptError::IdentityNotFound(filename),
_ => e.into(),
})?;
for entry in identity_file.into_identities() {
match entry {
IdentityFileEntry::Native(i) => recipients.push(Box::new(i.to_public())),
IdentityFileEntry::Plugin(i) => plugin_identities.push(i),
}
}
}

If here we parse - specially to indicate that we should slurp standard in as a singular identity file, then the behavior will line up between age and rage.

The comparison point for how age handles this would be parseIdentitiesFile in the Go implementation.

Originally posted by @plaidfinch in #379 (comment)

@stevelr
Copy link

stevelr commented May 29, 2023

Named pipes have the same limitation as stdin: The function read_recipients assumes the identity path is a file that can be read multiple times. age reads the "file" once so it works with pipes and stdin. Update: perhaps this isn't a multi-read issue, it might be due to stdin or pipe not being closed. If that's the problem, then a fix may be either for the caller to close the input stream or pipe (preferable), or the reader (rage) to use nonblocking io.

age-keygen -o my-secret
echo hello | age -e -i my-secret > hello.age

# age
mkfifo fifo-1 && cat my-secret > fifo-1 &
cat hello.age | age -d -i fifo-1
# outputs "hello" as expected (and background task exits)

# rage
mkfifo fifo-2 && cat my-secret > fifo-2 &
cat hello.age | rage -d -i fifo-2 
# hangs (and background task exits)

Why pipes?
One advantage of pipes over stdin is that the pipe can be used in streaming use cases where stdin is the file being encrypted or decrypted. Like files, pipes appear in the file system, which creates a potential exposure where other processes might be able to read the secret, but it's arguably lower risk than files because it can be read only once.

I'd prefer to use an environment variable containing the identity key, rather than a pipe, but in the interests of maintaining compatibility with age, that would require changing the spec and both implementations. What do others think?

@stevelr
Copy link

stevelr commented May 29, 2023

@str4d Are you open to a PR that accepts environment variables with the prefix "OP_IDENTITY" (examples: OP_IDENTITY, OP_IDENTITY_001, OP_IDENTITY_ADMIN, ...), each containing the "contents" of an identity file /private key (either age-format or ssh) ? If that would be ok, I'll volunteer to create and submit it. Whether or not the pipes/stdin issue is considered a bug, I personally would prefer environment variables so keys don't need to be in the file system at all.

@plaidfinch that could address your use case, right?

🤔 It should be possible to unify the implementation with the code in wage where identities are passed to an inner function as an array of Read (ReadableStream in the web-sys case). That implementation could be called for any file-like input, including stdin/pipes, and could be called by rage cli, or WASI, that enumerates environment variables.

@plaidfinch
Copy link
Author

@stevelr this would address my use case, though I think support for stdin would still be a useful addition.

@stevelr
Copy link

stevelr commented May 30, 2023

Just noticed this comment that this there are plans for supporting this in the plugin api. I haven't been following that api development so I don't know whether the api is ready to write that plugin, or whether it's blocked waiting for a future capability of the plugin api. I didn't see a current plugin that implements environment variable support.

@str4d
Copy link
Owner

str4d commented Jun 12, 2023

@str4d Are you open to a PR that accepts environment variables with the prefix "OP_IDENTITY" (examples: OP_IDENTITY, OP_IDENTITY_001, OP_IDENTITY_ADMIN, ...), each containing the "contents" of an identity file /private key (either age-format or ssh) ?

No, that is not an approach I will merge into rage. If someone wants a CLI binary that supports that kind of API, they can create it using the age library themselves, or as a trivial script wrapping rage (once #379 is fixed), or via a plugin:

Just noticed this comment that this there are plans for supporting this in the plugin api. I haven't been following that api development so I don't know whether the api is ready to write that plugin, or whether it's blocked waiting for a future capability of the plugin api. I didn't see a current plugin that implements environment variable support.

Yes the plugin APIs should be sufficient to implement a plugin like this now. I am unaware of any such plugin yet, but environment variables given to rage are available to the plugins (they are started as child processes that inherit the parent's environment).

@str4d
Copy link
Owner

str4d commented Jun 12, 2023

Regarding this issue, I suspect that both it and #379 will be fixed once age is changed to not assume it can read the identity files more than once. My preferred approach for that is here: #354 (comment)

@str4d str4d added the impl divergence A divergence between this implementation and others label Aug 6, 2023
@str4d
Copy link
Owner

str4d commented Jan 16, 2024

Cleaning up the issue tracker; this is a duplicate of #177.

@str4d str4d closed this as completed Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impl divergence A divergence between this implementation and others
Projects
None yet
Development

No branches or pull requests

3 participants