-
Notifications
You must be signed in to change notification settings - Fork 13
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
Implementation of new In-toto 0.1.0, DSSE 1.0.0, and SLSA Provenance 0.1.1 #24
Conversation
This is the fist set of changes for the new implementation of in-toto spec. This includes: * Moving of existing functionality into legacy package * Creating a set of base model classes * Implementing validators * Implementing helper class to start validation process and transform to JSON * Set of tests for the above
- Updated the changelog - Removed unused Makefile - Remove unused .travis.yml
- Added SLSA models and tests. - Added DSSE implementations and simple Signers and Verifiers
* Added better description of helper methods and Signer/Verifier interface * Made sure the `pivateKey` field is final.
Make sure new classes have proper JavaDoc
Added missing JavaDoc
Changed some of the description
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.
Can you provide example output of both the signed JSON DSSE as well as the JSON Statement?
I think that will make it easier to check if all the fields are named correctly, etc...
Also, have you tested this against the reference implementation (in terms of verifying/generating signed DSSEs)?
https://github.com/secure-systems-lab/dsse/blob/master/implementation/signing_spec.py
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.
It would help to have test vectors showings the expected output and testing against that. It's must easier to hand-inspect a known-good JSON file than it is to verify that the code produces the right JSON. :-)
*/ | ||
public static String createPreAuthenticationEncoding(String payloadType, String payload) { | ||
return String.format( | ||
"DSSEv1 %d %s %d %s", payloadType.length(), payloadType, payload.length(), payload); |
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 handle multi-byte UTF-8 characters properly? Could you add a test for that?
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 think so. I added a test fort this.
* LEN(s) = ASCII decimal encoding of the byte length of s, with no leading zeros | ||
*<pre/> | ||
* @param payloadType the type of payload. Fixed for in-toto Envelopes | ||
* @param payload the base64 encoded Statement in JSON |
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 is not supposed to be base64 encoded. It's supposed to be the raw payload in bytes.
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 was following https://github.com/in-toto/attestation/tree/main/spec#envelope where it says that the payload is:
Base64-encoded JSON Statement.
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.
That is for the JSON payload
field. JSON only handles unicode strings, not bytes, so all binary data must be base64-encoded. However, PAE access the raw binary (bytes) payload
, before encoding.
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've made changes to match the spec now, please have a look
* Added missing getters and getters * Fixed issues with time formatting * Replaced `int` to `Integer` to allow `definedInMaterial` to be null * Added SHA256 in the sample signer and verifier. * Removed duplicated code. * Clarified and reformatted README.md * Added the ability to pass key id in the sample Signer * Removed PredicateType enum as its no longer needed.
You can see the output of various things while running the JUnit tests. The test called
I'll see if I can get the python code working to check against it. |
Here's the payload, for reference: {
"_type": "https://in-toto.io/Statement/v0.1",
"subject": [
{
"name": "curl-7.72.0.tar.bz2",
"digest": {
"SHA256": "d4d5899a3868fbb6ae1856c3e55a32ce35913de3956d1973caccd37bd0174fa2"
}
}
],
"predicateType": "https://slsa.dev/provenance/v0.1",
"predicate": {
"builder": {
"id": "mailto:[email protected]"
},
"recipe": {
"type": "https://example.com/Makefile",
"definedInMaterial": 0,
"entryPoint": "src:foo"
},
"metadata": null,
"materials": [
{
"uri": "https://example.com/example-1.2.3.tar.gz",
"digest": {
"sha256": "1234..."
}
}
],
"predicateType": "https://slsa.dev/provenance/v0.1"
}
} Bugs:
|
* Made sure enums are lowercase when converted to JSON * Made sure that predicate inner types does not show when converted to JSON * Created some keys for testing * Made sure the Signer accepts a byte array instead of a Java String * Removed unnecessary Base64 encoding while creating the PAE * Updated tests to reflect the changes
Done. |
I'm working with @MarkLodato to see if we can get a Python command that uses the spect to verify. |
In an effort to make testing easier against other libs. The Java implementation now uses standard keys generated with the OpenSSL command.
Added bigger example with metadata.
src/test/java/io/github/intoto/utilities/TestEnvelopeGenerator.java
Outdated
Show resolved
Hide resolved
* Updated the Statements so that they include their type by default. * Updated README to match implementation * Updated the example output to match the bundle name and extension. * Clarified the Predicate JavaDoc. * Clarified testing comments
DigestSetAlgorithmType.SHA256.toString(), | ||
"d4d5899a3868fbb6ae1856c3e55a32ce35913de3956d1973caccd37bd0174fa2")) | ||
Predicate predicate=createPredicate(); | ||
Statement statement=new Statement(); |
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.
optional: create a version of the constructor that accepts the subject list and predicate as args so users don't have to set them manually.
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.
+1
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 PR is getting big. Let's add this to a post PR merge. I also want to add the ability for the library to generate a DSSE envelope with multiple statements. This could be something we introduce after this PR
Made sure the tests are using the test keys from the files.
* Added additional test that can be used to manually verify a .intoto.jsonl file * Added JavaDoc and made sure pre tags are correctly closed in them.
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.
Looking good!
I found nothing major, but I do have a couple of questions...
DigestSetAlgorithmType.SHA256.toString(), | ||
"d4d5899a3868fbb6ae1856c3e55a32ce35913de3956d1973caccd37bd0174fa2")) | ||
Predicate predicate=createPredicate(); | ||
Statement statement=new Statement(); |
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.
+1
Everything else looks good to me. If Santiago & Aditya are happy them so am I. |
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 think I quite like the look of things here, I don't have any concerns. I do have a question for the Java folks here though regarding the namespacing. I don't know if it's significant in practice, but for several of the package namespaces used here, we don't actually control the domains they represent or even the corresponding GitHub usernames. I do recognize that the hyphen complicates things, and the Google Java style guide says to just concatenate words together: https://google.github.io/styleguide/javaguide.html#s5.2.1-package-names. Do we ever foresee any troubles because we don't control github.com/intoto? For that matter, should we also move the legacy package to be under intoto, because it's currently io.github.legacy? For dsse, we do have the option of using the secure-systems-lab domain as we do for the Go implementation. I imagine it'd be edu.nyu.engineering.ssl.dsse
in this case. And I guess dev.slsa
for that one? Please let me know if I'm overthinking this 😄
@adityasaky you bring up a good point on the package names. Originally I named the packages this way so that they can later be extracted and moved to their relevant repos. But since there is no warranty that this will be the case, I will make the change to match the intoto repository. |
I did another pass today, I think all outstanding issues are solved and the minor ones can be worked on smaller PRs and have been documented. Thank you so much @Alos ! |
Hey folks!
Please have a look at the PR. This change adds the following:
Link
implementation into legacy directory and classes are now tagged as deprecated.@UniqueSubject
annotation along with a newUniqueSubjectValidator
to make sure subjects are unique in theStatement
.gitignore
for JavaSample of a DSSE generated by the library using the included test keys (pretty-printed here for clarity):
Sample of the payload once its Base64 decoded and pretty-printed: