| Age | Commit message (Collapse) | Author |
|---|
|
- Add a buffered-reader-based function to trait Parse. This allows
us to manipulate the buffered reader stack before and after
parsing, e.g. to parse several armored objects in one stream. The
CertParser also does this, but uses internal interfaces for that.
|
|
- This is an internal interface that uses our reader stack's
cookie. We need this to traverse the buffered reader stack. We
did not, however, expose it as an external interface, because we
didn't want to bake in the cookie type into the API.
- Having a public API that operates on buffered readers is
convenient: the current Parser::from_reader operates on
io::Readers, and will most likely construct a
buffered_reader::Generic from it. This will eagerly buffer some
data, making this interface unsuitable if you want to read in one
artifact (e.g. an MPI) without consuming more data.
- Renaming the internal functions gives us a chance to add a more
general buffered reader interface.
|
|
- Previously, we did buffer the whole message, but the
implementation was done in a way that would have allowed a
constant-space operation with some more effort (maybe
considerable). However, the crypto-refresh abandons the idea of
doing one-pass processing of CSF messages on the basis that these
kind of messages are meant to be human-readable, and hence should
easily fit into memory. This means that we no longer need to
know the hash functions (and salt in case of v6 signatures) before
seeing the body, and indeed v6 CSF messages will no longer include
any Hash headers.
|
|
- Previously, we considered the line break immediately before the
signature marker to be part of the signature. This is not
correct. See Section 7.1 of RFC4880:
The line ending (i.e., the <CR><LF>) before the '-----BEGIN PGP
SIGNATURE-----' line that terminates the signed text is not
considered part of the signed text.
- This interpretation allows us to preserve the final newline in the
signed text. See
https://gitlab.com/sequoia-pgp/sequoia-sop/-/issues/16 where dkg
requests:
[...] if a trailing newline goes in, a trailing newline comes
out.
- The previous interpretation required very careful handling of the
trailing line break, making sure it is not hashed. This was
complicated by the fact that line breaks may use two characters,
and the two characters may straddle reads/writes. So, this change
of interpretation makes the code quite a bit simpler.
- To clarify, signing the four octet string "test" yields a message
looking like
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
test
-----BEGIN PGP SIGNATURE-----
...
-----END PGP SIGNATURE-----
Whereas signing the five octet string "test\n" now yields
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
test
-----BEGIN PGP SIGNATURE-----
...
-----END PGP SIGNATURE-----
- It is worth pointing out that GnuPG does something similar to what
we did before: it makes sure any signed text ends in a line
break (see `copy_clearsig_text`), but "swallows" this newline by
using it as delimiter between text and signature (see
`clearsign_file` where no explicit newline is emitted before the
armor filter is pushed and the signatures are emitted).
When verifying a message, GnuPG will emit the final line break,
being very careful not to hash it (see `handle_plaintext`).
The result is that any message signed by GnuPG seems to end in a
line break when verified by GnuPG, but will never end in a line
break when verified with Sequoia (or any other implementation that
considers the signature-separating line break to be not part of
the message, like OpenPGP.js). Signature validity is not
affected.
|
|
- Upgrade base64 to version 0.21.
|
|
- The packet parser hashes packet bodies to provide a robust
equality relation even when packet bodies are streamed. To hash
all bytes on the fly everywhere, we do that when it is consumed in
PacketParser::consume.
- This function assumes that if BufferedReader::data and friends
returned n bytes, future calls to these interfaces will succeed if
up to n bytes are requested, and no data was consumed in the
meantime.
- However, armor::Reader::data_helper did not provide that
guarantee, making PacketParser::consume panic with the message "It
is an error to consume more than data returns", which doesn't
quite correctly name the problem at hand.
- Fix this crash by fixing armor::Reader::data_helper in the same
way the previous commit fixes
buffered_reader::Generic::data_helper.
- Fixes #957.
|
|
|
|
- Fixes #3e188fb312ad4db1395f5e836bffaf2034b88a42.
|
|
- Change the default buffer size from 8 KB to 32 KB.
- Benchmarking using the chameleon reading a 23 MB cert-d with about
800 certificates, and verifying a signature over a short (2 KB)
message, showed that 32 KB is optimal. In particular, 16 KB and
64 KB buffer sizes were, respectively, 10% and 30% worse.
|
|
- When `buffered_reader::Generic::data_helper` is called and the
amount of data that is requested exceeds the amount of data that
is available, we read from the underlying reader.
- When determining how much to read from the underlying reader, we
took the maximum of the amount requested and the default buffer
size, and then subtracted the amount of data that is available.
- This means that when the amount requested is greater than the
buffer size, we would read exactly the amount requested. This is
problematic for two reasons. First, it is not unusual for a user
of a `BufferedReader` to not consume the data (e.g., a
`buffer_reader::Dup` never consumes data). In that case, once
calls to `BufferedReader::data` request more than the default
buffer size, the `BufferedReader` would forward any reads to the
underlying reader, and append the result to the available data to
create a single continuous `Vec<u8>`. Second, many of these reads
are for just one more byte than is available. The consequence is
that once the amount requested exceeds the amount available, many
subsequent reads would read from the underlying reader, and
`memcpy` the data held by the `BufferedReader`, which destroyed the
performance.
- Avoid most of the reads and the `memcpy`s by changing the behavior
of `buffered_reader::Generic::data_helper` as follows: if the
amount requested exceeds the amount available, try to read the
amount requested plus the buffer size minus what is available.
- Make the same change to `openpgp::armor::Reader`.
- Fixes #969.
Co-authored-by: Justus Winter <justus@sequoia-pgp.org>
|
|
- This is a verbatim copy, but unfortunately the copies diverged.
This commit aligns them again. Notably, this brings the following
fixes and improvements to armor::Reader:
- 7731320e: Once EOF is hit, don't poll reader again.
- 19a13f9b: Add tracing.
- a3c77e3f: Recycle buffers.
|
|
- The documentation got out of sync with ReaderMode.
- Describe various available modes in terms of ReaderMode.
- Fixes #847.
|
|
- Also warn about the potential deletion of this function in version
2.0.
|
|
|
|
|
|
- The CRC24 checksum is optional (and has been since at least
RFC2440, released in 1998), expensive to compute, doesn't add a
meaningful integrity check, and will be more strongly discouraged
in the next revision of OpenPGP.
- This changes our armor::Reader to not compute it in the first
place. This improves reading performance.
|
|
|
|
- Adapt to the new API:
- Gen is now a struct, not a Trait, and replaces StdThreadGen.
- The rand re-export has been removed. As a consequence, we need
our own function to generate an arbitrary value from a range.
|
|
- Found by clippy::redundant_slicing.
|
|
- In CamelCase, acronyms count as one word. Apply this rule where
API and lalrpop are not impacted.
- Found by clippy::upper_case_acronyms.
|
|
- Fixed with the help of clippy::needless_borrow.
|
|
|
|
|
|
|
|
Instead of:
if text.starts_with(prefix) {
&text[prefix.len()..]
} else {
&text
}
use this:
if let Some(rest) = text.strip_prefix(prefix) {
rest
} else {
&text
}
The strip_prefix is easier to understand, and also removes the
dependency between the if condition and the computation to get the
slice after the prefix. The dependency is reasonably clear, but the
compiler does not understand it so it's plausible that a future change
might change the condition but not the slice. The approach using
strip_prefix avoids that.
This was found by the clippy lint manual_strip:
https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip
|
|
A stable sort is one where values that compare equal are in the same
order relative to each other in the output as they were in the input.
Thus, if input has values a0, b, and a1, with a0==a1, a0<b, a1<b, then
the output of a stable sort is guaranteed to be be [a0, a1, b]. For an
unstable sort, the output may also be [a1, a0, b]. This can matter if,
for example, the values are structs and only one field is used for
comparison.
On data where an unstable sort (.sort_unstable) works, it's bad to use
a stable sort (.sort), because it uses more CPU without affecting the
end result.
If the types of values in a vector are primitive types, as they are in
the cases changed by this commit, an unstable sort will do just fine.
Found by clippy lint stable_sort_primitive:
https://rust-lang.github.io/rust-clippy/master/index.html#stable_sort_primitive
|
|
Found by clippy lint nonminimal_bool:
https://rust-lang.github.io/rust-clippy/master/index.html#nonminimal_bool
|
|
The extra & in a pattern (match arm or if let) is unnecessary and only
makes the code harder to read. In most places it's enough to just
remove the & from the pattern, but in a few places a dereference (*)
needs to be added where the value captured in the pattern is used, as
removing the & changes the type of the captured value to be a
reference.
Overall, the changes are almost mechanical. Although the diff is huge,
it should be easy to read.
The clippy lint match_ref_pats warns about this. See:
https://rust-lang.github.io/rust-clippy/master/index.html#match_ref_pats
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#single_char_pattern
- https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#single_component_path_imports
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#len_zero
- https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#needless_return
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#match_like_matches_macro
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#char_lit_as_u8
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#useless_conversion
|
|
- https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call
|
|
- Implement verification of messages using the Cleartext Signature
Framework by detecting them in the armor reader, and transforming
them on the fly to inline signed messages.
- The transformation is not perfect. We need to synthesize
one-pass-signatures, but we only know the hash algorithm(s) used.
Luckily, this is the only information the packet parser needs.
- We only enable the transformation when using stream::Verifier.
The transformation is transparent to the caller. Currently, there
is no way to disable this. In the next major revision, we may add
ways to control this behavior.
- Fixes #151.
|
|
- Previously, armor::Reader implemented BufferedReader using the
Generic reader on top of IoReader's io::Read implementation.
However, that is no longer good enough, because we need to access
the cookie from (Io)Reader::initialize.
- The real fix is to directly implement the BufferedReader protocol.
That would have been the right thing to do from the beginning,
instead of using buffered_reader::Generic. This may actually
simplify the code and reduce buffering. However, implementing the
BufferedReader protocol is a bit error-prone, so we defer it once
again!
- Instead, manually inline the code from the Generic reader.
- In the following commits, we will take advantage of that and
access the cookie.
|
|
- In the next commit, we will inline buffered_reader::Generic, which
also hash a field called 'buffer'. To avoid changing any code
copied from the generic reader, rename this field first.
|
|
- Anything beyond 24 bits is masked off anyway, so this doesn't
change the result of the checksum.
|
|
|
|
|
|
|
|
|
|
|
|
- Avoid the additional `fn f()`.
|
|
- See #480.
|
|
- See #615.
|
|
- This way the entire `BufferedReader<C>` will be `Send` and `Sync`.
- Modify all other crates accordingly.
- See #615.
|
|
- Declare trait bounds using a where clause. It looks a bit odd if
there is no bound, but not worse than before.
|
