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

Add rsassa-pss-sha256 verifier #126

Closed

Conversation

cce
Copy link

@cce cce commented Oct 19, 2020

Based on RSAVerifier removed in #96 and ECDSA support added in #98

This improves compatibility with reference Python TUF and other TUF implementations. go-tuf already supports the other two verifiers (ECDSA, ED25519) mentioned in the TUF spec, so this completes the set.

Happy to make changes, add tests, etc as per PR feedback.

@coveralls
Copy link

coveralls commented Oct 19, 2020

Pull Request Test Coverage Report for Build 315492196

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 15 of 23 (65.22%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.02%) to 66.997%

Changes Missing Coverage Covered Lines Changed/Added Lines %
verify/verifiers.go 15 23 65.22%
Totals Coverage Status
Change from base Build 277147652: -0.02%
Covered Lines: 1553
Relevant Lines: 2318

💛 - Coveralls

@cce
Copy link
Author

cce commented Oct 19, 2020

One idea I had for integration testing was to add RSA-signed repos from the Python reference TUF project to go-tuf's python_interop tests. The Python reference TUF unit/integration test repos use a mix of rsassa-pss-sha256 and ed25519, but go-tuf uses only ed25519 for the python_interop tests. This was a bigger change than I wanted to make without advice from a go-tuf maintainer though. Instead I copied the ECDSA verifier tests (including the ecdsaSigner) and made RSA versions of them.

@cce cce force-pushed the rsassa-pss-sha256 branch from 0848051 to 66f0610 Compare October 19, 2020 13:11
@cce
Copy link
Author

cce commented Jan 27, 2021

Any comments/feedback?

@trishankatdatadog
Copy link
Member

Hi @shibumi and @erickt, could we get some help reviewing here? 🙂

@shibumi
Copy link
Contributor

shibumi commented Jan 28, 2021

@cce your code will not work with PKCS1, correct? Maybe we should mention this somewhere in a comment. Despite that it looks good to me :)

@cce
Copy link
Author

cce commented Feb 4, 2021

Oh hrmm. I see you are trying each of Parse{PKCS8,PKCS1,PKIX} in order in in-toto-golang parseKey. I can implement the same parse order in this PR, if that works for you.

It only does PKIX now, I was initially just trying to get compatibility for verifying Python TUF where you see lots of "BEGIN PUBLIC KEY" in the examples — but now I am catching up on threads like secure-systems-lab/securesystemslib#251 and #127 (from my colleague @pawel-kedzior-sw) and reading up on the in-toto roadmap doc about new signing specs and an abstract signing interface for Python.

@shibumi do you think in-toto-golang and go-tuf may end up sharing a library like the Python implementations do? It would be nice to have a signing interface to plug in alternate implementations, e.g. HSMs or AWS KMS asymmetric key APIs, without maintaining a fork or adding the AWS SDK to these projects directly.

@cce
Copy link
Author

cce commented Feb 5, 2021

@shibumi OK! I added PKCS8/PKCS1/PKIX as per your feedback and think this is ready now.

@cce
Copy link
Author

cce commented Feb 9, 2021

@erickt I believe this is approved and ready to merge.
@shibumi still curious your thoughts on a shared Go library for the signing spec and abstract Signer interface used by the Go TUF and Go in-toto implementations.

@shibumi
Copy link
Contributor

shibumi commented Feb 9, 2021

@cce I am not sure if in-toto-go and tuf-go share so much common code for signing. If they do, we should definitely use a shared signing library for this. I really like your abstract signer interface implementation, I am just afraid it take us a lot of work to come up with a solution that will work for both projects.

@trishankatdatadog
Copy link
Member

Yeah, I'm afraid a shared signing library for Go might be too much right now, although we have one in Python for both, but the future signing-spec might help with this...

@lukpueh
Copy link
Member

lukpueh commented Feb 12, 2021

Here's what I was trying to say yesterday at the in-toto community meeting in regards to the here discussed go-securesystemslib (based on my experience with python-{securesystemslib, in-toto and tuf}):

It's generally a good idea to unify the implementation of shared aspects of tuf and in-toto, but a separate library does add maintenance overhead and requires more coordination between tuf and in-toto, which the current {python, golang}-{in-toto, tuf} core dev/maintainer team probably does not have the resources for.

Moreover, python-securesystemslib, which seems like the obvious reference for a golang port, currently has a lot of moving parts, i.e. cleaning up legacy code and preparing for the adoption of the new signing-spec, so a verbatim port is not advisable right now. That's why I suggested to defer work on go-securesystemslib until python-securesystemslib has become more stable, which in addition should also free resources on above mentioned core dev/maintainer team that then could go into the port.

Alternatively, and as @SantiagoTorres suggested, go-securesystemslib could become a completely independent implementation of the new signing-spec instead of a python-securesystemslib go port. While this would require less coordination with the core team, it would mean some duplication of efforts regarding design and implementation that already went or are currently going into python-securesystemslib.

cc @joshuagl, @mnm678, @jku

@trishankatdatadog
Copy link
Member

Update: sorry, we've all been busy, but rumors are that we might get some help soon, stay tuned...

@mnm678
Copy link
Collaborator

mnm678 commented Sep 10, 2021

cc @asraa is this superceded by #148?

@asraa
Copy link
Contributor

asraa commented Sep 10, 2021

cc @asraa is this superceded by #148?

Sort of. #148 has a followup that adds the RSA verifier (not pushed yet, but I can draft it). Most of the functionality is the same, this PR had some issues when I tested it against python generated RSA keys (It doesn't assume pem encoding of the public key string)

@asraa
Copy link
Contributor

asraa commented Sep 23, 2021

This is now totally superseded by #148.

@trishankatdatadog
Copy link
Member

This is now totally superseded by #148.

Closing, thanks!

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.

7 participants