-
Notifications
You must be signed in to change notification settings - Fork 164
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
Adds hashed rekord type #501
Conversation
Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>
d1ca7de
to
e2d92a4
Compare
This is ready for review! Docs and tests added |
Signed-off-by: Asra Ali <asraa@google.com>
Nice! Thanks for taking this over and finishing it up! I'll let @bobcallaway take a look too since I wrote part of this. |
Thanks! |
signature, err := artifactFactory.NewSignature(bytes.NewReader(v.HashedRekordObj.Signature.Content)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
v.sigObj = signature | ||
|
||
key, err := artifactFactory.NewPublicKey(bytes.NewReader(v.HashedRekordObj.Signature.PublicKey.Content)) | ||
if err != nil { | ||
return nil, err | ||
} | ||
v.keyObj = key |
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.
is there a reason to reparse and overwriting v.sigObj
and v.keyObj
here? I think you should ensure they're not nil but otherwise they've already been parsed and loaded by the time Canonicalize
has been called - so i don't think this is needed.
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.
Yep, fixed -- In most other types they're set by FetchExternalEntities
, but here they're set during validate()
during signature verification. Given that i had to make validate()
modify the entry
Signed-off-by: Asra Ali <asraa@google.com>
codeql appears stuck |
} | ||
v.sigObj, err = artifactFactory.NewSignature(bytes.NewReader(sig.Content)) | ||
if err != nil { | ||
return errors.Wrap(err, "creating new signature object") |
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.
this also needs to be a types.ValidationError
because if the signature is malformed it will fail here
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.
agh! i misinterpreted the "all but x should be types.ValidationError" comment and reverted this one for parity with the one below. submitting a fix
} | ||
v.keyObj, err = artifactFactory.NewPublicKey(bytes.NewReader(key.Content)) | ||
if err != nil { | ||
return errors.Wrap(err, "creating new public key object") |
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.
this also needs to be a types.ValidationError
because if the key is malformed it will fail here
* WIP: new hashed type Signed-off-by: Dan Lorenc <lorenc.d@gmail.com> * wip add signature verification Signed-off-by: Asra Ali <asraa@google.com> * address bobs comments Signed-off-by: Asra Ali <asraa@google.com> Co-authored-by: Dan Lorenc <lorenc.d@gmail.com>
Summary
Ticket Link
Fixes #481
Release Note