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

feat: Assertion verification #452

Merged
merged 21 commits into from
Dec 17, 2024
Merged

Conversation

elizabethhealy
Copy link
Member

@elizabethhealy elizabethhealy commented Dec 9, 2024

Adds option to provide assertion verification keys and support for signing keys on assertion input
(supports rs256 and hs256 keys)
Includes bats tests for both key types

@elizabethhealy elizabethhealy marked this pull request as ready for review December 10, 2024 15:00
@elizabethhealy elizabethhealy requested a review from a team as a code owner December 10, 2024 15:00
Copy link
Contributor

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

Specifying the keys directly as arguments puts the key at more risk of exposure. Arguments will be recorded to disk in the command history, could be exposed on multi-tenant systems, logged, etc. This is particularly concerning since you appear to be supporting not just public keys, but private keys as well.

@elizabethhealy Would it be reasonable to accept a file path or use environment variables instead of specifying the keys as arguments?

@jakedoublev
Copy link
Contributor

Specifying the keys directly as arguments puts the key at more risk of exposure. Arguments will be recorded to disk in the command history, could be exposed on multi-tenant systems, logged, etc. This is particularly concerning since you appear to be supporting not just public keys, but private keys as well.

@elizabethhealy Would it be reasonable to accept a file path or use environment variables instead of specifying the keys as arguments?

This suggestion from @jentfoo aligns with an open issue we have to generally improve CLI security: #417

@elizabethhealy
Copy link
Member Author

elizabethhealy commented Dec 10, 2024

@jentfoo @jakedoublev
using a file name as input instead makes sense, so in that case the format of the assertions provided would be [{"id":"assertion1","type":"handling","scope":"tdo","appliesToState":"encrypted","statement":{"format":"json+stanag5636","schema":"urn:nato:stanag:5636:A:1:elements:json","value":"{\"ocl\":\"2024-10-21T20:47:36Z\"}"},"signingKey":{"alg":"RS256","key":"my_private_rs256key.pem"}}] and just require that whats in the key field is a path to a file
or should we support an additional field within "signingKey" like "key_file" where a user would pass the pem?

@jakedoublev
Copy link
Contributor

@jentfoo @jakedoublev using a file name as input instead makes sense, so in that case the format of the assertions provided would be [{"id":"assertion1","type":"handling","scope":"tdo","appliesToState":"encrypted","statement":{"format":"json+stanag5636","schema":"urn:nato:stanag:5636:A:1:elements:json","value":"{\"ocl\":\"2024-10-21T20:47:36Z\"}"},"signingKey":{"alg":"RS256","key":"my_private_rs256key.pem"}}]

@elizabethhealy Could the entire assertion JSON be stored and read into the command from a file instead of just the keys referenced by the assertions? I'm thinking that would be aligned with what the CLI does for client credentials so --with-client-creds-file creds.json expects the entire {"clientId":"id","clientSecret":"client_secret"} stored in ./creds.json instead of --with-client-creds-file '{"clientId":"id","clientSecret":"secret_file.txt"}' where just the secret is present in ./secret_file.txt.

@elizabethhealy
Copy link
Member Author

elizabethhealy commented Dec 10, 2024

@jakedoublev i like that, so --with-assertions, --with-assertions-from-file, --with-assertion-verification-keys, --with-assertion-verificiation-keys-from-file and its up to the user to decide whichever is best for them, it will still support passing in keys directly but also give a safer option

@jakedoublev
Copy link
Contributor

@elizabethhealy and I discussed and we will update --with-assertions to parse either from JSON (backwards-compatible) or try to read from a file (more secure) before reporting an error if neither is successful. This new decrypt flag --with-assertion-verification-keys will only read the entire JSON (including in-lined PEMs) from a file and not support JSON in the flag value/argument.

jakedoublev
jakedoublev previously approved these changes Dec 17, 2024
@elizabethhealy elizabethhealy merged commit 5a8fe0d into main Dec 17, 2024
7 checks passed
@elizabethhealy elizabethhealy deleted the add-assertion-verification-key branch December 17, 2024 21:12
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.

3 participants