Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: Assertion verification #452
base: main
Are you sure you want to change the base?
feat: Assertion verification #452
Changes from 16 commits
6de7a5f
33a806e
5eacf3c
ce33b1e
cd422cd
a4fa180
5e812be
fffb2ef
898d500
33de674
2db8d06
4a90670
45bea1e
ae23aa8
2ff3f96
b77b3b8
73baae7
a2dd130
19523aa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we should only support the file method. A user may not be aware of the risks of specifying the key as a command argument.
We could consider adding argument support after #417 is implemented.
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 keeping the json string options was just to maintain backward compatability, we can add some more documentation here to emphasize that its deprecated and the file path is the more safe option
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.
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.
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.
Are assertions present in the manifest? If so, can we add a test with
./otdfctl inspect
that asserts they were found present in the TDF created?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 comparison struct equality evaluate as expected? It seems like you'd need to compare struct fields for this equality check?
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.
Do you think this functionality could be exposed within provided platform lib/SDK functionality instead of recreated within each Go PEP?
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.
seems like most go packages that deal with crypto just take in args that are already typed rsa.PrivateKey or rsa.PublicKey, they assume you already have the key and if not they expect you to load it yourself. i think the cli has a bit extra code since the whole assertion object is being passed in as a string which isnt how it would normally be done in a go pep. i assume the pep would just construct the assertion similar to https://github.com/opentdf/platform/blob/7d55a909ac03e5a1234709d9051f906d9efd469e/sdk/tdf_test.go#L400-L425 and would just load the key from pem; also the cli has extra conditions to handle private vs public where as i assume a pep would know if it was a private key or public key they were loading. so TLDR i think the cli is more of an outlier in how its passing in assertions and the extra steps needed to parse them and want to avoid adding specific logic to the sdk just for the cli.
but if its determined that other peps have this need as well we can re-assess