Title: Scheme/parsec-tpm: Add TPM vendor and model information #345
Conversation
This change implements support for TPM vendor and model information in TPM endorsements (Issue veraison#139). The implementation includes: - Extended TaAttr structure with vendor and model fields - Input validation and sanitization for security - Comprehensive test coverage including: * Real-world TPM vendor/model combinations * International character handling * Security test cases * Edge cases and input validation - Updated documentation with new fields and security considerations Signed-off-by: Sukuna0007Abhi <[email protected]>
Sukuna0007Abhi
changed the title
scheme/parsec-tpm/README.md
Outdated
| "key": [{ | ||
| "type": "pkix-base64-key", | ||
| "value": "MFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAETKRFE_RwSXooI8DdatPOYg_uiKm2XrtT_uEMEvqQZrwJHHcfw0c3WVzGoqL3Y_Q6xkHFfdUVqS2WWkPdKO03uw==", | ||
| "parameters": { |
There was a problem hiding this comment.
I don't believe CoRIM crypto keys have additional parameters. Is this some extension or proposed change to the spec?
There was a problem hiding this comment.
Hi sir @setrofim You're absolutely right to question this - CoRIM crypto keys in the standard specification do not have additional parameters.
This is indeed a Veraison-specific extension to the CoRIM spec. The standard IETF CoRIM specification defines crypto keys with only type and value fields.
I've implemented this extension because:
TPM-specific needs: TPM vendor/model information doesn't map naturally to the standard environment.class structure used by other schemes (like PSA)
Parsec profile: This follows the custom Parsec TPM profile ("tag:github.com/parallaxsecond,2023-03-03:tpm") which allows for scheme-specific extensions
Optional & backward compatible: Existing CoRIMs continue to work - this only adds optional metadata
I've updated the documentation to clearly indicate this is a Veraison extension and not part of the standard CoRIM specification. The implementation validates against our specific profile rather than claiming standard compliance.
Should I consider an alternative approach that stays closer to standard CoRIM, or is this extension acceptable for the parsec-tpm scheme's specific requirements? Pls reply
There was a problem hiding this comment.
crypto-key-type-choice offers "type choice" rather than "map" extension point. Meaning you cannot add fields to existing values, you can only add new types. So you cannot add parameters to, e.g., "pkix-base64-key", you'd need to define a whole new type, "pkix-base64-key-with-params" and register that as an extension to the type choice (and then do the same for every other variant that needs to be extended in this way).
There was a problem hiding this comment.
Thanks for guiding sir @setrofim, understood
Following @setrofim's feedback that crypto-key-type-choice uses 'type choice' not 'map' extension point, refactored to comply with standard CoRIM specification. Changes: - REMOVED: key.Parameters approach (invalid per CoRIM spec) - ADDED: Extract vendor/model from environment.class (standard location) - Follows same pattern as PSA scheme - Fixed regex backreference issues in sanitization - Updated README with complete, valid CoRIM examples - Deleted ta_attrs.go and related test files (wrong approach) - All tests passing This addresses the review comment that we cannot add parameters to existing key types like 'pkix-base64-key'. Instead, vendor/model are now stored in the standard environment.class location. Resolves: CoRIM specification compliance issue Signed-off-by: Sukuna0007Abhi <[email protected]>
Sukuna0007Abhi
force-pushed
the
feature/tpm-vendor-model
branch
from
67d95ec to
7949f54
Compare
|
Ready for review sir @setrofim sir @yogeshbdeshpande sir @thomas-fossati |
scheme/parsec-tpm/sanitize.go
Outdated
| @@ -0,0 +1,213 @@ | |||
| // Copyright 2024 Contributors to the Veraison project. | |||
There was a problem hiding this comment.
I am wondering this generic implementation of string validation and checking for specific patterns is applicable more generically across entire Services scheme and could be moved to services/common ??
There was a problem hiding this comment.
I don't think this validation is particularly generic. For example, dangerousChars set does not include some punctuation that one may want to escape in other contexts (e.g. @, -, =, +, etc). Some checks (hasExcessiveRepitition, 'hasAbnormalDistribution`) use seemingly arbitrary thresholds. This all seems reasonable for Parsec/TPM (at least, for the prespective of someone not intimately familiar with the specs). However, if we want something generic, this is inadequate.
I'm not even sure we want generic santization -- different systems have different restriction on what they accept and what needs to be escaped (e.g. URL vs DB input vs environment/shell variables). It is better to do specific filtering/escaping at the point where it is relevant. Otherwise we just end up with iflters that are overly restrictive or result in excessive escaping.
There was a problem hiding this comment.
ok, understood, thanks. Are these then coming from a spec reference for restriction in TPM specific scene setting?
There was a problem hiding this comment.
Not that I'm aware of (as I've said -- I'm not familiar with the specs). But this looks like it's designed specifically to catch "unreasonable" vendor/model names. From that point of view, the implementation looks reasonable. But I would not accept this as a good generic santizer (not do I think it's necesarily a good idea to have one).
Signed-off-by: Sukuna0007Abhi <[email protected]> Co-authored-by: Yogesh Deshpande <[email protected]>
Signed-off-by: Sukuna0007Abhi <[email protected]> Co-authored-by: Yogesh Deshpande <[email protected]>
|
@Sukuna0007Abhi Let us discuss this PR in Veraison Meeting on 07/10/2025 |
|
Sure sir, @yogeshbdeshpande will try my best, to explain properly 🙏 |
Description:
This PR implements support for TPM vendor and model information in TPM endorsements, addressing Issue #139.
Changes include:
Security Considerations:
Testing:
Documentation:
Resolves: #139
Ready for Review sir @thomas-fossati @setrofim @yogeshbdeshpande