-
Notifications
You must be signed in to change notification settings - Fork 0
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
Profiles #29
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
yogeshbdeshpande
approved these changes
Jul 5, 2024
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.
Nits, but in general LGTM!
Add CCA platform claims under platform sub-package. These claims on the old psatoken CCA platform claims that were used prior, but have been modified to use the new profiles mechanisms that would allow layering further customisations on top of existing structures. Existing realm claims are moved into realm sub-package. As platform and realm sub-packages are introduced, there is also a minor refactor of the CBOR encoding code. Most if it is moved into encoding sub-package, with each sub-package (including top level) instantiation it's own encoding/decoding modes. This organisation is necessary so that each sub-package can define CBOR tags for its types and avoid cyclic imports. The encoding refactor also means that there are now small cbor.go files instantiating the CBOR modes for their sub-package. These files have 50% coverage that cannot be increased (they contain panics guarding against errors that cannot happen). As go tools evaluate coverage for sub-packages by averaging coverages of individual files, this significantly impacts the coverage metric for the top level, where there is now little code. Rather than lowering the CI minimum coverage requirements module-wide, the coverage script is modified to take overrides for specific sub-packages. Finally, to avoid confusion, an unused duplicate of the coverage script under .github/scripts/ is removed (the CI actually uses the script under top-level scripts/). BREAKING CHANGE: realm claims have been factored out into their own sub-package. E.g. github.com/veraison/ccatoken.IRealmClaims is now github.com/veraison/ccatoken/realm.IClaims. BREAKING CHANGE: psatoken's ICcaPlatformClaims has been replaced by the local platform.IClaims. BREAKING CHANGE: minimum go version bumped to 1.21 to align with updated psatoken. Signed-off-by: Sergei Trofimov <[email protected]>
Refactor realm claims to align with PSA token profile definitions and to allow them to be potentially extended in an analogous way via CCA profiles. - Use psatoken.IClaimsBase to define realm.IClaims. This exposes operations common to all claims objects, namely marshalling and validation. - Expose previously internal realm.validate() as realm.ValidateClaims(). This will help with implementing profiles that do not embed the original claims structure. - Expose individual claim field validators and rename them from isValidXXX (which implies a boolean return) to ValidateXXX. This will be useful if, e.g., client code wants to make sure a hash is a valid realm challenge without needing a claims structure. - Reuse errors defined inside psatoken, rather then re-defining them for realm claims. Errors such as "syntax error" or "missing mandatory claims" are in effect part of the generic IClaimsBase interface (i.e. common to all profile-able claims objects). - Do not use json tags inside error messages, as they may be different for profiles that implement their own claims without embedding existing ones. - Some minor stylistic tidying (consistent spacing, etc). Signed-off-by: Sergei Trofimov <[email protected]>
Signed-off-by: Sergei Trofimov <[email protected]>
yogeshbdeshpande
approved these changes
Jul 18, 2024
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.
LGTM!
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.
This adds platform claims implemented as a "profile" of psatoken. This also refactors realm claims to make use of similar mechanisms (and generally remove duplication between this and
psatoken