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

Fix #449 (ECDSA forcing DER/ASN.1) #450

Merged
merged 10 commits into from
Mar 2, 2025
Merged

Fix #449 (ECDSA forcing DER/ASN.1) #450

merged 10 commits into from
Mar 2, 2025

Conversation

karel-m
Copy link
Member

@karel-m karel-m commented Oct 14, 2018

Fix ECDSA forcing DER.

TODO:

  • update doc
  • add a test case with eth27 signature format
  • refactor ecc_recover_key()

This fixes #449

Copy link
Member

@sjaeckel sjaeckel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karel-m karel-m force-pushed the pr/ecdsa-der-attempt1 branch from 8e2b645 to 5613bae Compare April 10, 2019 16:10
@sjaeckel
Copy link
Member

so we wait now until the two missing points are done?

@karel-m
Copy link
Member Author

karel-m commented Apr 10, 2019

If you agree with the proposed split, which I assume you do, I (or any other volunteer) have to update doc and tests.

@sjaeckel
Copy link
Member

👍

@karel-m karel-m force-pushed the pr/ecdsa-der-attempt1 branch 2 times, most recently from 797f10c to 9340721 Compare June 12, 2019 08:58
@sjaeckel sjaeckel force-pushed the pr/ecdsa-der-attempt1 branch from 9340721 to d1ac928 Compare January 22, 2020 09:48
@sjaeckel sjaeckel added this to the next milestone Oct 26, 2020
@karel-m karel-m force-pushed the pr/ecdsa-der-attempt1 branch from d1ac928 to d708fe7 Compare April 11, 2021 14:39
@sjaeckel sjaeckel force-pushed the pr/ecdsa-der-attempt1 branch from f4d044e to a616dbb Compare December 28, 2024 19:53
@sjaeckel sjaeckel requested a review from levitte January 6, 2025 18:16
Copy link
Collaborator

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good so far. Do you need help with the last bits (docs / test)?

@sjaeckel sjaeckel force-pushed the pr/ecdsa-der-attempt1 branch from a616dbb to 46bf532 Compare January 9, 2025 12:38
@sjaeckel
Copy link
Member

sjaeckel commented Jan 9, 2025

Do you need help with the last bits (docs / test)?

Sure! Thanks :)

@sjaeckel sjaeckel force-pushed the pr/ecdsa-der-attempt1 branch from 46bf532 to a611b27 Compare February 24, 2025 16:26
@levitte
Copy link
Collaborator

levitte commented Feb 24, 2025

Do you need help with the last bits (docs / test)?

Sure! Thanks :)

Sorry that I never came back on this... [sigh] ambition doesn't always align with time

@sjaeckel
Copy link
Member

sjaeckel commented Feb 25, 2025

Thanks for the review!

I actually found the column of parameters easier to read

Yeah, I was lazy and c&p'ed from the sources, so it's the same style.

I'll have another look.

the italics style was meant to align with man-pages(7)

Aahh alright, that makes sense.

I believe that monospace is better suited for that purpose to highlight code/variable names (and that's also what is used in most modern formatting styles). Additionally this allows to use italics to reference to proper nouns/standards/etc.

should there be a future PR with s/textit/texttt/ applied

Absolutely - for all "code-related" terms.

@levitte
Copy link
Collaborator

levitte commented Feb 25, 2025

So, just for kicks, I tried a change from \textit to \texttt, and well, I dunno... typographically, it's a question what's more distinct in a text flow. Compare these two images:

Monotone:
monotone


Italics:
italic

@sjaeckel
Copy link
Member

sjaeckel commented Feb 25, 2025

I liked the idea, but I found it hard to decide on two completely different parts of the text, so I took the same text and applied the two different styles.

Monospace vs. Italics vs. Monospace-Italics

monospace

italics

monospace-italic

... and I agree that the text flow is different in the monospace version. It somehow feels kind of "better readable" with italics, but don't ask me why, I have the impression the meaning is clearer in the monospace version.

[Edit] and Monospace-Italics kind of combines both. It's a bit strange at first but it's very well readable and stands out from the regular text.

What should we do? 🤷

I'm slightly strongly leaning towards monospace monospace-italics ...

[Edit 2] FTR: I created the third with the following addition on the top

diff --git a/doc/crypt.tex b/doc/crypt.tex
index 81995116..4b4274c8 100644
--- a/doc/crypt.tex
+++ b/doc/crypt.tex
@@ -1,2 +1,3 @@
 \documentclass[synpaper]{book}
+\usepackage[T1]{fontenc}
 \usepackage{geometry}
@@ -46,2 +47,3 @@
 \definecolor{DGray}{gray}{0.5}
+\newcommand{\code}[1]{\texttt{\textit{#1}}}
 \newcommand{\emailaddr}[1]{\mbox{$<${#1}$>$}}

The first change I'd keep for sure, since otherwise one cant copy&paste from the PDF.

The second change declares the \code{} macro to format as monospace-italics.

@levitte
Copy link
Collaborator

levitte commented Feb 26, 2025

I think we can agree on monospace-italics.

The \code{} macro should be added either way, so whatever we end up choosing (or changing again in the future, 'cause one never knows), it makes for consistency, makes life easier, so just add it 😉

@sjaeckel sjaeckel force-pushed the pr/ecdsa-der-attempt1 branch 2 times, most recently from 31d4aaf to 5e2dade Compare February 26, 2025 17:00
@sjaeckel sjaeckel force-pushed the pr/ecdsa-der-attempt1 branch from 5e2dade to e6649b0 Compare February 27, 2025 10:41
@sjaeckel sjaeckel requested a review from levitte February 27, 2025 12:34
Copy link
Collaborator

@levitte levitte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a visual check, looks great :-)

karel-m and others added 10 commits March 2, 2025 12:09
via e.g.

```
make -j9 EXTRALIBS="../libtommath/libtommath.a " \
	CFLAGS="-DLTC_NOTHING -DLTC_MINIMAL -DLTC_MECC -DUSE_LTM \
	-DLTM_DESC -I../libtommath"
```
* make PDF content copy&paste-able
* add new `\code{}` command

Signed-off-by: Steffen Jaeckel <[email protected]>
Signed-off-by: Steffen Jaeckel <[email protected]>
It is already nearly independent of `LTC_DER`, so simply `#ifdef` that
code path instead of multiplying the APIs by the number of signature
formats.

Signed-off-by: Steffen Jaeckel <[email protected]>
This also adds a note about the potential limitation of the signature
formats, depending on the tailoring.

Signed-off-by: Steffen Jaeckel <[email protected]>
@sjaeckel sjaeckel force-pushed the pr/ecdsa-der-attempt1 branch from e6649b0 to 46f33c6 Compare March 2, 2025 11:09
@sjaeckel sjaeckel changed the title An attempt to fix #449 (ECDSA forcing DER/ASN.1) Fix #449 (ECDSA forcing DER/ASN.1) Mar 2, 2025
@sjaeckel sjaeckel merged commit a6b9aff into develop Mar 2, 2025
78 checks passed
@sjaeckel sjaeckel deleted the pr/ecdsa-der-attempt1 branch March 2, 2025 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ecc_sign_hash_ex + ecc_verify_hash_ex forces linking DER/ASN.1 code
3 participants