-
Notifications
You must be signed in to change notification settings - Fork 25
Add support for creating and verifying DSSE attestations #1084
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
base: main
Are you sure you want to change the base?
Conversation
e2be1c9 to
6db83ac
Compare
7dd7cb0 to
ef412b2
Compare
Signed-off-by: Aaron Lew <[email protected]>
ef412b2 to
20f6018
Compare
loosebazooka
left a comment
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.
looks pretty good some minor things
| @Override | ||
| int hashCode(); | ||
|
|
||
| @Override | ||
| boolean equals(Object obj); | ||
|
|
||
| @Value.Check | ||
| default void check() { | ||
| // This is a workaround for immutables not using Arrays.equals for derived byte[] | ||
| // see: https://github.com/immutables/immutables/issues/1610 | ||
| if (!Arrays.equals(getPAE(), getPAE())) { | ||
| throw new IllegalStateException("Should be unreachable"); | ||
| } | ||
| } |
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 any of this extra stuff necessary, seems like @Auxilary would be enough?
| if (subject.getName() != null && !subject.getName().isEmpty()) { | ||
| continue; | ||
| } | ||
| throw new IllegalArgumentException("Payload must contain at least one non-empty subject"); |
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.
Maybe, this error message could be more descriptive?
Payload must contain valid subjects?
| return signFiles(List.of(artifact)).get(artifact); | ||
| } | ||
|
|
||
| public Bundle attest(String payload) throws KeylessSignerException { |
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.
Attest seems to be pretty similar to sign, can we refactor out some of the common parts?
It looks like it might a little complicated, but I'd likk to see what it could look like?
| } | ||
|
|
||
| @Test | ||
| public void attest_validation() throws Exception { |
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 don't think this test is useful here for attest because we only have a single call of it. We used it in signFile/signDigest to ensure that the behavior made sense for all those depending on a single impl in signDigest
Closes #991
Summary
This change adds support for creating and verifying DSSE attestations.