search

romanz / trezor-agent

Hardware-based SSH/GPG/age agent
GNU Lesser General Public License v3.0
571 stars 150 forks source link

Is trezor-agent --help correct? #431

Open doolio opened 1 year ago

doolio commented 1 year ago

trezor-agent --help states the following:

  -e CURVE, --ecdsa-curve-name CURVE
                        specify ECDSA curve name: ed25519, nist256p1

In trying to educate myself more in these topics I came across this security.stackexchange question where the top answer states that:

"If you want a signature algorithm based on elliptic curves, then that's ECDSA or Ed25519; for some technical reasons due to the precise definition of the curve equation, that's ECDSA for P-256, Ed25519 for Curve25519."

which seems to contradict the --help output.

romanz commented 1 year ago

You're right - good catch!

doolio commented 1 year ago

OK if that is the case then should we reconsider the naming in other places to be more accurate and prevent confusion. For example here:

https://github.com/romanz/trezor-agent/blob/3c911e99a0394278104564092225d67c75e74b99/libagent/formats.py#L1

Firstly, the doctsring for this module suggests it is only for SSH but is that really the case. Isn't this a module in the generic library? GPG is mentioned in some parts.

https://github.com/romanz/trezor-agent/blob/3c911e99a0394278104564092225d67c75e74b99/libagent/formats.py#L14-L21

Are L14-L17 necessary if L19-21 exist? Although the constants in L20-L21 seem to be only used in this module. If L19 included SSH I think it would be sufficient and accurate. I've seen the curve name given as nistp256 in some places and nist256p1 in other places and not just in this repo. It is confusing. If they are equivalent labels should one be chosen and used throughout to avoid confusion. I would lean towards nistp256 as you often see it labelled as NIST P-256 online. The p1 suggests there could p2, p3 etc. Perhaps there are and if so maybe nist256p1 is what should be used throughout.

https://github.com/romanz/trezor-agent/blob/3c911e99a0394278104564092225d67c75e74b99/libagent/formats.py#L23-L33

I think SSH_NIST256_ should be renamed to SSH_ECDSA_ since that is the signing algorithm? These key types include the curve name (i.e. nistp256) as well unlike the ed25519 key types which do not include the curve name (i.e. curve25519). Also, I have read the sk variants are for hardware tokens so should they be used instead?

https://github.com/romanz/trezor-agent/blob/3c911e99a0394278104564092225d67c75e74b99/libagent/formats.py#L66-L68 https://github.com/romanz/trezor-agent/blob/3c911e99a0394278104564092225d67c75e74b99/libagent/formats.py#L192

Doctsrings could be updated to be more accurate.

https://github.com/romanz/trezor-agent/blob/3c911e99a0394278104564092225d67c75e74b99/libagent/formats.py#L240-242

Would need updating if changes are made to L14-21.

The considerations will likely necessitate changes in several other files too. If you agree with them I would be happy to submit PRs. I'm trying to learn to program (with python) and like this project because it serves my needs with my Trezor-T.

doolio commented 1 year ago

The curve_name naming here is also misleading if the default value is correct. https://github.com/romanz/trezor-agent/blob/3c911e99a0394278104564092225d67c75e74b99/libagent/age/client.py#L15

doolio commented 7 months ago

@romanz any further thoughts on the other issues I raised above?