Remove split between recipient and passphrase encryption #507
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #507 +/- ##
==========================================
- Coverage 52.01% 51.72% -0.30%
==========================================
Files 43 42 -1
Lines 3937 3938 +1
==========================================
- Hits 2048 2037 -11
- Misses 1889 1901 +12 ☔ View full report in Codecov by Sentry. |
| } else { | ||
| Err(DecryptError::InvalidHeader) | ||
| } | ||
| } | ||
|
|
||
| /// Returns `true` if the age file is encrypted to a passphrase. | ||
| pub fn is_scrypt(&self) -> bool { |
There was a problem hiding this comment.
This is the only API that I'm not entirely sure about exposing in the public API.
I'm defining / checking structural validity of the header (if an scrypt stanza is present, it must be the only stanza) in once place (HeaderV1), and enforcing it in two places:
- in the
Encryptorvia theHeaderV1::newconstructor; - in the
Decryptorconstructors viaDecryptor::from_v1_header.
During encryption, we still have the passphrase-specific helper, but it is effectively a thin wrapper around the Recipient API, and we now throw an error if the caller mixes things up (vs being infallible previously).
This method exposes which of the two states we are in during decryption (replacing what I previously exposed with an enum), so that we can report "mixed identity and passphrase" before asking the user for a passphrase.
@FiloSottile AFAICT you don't expose a similar API, and instead check for scrypt stanza presence or absence in multiple places:
- Using a fake identity to reject
scryptstanzas: https://github.com/FiloSottile/age/blob/faefdc3c81efa89fd41998b77c62234ca56be10b/cmd/age/age.go#L423-L432 - Using a lazy passphrase querier that internally checks the structural validity before asking the user for a passphrase:
AFAICT these are functionally equivalent to my approach: given that anyone can define an Identity and place it first, they can use that to extract stanza types. So my API isn't exposing more power than is already exposed. But does it still make sense to special-case scrypt in this way?
I might also just leave this API in the public API until I implement labels (#403), and re-evaluate then?
There was a problem hiding this comment.
I think the main difference is that the cmd/age approach is a CLI special case, not a library special case.
rejectScryptIdentity and LazyScryptIdentity are CLI UX affordances: the first to print a helpful error, the second to avoid asking for a passphrase if we are going to error out sooner. Neither are concerned with scrypt stanzas being the only one in the file.
The structural check is built into the scrypt Identity implementation on the decryption side: it just receives all the stanzas, and errors out if it sees multiple. Note that this is not security load-bearing, since an attacker that was a co-recipient can strip the other stanzas.
On the encryption side, I used to have a special case to prevent library users from mixing scrypt with other stanzas, but that is now replaced with labels.
There was a problem hiding this comment.
The structural check is built into the scrypt Identity implementation on the decryption side: it just receives all the stanzas, and errors out if it sees multiple.
Aha, this is where we differ. My Identity trait does have an Identity::unwrap_stanzas method, but it was added as a performance optimization in #129 for identities that can benefit from pre-computations (such as plugin identities, where you want to provide all stanzas to the same plugin invocation - this is specifically what I added it for). The default implementation of it just passes each stanza individually to Identity::unwrap_stanza and returns the file key from the first stanza that succeeds.
Identity was the first trait I added (in #117), which I introduced as part of building the plugin functionality. I suspect that I limited it to one stanza at a time in order to limit how much logic would be implemented behind it (leaving most of the restrictions implemented in the header logic, like where I currently implement the scrypt restriction for decryption). However, given that multi-stanza unwrapping was later added specifically for plugins, that on its own isn't a strong reason to keep it that way
I think I also see the benefit of Recipient and Identity being symmetric. Originally Recipient only returned a single stanza (#119) and thus was symmetric with Identity, but that changed in #129 as well to enable encrypt-time coalescing by allowing a Recipient impl to represent multiple recipients (and thus generate multiple stanzas).
So I think what I will do after this PR is refactor Identity so that its trait is documented as always receiving all stanzas associated with an individual file.
Note that this is not security load-bearing, since an attacker that was a co-recipient can strip the other stanzas.
Yep, which is partly why I don't implement that restriction in my scrypt impl, and instead implement it in the header parser.
str4d
force-pushed
the
504-the-day-of-reckoning
branch
from
8e77ec1
to
f69c29b
Compare
Closes #504.