-
Notifications
You must be signed in to change notification settings - Fork 21
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
WIP: Defining an interface for allowing signing and verifying with different envelope types #109
Conversation
Signed-off-by: chaosinthecrd <[email protected]>
Signed-off-by: chaosinthecrd <[email protected]>
witness Signed-off-by: chaosinthecrd <[email protected]>
@@ -40,8 +40,8 @@ func (s *ECDSASigner) KeyID() (string, error) { | |||
return GeneratePublicKeyID(&s.priv.PublicKey, s.hash) | |||
} | |||
|
|||
func (s *ECDSASigner) Sign(r io.Reader) ([]byte, error) { | |||
digest, err := Digest(r, s.hash) | |||
func (s *ECDSASigner) Sign(ctx context.Context, data []byte) ([]byte, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need to be deprecated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I presume you mean the switch from taking in an io.Reader
vs Context
and []byte
?
So this hasn't been documented at all yet, and probably needs doing so, but switching to this is to align closer to the secure systems lab lib, so moving over to it is less hassle when we do.
However, passing in an io.Reader
might be more beneficial for making all of this test driven, so it probably warrants a wider discussion. Also there might be other perfectly good reasons for this being passed as an argument.
To answer the original question, I am not sure about the need for deprecation inside this cryptoutil
library more generally. Are we intending for it to be used directly? I have also been thinking about embedding it within the signature
package, and only exposing it in the form of NewSignerFromReader
/NewSignerFromFile
. Not sure what your thoughts are on this?
@@ -65,8 +65,8 @@ func (v *RSAVerifier) KeyID() (string, error) { | |||
return GeneratePublicKeyID(v.pub, v.hash) | |||
} | |||
|
|||
func (v *RSAVerifier) Verify(data io.Reader, sig []byte) error { | |||
digest, err := Digest(data, v.hash) | |||
func (v *RSAVerifier) Verify(ctx context.Context, data []byte, sig []byte) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will using []byte vs io.Reader effect memory usage? Verify is likely to be used by a service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question - one that I haven't thought of. The main motivation was to streamline it with the secure systems lab dsse implementation.
cryptoutil/signer.go
Outdated
|
||
// NOTE: Signer verifier required this function and I don't know why | ||
func (sv SignerVerifier) KeyID() (string, error) { | ||
return sv.Signer.KeyID() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the keyID needs to be Canonical, which might explain it
@@ -0,0 +1,108 @@ | |||
package cose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COSE has a draft spec for RFC 3161 ref: https://www.ietf.org/id/draft-ietf-cose-tsa-tst-header-parameter-01.html
We should get this implemented as part of the first release
Signed-off-by: chaosinthecrd <[email protected]>
Given the aims to consolidate this library against other go libraries within in-toto and secure-systems-lib (https://github.com/secure-systems-lab/go-securesystemslib and https://github.com/in-toto/in-toto-golang), we are going to hold off on finishing and merging this work. It will however come in handy when the time comes. |
This is a draft for now so people can see my progress.
I am trying to refactor the
dsse
andcryptoutil
parts of the repo so they can be:a) set up for the expansion to more envelope types to sign and verify
b) make the current internal
dsse
library more streamlined with https://github.com/secure-systems-lab/go-securesystemslib. While we don't use it right now, we plan on migrating to it when it supports timestamps and intermediates (I think this PR might be related secure-systems-lab/dsse#61).