feat(kas): Partial implementation EC384 and EC521 support in KAS #883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pflynn-virtru
changed the title
dmihalcik-virtru
requested changes
May 30, 2024
There was a problem hiding this comment.
This is not backward compatible and will break hsm.go. Can you test with hsm enabled?
Also, we may need instructions about how to upgrade - e.g. what do people need to change in their opentdf.yaml files and/or rename their existing EC key files? Will this break existing nanotdf documents?
|
@biscoe916 can we get a definitive decision on HSM? |
pflynn-virtru
force-pushed
the
881-support-additional-elliptic-curves
branch
from
a690ea3
to
a3bf7b5
Compare
Enabled EC384 and EC521 support in KAS and added relevant unit tests. The config file now supports the new EC384 and EC521 key types. Also, test keys for both types have been generated and added.
Improved the clarity of an error message in the "NewOpenTDFServer" method. Refactored key configurations in the dev and example YAML files to differentiate between multiple elliptic curve securities. Also made updates to various scripts to generate and handle these keys suitably.
This commit includes the refactoring of the publicKey.go file for better error handling and support for different algorithms. The log messages have been updated to provide clearer context upon failure of key generation methods for both EC and RSA algorithms. Also, changes were made to the init-temp-keys.cmd script to correct the path of the generated keys.
The code was updated to address an issue where an unknown key algorithm could be passed to the publicKey functions. It now properly catches this event and logs an error message. The additional condition checks if the algorithm is not RSA in addition to being non-empty, helping prevent potential configuration errors.
This commit modifies the execution of the `init-temp-keys.sh` script in `.github/workflows/checks.yaml` file. The change includes passing '--output ./keys/' argument to the script to dictate where the keys will be stored. This specifies the directory for the keys and enhances organization.
Updated the paths for secp256r1, secp384r1, and secp521r1 key files in the opentdf-dev.yaml file, removing the '/keys/' prefix. Also modified the path in the init-temp-keys.sh command within checks.yaml. This adjustment accommodates a change in file structure and ensures consistent script execution.
This commit includes modifications to symmetric key generation in the crypto provider. Now, GenerateNanoTDFSymmetricKey requires a keyID in addition to ephemeralPublicKeyBytes. This change allows for better security features as the encryption processes are enhanced. Minor changes such as comments added to the decrypt file and an adjustment to test output validation were also implemented.
The attributes.swagger.json file in the policy documentation was deleted. It was located in the docs/openapi/policy/attributes directory. This file contains the OpenAPI specification for attribute related services. The commit message should reflect the elimination of this detailed specification file.
The if-else constructs in the GetCurveName() method of the NanoTDFHeader struct in nanotdf.go have been refactored to a switch-case statement for better readability. Also, the invocation to retrieve the public key algorithm has been updated to use req.GetAlgorithm rather than directly accessing the property.
Added secp256k1 to the ECCMode in nanotdf.go. Also, the logging level for the 'kas public key algorithm' has been downgraded from Info to Debug to reflect its utilization better and reduce log volume in non-debug scenarios.
The kNanoTDFGMACLength constant and ErrInternal error constant were removed as they were not being used. The unnecessary ECCurve fetching code was also removed, now ECCurve is fetched directly from the header. The CryptoProvider now generates keys based on this directly fetched ECCurve.
The public key handling process has been updated to accommodate three different elliptic curve algorithms: EC256R1, EC384R1 and EC512R1. The tests have also been adjusted to ensure these changes function as expected. Algorithm constants in the crypto provider code file have also been updated to reflect these changes in public key handling.
The cryptoProvider configuration in both opentdf-dev.yaml and opentdf-example.yaml files has been refactored. The rsa and ec field structure has now been replaced with a simpler, streamlined `keys` field, which should enhance readability and maintainability of these configurations.
Updated both the .cmd and .sh versions of the init-temp-keys scripts to include the generation of elliptic curve (EC) keys with prime256v1 curve. Now, alongside RSA and secp256r1 key pairs, the scripts also generate prime256v1 EC key pairs.
pflynn-virtru
force-pushed
the
881-support-additional-elliptic-curves
branch
from
369e2fe
to
6b1fc14
Compare
The encryption functionality has been updated to allow for more options, such as the nano format and ability to disable storing key identifiers in TDF KAOs. The encrypt command has also been refactored for conciseness and clarity. Code for token endpoint during client creation is removed, and new options have been added to address the nanoTDF format and managing key identifiers.
patmantru
requested changes
Jun 14, 2024
In the SDK, error logging and returning has been implemented for the cases when an unsupported algorithm (ec:secp256k1) or any unknown algorithm is used. This will aid in better fault detection and diagnosis.
The decryption steps in the tdf-roundtrips bats test were commented out, and have been enabled again. In addition, the decrypt function was updated to accommodate both TDF and Nano files. It now attempts to decrypt both file types instead of returning an error when the file is not a TDF file.
The change removes the specific comparison of the output buffer content with "Hello Virtru" in the decrypt command. Instead, it just prints the content of the output buffer directly, making it more universally applicable.
The decryption command previously printed output directly from the buffer. This commit changes that and converts the output buffer to a string before printing. This may improve readability and consistency across the application.
Removed the warning message in the 'tdf-roundtrips.bats' test file which is no longer relevant. Also, in 'decrypt.go', replaced the 'cmd.Println' method with the 'fmt.Println' method to print the output string. This change is meant to simplify and standardize the print methods across the codebase.
strantalis
reviewed
Jun 17, 2024
patmantru
previously approved these changes
Jun 17, 2024
The function signatures for GenerateEphemeralKasKeys, ECPublicKey, and ECCertificate in hsm.go have been updated. The functions now accept an additional parameter, though it's currently unused. This change provides better flexibility for future adjustments or enhancements of these functions.
dmihalcik-virtru
previously approved these changes
Jun 18, 2024
Co-authored-by: Dave Mihalcik <[email protected]>
…ort-additional-elliptic-curves # Conflicts: # docs/openapi/policy/attributes/attributes.swagger.json # docs/openapi/policy/namespaces/namespaces.swagger.json
patmantru
previously approved these changes
Jun 24, 2024
dmihalcik-virtru
previously approved these changes
Jun 24, 2024
dmihalcik-virtru
dismissed stale reviews from patmantru and themself
via
2f5d945
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Enabled EC384 and EC521 support in KAS and added relevant unit tests. The config file now supports the new EC384 and EC521 key types. Also, test keys for both types have been generated and added.
This will enable creation of a nanotdf with different curves. This will NOT enable decryption of nanotdf since we have no place to put the key identifier, see: #900
partially resolves: #881