feat(core): KID in NanoTDF KAS ResourceLocator borrowed from Protocol #1222
Conversation
This file contains hidden or 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
The 'getURL' method has been updated to 'GetURL' in the ResourceLocator struct. Also, a 'GetKasUrl' method has been added to the NanoTDFHeader struct. These updates ensure KID is effectively fetched and used within the code. A Query parameter handling function 'addQueryParamToURL' has also been added to help in adding "kid" parameter to the URL.
Added a debug log to display the kasPublicKey fingerprint. Also, the 'kid' parameter in 'addQueryParamToURL' method is now hardcoded to "e0" instead of using the result of the calculated 'kidHex'.
The error message that is returned when there's an issue with extracting the policy in rewrap.go has been updated to start with a capital letter, ensuring consistency in error message formatting across the codebase.
The update modifies the function to omit the 'kid' query parameter from all URLs. This parameter was deleted from the queries and then re-encoded. The change prevents potential issues that might arise from passing 'kid' in our requests.
The commit modifies the function to directly retrieve the key identifier (kid) from the KAS public key endpoint instead of computing it inline. This simplifies the process, reduces code redundancy, and makes debugging easier, since now the 'kid' is being fetched directly along with the public key.
Extended the ResourceLocator struct to include an identifier field and adjusted its methods to support setting and serialization of the identifier. Removed the addQueryParamToURL function as it is no longer required.
Introduce the GetIdentifier method to retrieve identifiers based on protocol enums, enhancing the ResourceLocator functionality. Refactor rewrap logic to handle KID parsing directly, removing redundant URL parsing code.
Reorder the import statements to follow the standard Go conventions, ensuring that built-in packages are listed before third-party packages. This improves code readability and maintainability.
Renamed `urlProtocol` to `protocolHeader` for clarity and updated identifier encoding logic. Simplified the identifier length validation and adjusted the reading logic to handle different identifier lengths correctly.
Updated method calls from getURL() to GetURL() for consistency with naming conventions. Adjusted the order of imports in sdk_test.go to remove duplicate and align with best practices.
Corrected the handling of resource locator identifier for various protocols and fixed typos. Added missing `urlProtocolHTTPS` case and applied `nolint` directives to magic number comments for better code clarity and linting compliance.
Updated comments to more clearly indicate byte lengths in sdk/resource_locator.go. Fixed function name capitalization in sdk/nanotdf.go and service/kas/access/rewrap.go from GetKasUrl to GetKasURL for consistency.
Previously, the code attempted to update the KAS URL regardless of whether the 'kid' value was empty or not. This change ensures that the KAS URL is only updated if 'kid' is provided, preventing potential errors when 'kid' is empty.
This commit introduces functionality to handle HTTP URLs in the resource locator. It includes validation of URL length and assignment of protocol values based on identifier length. Additionally, it sets the URL body and identifier fields appropriately.
Updated the GetURL function to use a switch statement with bitwise AND for better protocol handling. Added cases for unsupported protocols with specific identifiers to improve error reporting.
Updated error messages in `standard_crypto.go` to differentiate between specific failure scenarios during key retrieval. Added new error constants in `errors.go` for better clarity and debugging.
Renamed ErrKeyPairInfoNotFoundMalformed to ErrKeyPairInfoMalformed for clarity. Adjusted logging contexts to include additional information and switched from Warn to Error for lookupKid failures. These changes improve error messaging and provide better insights during debugging.
Updated the error message for `NoIdentifier` and `urlProtocolHTTPS` cases to make it clear they are considered legacy. Also, added a check to ensure the identifier is not empty for `TwoByteIdentifier`, `EightByteIdentifier`, and `ThirtyTwoByteIdentifier` cases, providing a clear error message if it is missing.
pflynn-virtru
changed the title
dmihalcik-virtru
previously requested changes
Aug 2, 2024
Updated `resource_locator.go` to handle `urlProtocolSharedRes` by appending a colon, avoiding errors for shared resources. Removed handling for unsupported protocols previously causing errors.
Renamed identifier constants to follow a more consistent and descriptive naming convention. This improves code readability and aligns naming with standard practices, making it easier to understand and maintain.
Enhanced the KAS URL update logic to handle cases where the kid is set alongside the noKID feature toggle. Additionally, updated the `go.work.sum` file to reflect the latest versions of several dependencies, ensuring compatibility and leveraging new features.
Introduces a new flag, `--no-kid-in-nano`, to the encryption command to disable storing key identifiers in the NanoTDF format for legacy compatibility. Updates corresponding test cases and SDK options to support this feature.
Renamed variables and adjusted options to ensure the correct flag 'noKIDInNano' is used consistently in encryption configuration. This change prevents potential misconfigurations and ensures legacy file compatibility.
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
Warning This pull request does not reference any issues. Please add a reference to an issue in the body of the pull request description. |
strantalis
reviewed
Aug 12, 2024
There was a problem hiding this comment.
It looks like you aren't parsing them from the Reader or serializing off-size identifiers properly. I've put in some suggestions for dealing with them but I'd be okay with a cleaner solution. e.g. maybe store the protocol alone in memory and only figure out the identifier | protocol during serialization, since that information is already in the identifier field itself (its length). maybe still do a length check during SetIdentifier though
Revised configurations to toggle KID storage in NanoTDF files, ensuring compatibility with legacy formats. Updated tests and command-line flags to reflect the new implementation.
Introduced a method to determine identifier lengths based on protocol headers in `resource_locator.go`. Updated `NewResourceLocator` and `SetResourceLocator` functions to consider these lengths when processing identifiers, including padding where necessary.
Introduce tests to verify identifier length, and update ResourceLocator functionality to handle identifiers correctly during URL processing and storage. Ensure errors are raised for empty identifiers and data is written properly.
Update condition to check nanoFeatures.noKID instead of tdfFeatures.noKID to ensure the correct feature flag is used when setting the KAS URL with kid. This bug fix prevents potential misconfigurations and ensures the intended logic is applied.
…rl-protocol-enun # Conflicts: # sdk/options.go
Refactored the resource locator reader function to use a dedicated method, reducing code duplication and improving readability. Updated error messages to display protocol values in hexadecimal format, providing clearer diagnostics when encountering unsupported or missing protocol identifiers.
Modified the protocol validation logic in `ResourceLocator` to use bitwise operations for more accurate protocol comparison. This addresses potential issues with future protocol support and ensures robust protocol handling.
biscoe916
dismissed
dmihalcik-virtru’s stale review
feedback has been addressed
biscoe916
approved these changes
Aug 23, 2024
This was referenced Aug 23, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 23, 2024
🤖 I have created a release *beep* *boop* --- ## [0.3.12](sdk/v0.3.11...sdk/v0.3.12) (2024-08-23) ### Features * **core:** KID in NanoTDF KAS ResourceLocator borrowed from Protocol ([#1222](#1222)) ([e5ee4ef](e5ee4ef)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 23, 2024
🤖 I have created a release *beep* *boop* --- ## [0.4.21](service/v0.4.20...service/v0.4.21) (2024-08-23) ### Features * **core:** KID in NanoTDF KAS ResourceLocator borrowed from Protocol ([#1222](#1222)) ([e5ee4ef](e5ee4ef)) ### Bug Fixes * **authz:** entitlements fqn casing ([#1446](#1446)) ([2ffc66b](2ffc66b)), closes [#1359](#1359) * **core:** Autobump service ([#1417](#1417)) ([e6db378](e6db378)) * **core:** Autobump service ([#1441](#1441)) ([e17deab](e17deab)) * **core:** Autobump service ([#1449](#1449)) ([7e443da](7e443da)) * **core:** case sensitivity in AccessPDP ([#1439](#1439)) ([aed7633](aed7633)), closes [#1359](#1359) * **core:** policy db should use pool connection hook to set search_path ([#1443](#1443)) ([8501ff5](8501ff5)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
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.
identifierfield to Resource LocatoridentifiersizeCloses #1203
Issue: #1203
Specification: opentdf/spec#40
ADR: #900