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

Is trezor-agent --help correct? #431

Open
doolio opened this issue Sep 5, 2023 · 4 comments
Open

Is trezor-agent --help correct? #431

doolio opened this issue Sep 5, 2023 · 4 comments
Assignees

Comments

@doolio
Copy link
Contributor

doolio commented Sep 5, 2023

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
Copy link
Owner

romanz commented Sep 6, 2023

You're right - good catch!

@romanz romanz self-assigned this Sep 6, 2023
@doolio
Copy link
Contributor Author

doolio commented Sep 7, 2023

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:

"""SSH format parsing and formatting tools."""

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.

# Supported ECDSA curves (for SSH and GPG)
CURVE_NIST256 = 'nist256p1'
CURVE_ED25519 = 'ed25519'
SUPPORTED_CURVES = {CURVE_NIST256, CURVE_ED25519}
# Supported ECDH curves (for GPG)
ECDH_NIST256 = 'nist256p1'
ECDH_CURVE25519 = 'curve25519'

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.

# SSH key types
SSH_CERT_POSTFIX = b'-cert-v01@openssh.com'
SSH_NIST256_DER_OCTET = b'\x04'
SSH_NIST256_KEY_PREFIX = b'ecdsa-sha2-'
SSH_NIST256_CURVE_NAME = b'nistp256'
SSH_NIST256_KEY_TYPE = SSH_NIST256_KEY_PREFIX + SSH_NIST256_CURVE_NAME
SSH_NIST256_CERT_TYPE = SSH_NIST256_KEY_TYPE + SSH_CERT_POSTFIX
SSH_ED25519_KEY_TYPE = b'ssh-ed25519'
SSH_ED25519_CERT_TYPE = SSH_ED25519_KEY_TYPE + SSH_CERT_POSTFIX
SUPPORTED_KEY_TYPES = {SSH_NIST256_KEY_TYPE, SSH_NIST256_CERT_TYPE,
SSH_ED25519_KEY_TYPE, SSH_ED25519_CERT_TYPE}

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?

Construct a verifier for ECDSA signatures.
The verifier returns the signatures in the required SSH format.
Currently, NIST256P1 and ED25519 elliptic curves are supported.

Currently, NIST256P1 and ED25519 elliptic curves are supported.

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
Copy link
Contributor Author

doolio commented Sep 11, 2023

The curve_name naming here is also misleading if the default value is correct.

result = interface.Identity(identity_str='age://', curve_name="ed25519")

@doolio
Copy link
Contributor Author

doolio commented Apr 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants