feat: add dsa support #276

dignifiedquire · 2024-01-27T13:47:01Z

Based on #124

Depends on RustCrypto/signatures#798

TODOs

Implement verification
Update to released version of feat(dsa): implement `SigningKey::sign_prehashed_rfc6979 RustCrypto/signatures#798
Implement key creation
Update key tests

src/composed/key/builder.rs

hko-s · 2024-02-06T21:36:00Z

tests/key_test.rs

-    (test_parse_dumps_7, 7, 18_133, 21_000),
-    (test_parse_dumps_8, 8, 18_139, 21_000),
-    (test_parse_dumps_9, 9, 17_992, 21_000),
+    (test_parse_dumps_0, 0, 17_697, 20_998),


I am surprised to see these numbers go down by so much.

If I understand the meaning of this diff correctly, then: by implementing DSA in this PR, around 500 public keys per ~20k block of SKS data can now not be used anymore. Presumably because the signature verification fails?

Did you look into what's going on with those keys?

so before we completely ignored dsa keys, so this number was expected to go down, I haven’t had time to dig into the failures in detail though

Right, I agree with the expectations that the number would go down somewhat, but 2.5% of all SKS keys being seemingly broken now doesn't feel right. I wonder what percentage of all DSA keys in the test data is broken.

Maybe this quantitative regression indicates that some historical implementations of PGP encoded DSA key material in a way that is not currently handled correctly?

that is very much possible, we should investigate this, but I didn't want to block getting the basic support in by this

makes sense. opening a ticket to investigate this some other time might be nice as a reminder.

hko-s · 2024-02-06T23:10:40Z

The interop test suite didn't yield any interesting results for this PR, so I decided to test data signatures manually, using a few SOP implementations.

This comment documents my testing procedure. I'm attaching the test artifacts.

Generate test key

As setup, we generate a dsa3072 key with rpgp:

$ rsop generate-key --profile dsa alice@example.org > key.dsa

Generate data signature with rsop (rpgp), verify with different implementations

We then produce a data signature with rsop (rpgp):

$ echo -n "hello world" | rsop sign key.dsa > sig.rpgp

... and verify it with other SOP implementations:

$ echo -n "hello world" | rsop verify sig.rpgp key.dsa
2024-02-06T22:39:11Z 13375374f22eb5a0563137673cb7f9b7a8bb79a2 13375374f22eb5a0563137673cb7f9b7a8bb79a2 mode:binary

$ echo -n "hello world" | sqop verify sig.rpgp key.dsa
2024-02-06T22:39:11Z 13375374F22EB5A0563137673CB7F9B7A8BB79A2 13375374F22EB5A0563137673CB7F9B7A8BB79A2 mode:binary

$ echo -n "hello world" | pgpainless-cli verify sig.rpgp key.dsa
2024-02-06T22:39:11Z 13375374F22EB5A0563137673CB7F9B7A8BB79A2 13375374F22EB5A0563137673CB7F9B7A8BB79A2 mode:binary

$ echo -n "hello world" | ~/go/bin/gosop verify sig.rpgp key.dsa
2024-02-06T22:39:11Z 13375374F22EB5A0563137673CB7F9B7A8BB79A2 13375374F22EB5A0563137673CB7F9B7A8BB79A2

Generate data signature with sqop (sequoia), verify with different implementations

For comparison, we create a signature with sqop (sequoia), using the same private key (generated by rpgp):

$ echo -n "hello world" | sqop sign key.dsa > sig.seq

Verifying this signature works with all SOP implementations used above, including rsop and gosop.

$ echo -n "hello world" | rsop verify sig.seq key.dsa
2024-02-06T22:40:32Z 13375374f22eb5a0563137673cb7f9b7a8bb79a2 13375374f22eb5a0563137673cb7f9b7a8bb79a2 mode:binary

$ echo -n "hello world" | ~/go/bin/gosop verify sig.seq key.dsa
2024-02-06T22:40:32Z 13375374F22EB5A0563137673CB7F9B7A8BB79A2 13375374F22EB5A0563137673CB7F9B7A8BB79A2

sig.seq.txt
key.dsa.txt
sig.rpgp.txt

EDIT: a previous version of this comment showed a failure with gosop that I mistook for a signature verification problem.
However, that failure was a bug in the gosop CLI tool itself: gosop crashes for filenames that are too short (my testing signature file was initially called just sig, which caused a gosop crash; also see ProtonMail/gosop#22)

src/crypto/dsa.rs

hko-s · 2024-02-07T07:33:18Z

I've concluded my review. Besides being surprised by the numbers in the test_parse_dumps tests, nothing stood out to me.

This handles no secret data and I'm reasonably confident in the implementation, but I'm not sure what the standard is for accepting new cryptographic implementations. This is actually already covered by the test suite, the DSA signatures were just being ignored previously.

Old version would fail for signatures where the digest's most significant bit happened to be zero. By chance, this was not the case for all three DSA signatures in the test suite.

This also adds more tests, using the test vectors from RFC6979. This adds a dependency on the hmac-drbg crate. The ecdsa crate also needs this functionality, and in a comment they mention wanting to switch to this crate after a pull request (which has already been merged) is merged. The current implementation in the ecdsa crate is not usable, since it assumes the hash output size will always be larger than the desired output size, and this is not the case for small hash functions with big DSA keys. The hmac-drbg crate does implement this correctly. Besides that the implementations seem interchangeable.

This adds just two tests from the test vectors because generating these parameters is a slow operation. Also a couple of really small tweaks to dsa.rs. Sorry.

Replace byte vectors and `Mpi` values with `BigUint`.

Wow.

Replace with a copy/paste from the ecdsa crate until that code can be split out into a separate crate.

depends on RustCrypto/signatures#798

needed because of dependencies

Co-authored-by: Heiko Schaefer <59601023+hko-s@users.noreply.github.com>

dignifiedquire force-pushed the feat-dsa branch from b73f165 to f8807a6 Compare January 27, 2024 16:14

dignifiedquire mentioned this pull request Jan 27, 2024

Add DSA verify implementation #124

Closed

dignifiedquire force-pushed the feat-dsa branch from cdcfb48 to c969839 Compare February 6, 2024 19:35

dignifiedquire marked this pull request as ready for review February 6, 2024 19:35

hko-s reviewed Feb 6, 2024

View reviewed changes

hko-s reviewed Feb 7, 2024

View reviewed changes

src/crypto/dsa.rs Outdated Show resolved Hide resolved

rvolgers and others added 19 commits February 7, 2024 13:01

Fix DSA signature verification

6e81f4c

Old version would fail for signatures where the digest's most significant bit happened to be zero. By chance, this was not the case for all three DSA signatures in the test suite.

Fix check for dsa sig values, and use Zero trait

cf53555

Implement DSA p, q parameter generation

47f5051

This adds just two tests from the test vectors because generating these parameters is a slow operation. Also a couple of really small tweaks to dsa.rs. Sorry.

Add some TODO comments and a reference link

d28248e

Implement key generation and use rng api

c3a63e4

Only use BigUint in external API

d5c6ebd

Replace byte vectors and `Mpi` values with `BigUint`.

Resolve question about leading zero bytes

6336874

Wow.

Remove external hmac-drbg dependency

665b6bf

Replace with a copy/paste from the ecdsa crate until that code can be split out into a separate crate.

switch to dsa crate

c6f1041

feed correct digest algorithm to dsa signer

0903ec0

depends on RustCrypto/signatures#798

update test cases and status

676d813

implement DSA key generation

82915c5

update to released version of dsa

6c468cc

feat: bump MSRV to 1.69

6ccc27d

needed because of dependencies

Update src/crypto/dsa.rs

f8adf92

Co-authored-by: Heiko Schaefer <59601023+hko-s@users.noreply.github.com>

Update src/composed/key/builder.rs

386560c

Co-authored-by: Heiko Schaefer <59601023+hko-s@users.noreply.github.com>

Update src/composed/key/builder.rs

f6ca4c4

Co-authored-by: Heiko Schaefer <59601023+hko-s@users.noreply.github.com>

dignifiedquire force-pushed the feat-dsa branch from 9d266a1 to f6ca4c4 Compare February 7, 2024 12:01

fixups

f681811

dignifiedquire mentioned this pull request Feb 8, 2024

investigate DSA key failures in the dumps #284

Open

dignifiedquire merged commit 4ed459b into master Feb 8, 2024
18 checks passed

dignifiedquire deleted the feat-dsa branch February 8, 2024 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add dsa support #276

feat: add dsa support #276

dignifiedquire commented Jan 27, 2024 •

edited

Loading

hko-s Feb 6, 2024

dignifiedquire Feb 7, 2024

hko-s Feb 7, 2024

dignifiedquire Feb 7, 2024

hko-s Feb 7, 2024

dignifiedquire Feb 8, 2024

hko-s commented Feb 6, 2024 •

edited

Loading

hko-s commented Feb 7, 2024

feat: add dsa support #276

feat: add dsa support #276

Conversation

dignifiedquire commented Jan 27, 2024 • edited Loading

hko-s Feb 6, 2024

Choose a reason for hiding this comment

dignifiedquire Feb 7, 2024

Choose a reason for hiding this comment

hko-s Feb 7, 2024

Choose a reason for hiding this comment

dignifiedquire Feb 7, 2024

Choose a reason for hiding this comment

hko-s Feb 7, 2024

Choose a reason for hiding this comment

dignifiedquire Feb 8, 2024

Choose a reason for hiding this comment

hko-s commented Feb 6, 2024 • edited Loading

Generate test key

Generate data signature with rsop (rpgp), verify with different implementations

Generate data signature with sqop (sequoia), verify with different implementations

hko-s commented Feb 7, 2024

dignifiedquire commented Jan 27, 2024 •

edited

Loading

hko-s commented Feb 6, 2024 •

edited

Loading