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

Updates for parsing hashedrekord LogEntry #152

Merged
merged 3 commits into from
Oct 24, 2022

Conversation

lkatalin
Copy link
Contributor

Summary

Updates to remove nonexistent fields and add functionality to parse hashedrekord LogEntry struct.

@lkatalin lkatalin requested review from lukehinds and flavio October 24, 2022 04:45
Comment on lines 85 to 86
let decoded_str = std::str::from_utf8(&decoded)?;
Ok(String::from(decoded_str))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpicking, this can replaced with something like String::from_utf8

Comment on lines 24 to 25
let decoded_str = std::str::from_utf8(&decoded)?;
let body = serde_json::from_str::<Body>(decoded_str)?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be replaced with:

let body = serde_json::from_slice(decoded)?;

The presence of the url field does not allow a hashedrekord
entry to be parsed properly. The hash field should be public
for parsing also.

Signed-off-by: Lily Sturmann <lsturman@redhat.com>
The presence of the field does not allow a hashedrekord entry to be
parsed properly.

Signed-off-by: Lily Sturmann <lsturman@redhat.com>
@lkatalin lkatalin force-pushed the parse_logentry branch 2 times, most recently from b088532 to 112ccee Compare October 24, 2022 13:11
- Make signature fields public
- Add Body struct and implementation
- add base64 decoding

Signed-off-by: Lily Sturmann <lsturman@redhat.com>
@lkatalin
Copy link
Contributor Author

You are right, thank you @flavio!

@lkatalin lkatalin requested a review from flavio October 24, 2022 13:19
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


pub fn decode(&self) -> Result<String, SigstoreError> {
let decoded = decode(&self.content)?;
String::from_utf8(decoded).map_err(|e| SigstoreError::from(e.utf8_error()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is totally fine, but if you want to save some keystrokes (and avoid to go crazy figuring out how to convert the error), I find totally acceptable to do something like that:

let response = String::from_utf8(decoded)?;
Ok(response)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me know if you want me to merge this as it is, or if you want to use the code from above. I'm totally fine with the current PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I was going a little bit crazy. 🙃 However, I just tried this and it gives me an error

error[E0277]: `?` couldn't convert the error to `errors::SigstoreError`
  --> src/rekor/models/hashedrekord.rs:86:50
   |
86 |         let response = String::from_utf8(decoded)?;
   |                                                  ^ the trait `From<FromUtf8Error>` is not implemented for `errors::SigstoreError`

I can always implement FromUtf8Error as a SigstoreError in errors.rs, but it is so similar to Utf8Error that it seemed redundant. Is there still a way to use your trick in this case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch, yes... that would be redundant.

I think you could map this error into a more generic Sigstore one, like the UnexpectedError, you could build a message string with format! and provide some more context about the failure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yes, that is an option. I think mapping it to Unexpected with a string format would be more verbose (and in a sense less precise). Would you want to keep it as-is since we can't automatically convert to SigstoreError cleanly? Unless you think we do need more context, then I can add some.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's leave as it currently is, it's much cleaner. If we need to map this type of error again, we can then implement a StringFromUf8Error or something like that inside of SigstoreError

@flavio flavio merged commit 0725c0d into sigstore:main Oct 24, 2022
flavio added a commit to flavio/sigstore-rs that referenced this pull request Nov 24, 2022
Fixes
=====

* Fix typo in cosign/mod.rs doc comment by @danbev in sigstore#148
* Fix typo in KeyPair trait doc comment by @danbev in sigstore#149
* Update cached requirement from 0.39.0 to 0.40.0 by @dependabot in sigstore#154
* Fix typos in PublicKeyVerifier doc comments by @danbev in sigstore#155
* Fix: CI error for auto deref by @Xynnn007 in sigstore#160
* Fix typo and grammar in signature_layers.rs by @danbev in sigstore#161
* Remove unused imports in examples/rekor by @danbev in sigstore#162
* Update link to verification example by @danbev in sigstore#156
* Fix typos in from_encrypted_pem doc comments by @danbev in sigstore#164
* Fix typos in doc comments by @danbev in sigstore#163
* Update path to fulcio-cert in verify example by @danbev in sigstore#168

Enhancements
============

* Add getter functions for LogEntry fields by @lkatalin in sigstore#147
* Add TreeSize alias to Rekor by @avery-blanchard in sigstore#151
* Updates for parsing hashedrekord LogEntry by @lkatalin in sigstore#152
* Add certificate based verification by @flavio in sigstore#159
* Add support for OCI Image signing (spec v1.0) by @Xynnn007 in sigstore#158
Contributors
============

* Avery Blanchard (@avery-blanchardmade)
* Daniel Bevenius (@danbev)
* Flavio Castelli (@flavio)
* Lily Sturmann (@lkatalin)
* Xynnn (@Xynnn007)

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@flavio flavio mentioned this pull request Nov 24, 2022
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.

2 participants