-
Notifications
You must be signed in to change notification settings - Fork 108
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 ecdsa verifier implemented according to the specification? #127
Comments
Notifying @titanous, @vladimir-v-diaz, @awwad |
Thanks for pointing this out, @pawel-kedzior-sw! @vladimir-v-diaz and @awwad are no longer affiliated with the project (and I am unsure about @titanous). It is probably best to coordinate with @erickt and @shibumi, who currently maintain As a TUF reference implementation and specification maintainer I can tell you that the exact public key format is not highly relevant to the functioning of TUF and is likely to be removed from the TUF specification in the near future (see secure-systems-lab/dsse#1 for that discussion). That said, if you aim for interoperability with other TUF implementations the key format does indeed matter. |
Is this questions answered or do we want to generate an 'actionable' out of this? |
On a slightly unrelated note, maybe it's best to prune some people who are no longer affiliated with or active in the project from teams or the list of maintainers... |
@lukpueh - agree that as long as interop is not taken into account, the public key format (or the chosen key-type/scheme altogether) is irrelevant. On the other hand there has been put some effort to test interop with python implementation Here is my perspective, the ecdsa verifier is part of go-tuf, yet there is no key-gen operation implemented. So I assume this scheme is not used in wild, yet there must have been some reason that @titanous had in the past to develop it. The key-gen for ecdsa is something I implemented on my private fork (I am working with my management to get green light to open source it and create pull request to go-tuf), and would like to use in production. The problem I foresee is that once this will be used in production, I am locked to the format, yet it does not conform to the specification. So in the future someone may have good reasons to change it to meet the specification, which will break compatibility. |
Thanks for providing context, @pawel-kedzior-sw!
That sounds great! 🎉
I can see the predicament. But once we move forward with the mentioned signing spec and other key format revision (see secure-systems-lab/securesystemslib#251 for related brainstorming), we will need to think of a transitioning/deprecation strategy for adopters anyway. I am a bit unsure if it makes a big difference what key formats you used before. That said, I'm not opposed to your suggestion of fixing this now. The only constraint I could think of is if someone already relied on the currently implemented format. Maybe TUF spec co-maintainer @mnm678, who has worked on interoperability standardization, can also weigh in here. |
For now, if interoperability is desired, it's probably easiest to fix this now. In the more general case, if we later need to change the key format for the signing spec or any other reason, we'll need a deprecation strategy. I've outlined one possible strategy for dealing with backwards-incompatible changes in TAP 14, which describes how repositories could maintain multiple major versions during a transition period, then phase out the old version. |
Closing in favor of #223, which appears to be a duplicate (but has more details) |
The ecdsa-sha2-nistp256 verifier,
which is implemented here: https://github.com/theupdateframework/go-tuf/blob/master/verify/verifiers.go#L49
assumes uncompressed form of public key specified in section 4.3.6 of ANSI X9.62 (because it uses elliptic.Unmarshal function)
The specification however says that this scheme should use format where "PUBLIC is in PEM format and a string."
theupdateframework/python-tuf#498
So it should use: x509.MarshalPKIXPublicKey + pem.EncodeToMemory.
Would it be fine to fix it now? Is such a fix constrained anyhow?
The text was updated successfully, but these errors were encountered: