fix: fixes attribute reading on private objects#805
fix: fixes attribute reading on private objects#805nb-tech5 wants to merge 5 commits intosofthsm:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds virtual helpers to centralize ByteString attribute retrieval and conditional decryption, introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant P11Attribute
participant P11NonPrivateAttribute
participant Token
participant OSAttribute
Caller->>P11Attribute: retrieve(token, isPrivate, attr, ...)
alt attr type == ByteString
P11Attribute->>P11Attribute: retrieveAttrByteString(token, isPrivate, attr, value)
alt attr is instance of P11NonPrivateAttribute
P11NonPrivateAttribute-->>P11Attribute: return raw ByteString (no decrypt)
else isPrivate and value non-empty
P11Attribute->>Token: request decryption(value)
Token-->>P11Attribute: decrypted bytes or CKR_GENERAL_ERROR
end
P11Attribute-->>Caller: return value (size) or CK_RV error
else
P11Attribute-->>Caller: handle other attribute types
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/P11Attributes.h (2)
125-126: Fix typo in comment.There's a typo in the comment: "Retrie" should be "Retrieve".
The method signature and design look good for addressing the asymmetric attribute handling issue.
- // Retrie the value if allowed + // Retrieve the value if allowed virtual CK_RV retrieveAttrByteString(Token *token, bool isPrivate, OSAttribute *attr, ByteString &value);
135-143: Fix typo in comment.Same typo here: "Retrie" should be "Retrieve".
The class design is well-structured for providing non-private attribute behavior.
- // Retrie the value if allowed + // Retrieve the value if allowed virtual CK_RV retrieveAttrByteString(Token *token, bool isPrivate, OSAttribute *attr, ByteString &value);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/P11Attributes.cpp(4 hunks)src/lib/P11Attributes.h(3 hunks)
🔇 Additional comments (5)
src/lib/P11Attributes.h (2)
471-475: Good design choice for start date attributes.Changing
P11AttrStartDateto inherit fromP11NonPrivateAttributeis appropriate since start dates are typically not sensitive data requiring encryption. This should resolve the retrieval failures on private objects mentioned in the PR.
489-493: Consistent and appropriate change for end date attributes.Like the start date change, moving
P11AttrEndDateto inherit fromP11NonPrivateAttributeis consistent and appropriate since end dates don't typically require encryption.src/lib/P11Attributes.cpp (3)
72-87: Well-implemented centralized retrieval method.The implementation correctly handles both private and non-private attributes:
- Attempts decryption for private, non-empty attributes
- Includes proper error handling with logging
- Falls back to raw value for non-private or empty attributes
This centralizes the logic that was previously duplicated in the
retrievemethod.
503-507: Correct implementation for non-private attributes.The
P11NonPrivateAttribute::retrieveAttrByteStringimplementation correctly bypasses decryption and always returns the raw byte string value, which is appropriate for attributes that are never encrypted.
293-299: Excellent refactoring that centralizes retrieval logic.The modifications to the
retrievemethod successfully replace inline decryption logic with calls to the newretrieveAttrByteStringmethod in both the size calculation and value copying sections. This:
- Eliminates code duplication
- Enables polymorphic behavior through virtual method overrides
- Fixes the asymmetric handling issue described in the PR
- Maintains consistent error handling
The refactoring is clean and preserves the original functionality while enabling the fix.
Also applies to: 347-357
|
Hi @bellgrim, would you kindly let me know what could be the time frame to push this change (if accepted) into a release version? Thanks. |
jschlyter
left a comment
jschlyter
left a comment
There was a problem hiding this comment.
Please check test results and fix.
|
It would be nice to include a test for this. |
|
Also it would be good to rebase it so the Linux test runs. |
nb-tech5
force-pushed
the
fix-fixes-reading-public-attribute-from-private-object
branch
from
a197069 to
44f941e
Compare
|
Rebased and changed the RSA private key to use a non-empty start and end dates. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/test/ObjectTests.cpp (2)
1789-1797: Consider adding equivalent coverage for secret keysSince the issue applies to private key and secret key objects, a small additional test that creates a CKO_SECRET_KEY with CKA_START_DATE/CKA_END_DATE and verifies retrieval would broaden regression coverage for the new retrieval hook.
I can draft a minimal extension to testCreateSecretKey that adds start/end dates and validates C_GetAttributeValue; let me know if you want that patch.
1814-1814: Nit: fix the comment to say “private key”This function tests a private key; the comment mentions “public key”.
Apply this diff:
- // Create minimal RSA public key object + // Create minimal RSA private key object
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/lib/P11Attributes.cpp(4 hunks)src/lib/P11Attributes.h(3 hunks)src/lib/test/ObjectTests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/P11Attributes.h
- src/lib/P11Attributes.cpp
🔇 Additional comments (3)
src/lib/test/ObjectTests.cpp (3)
1785-1786: Good addition: concrete CK_DATE values exercise the new non-private retrieval pathThe CK_DATE initializers are well-formed and will effectively validate the symmetric read path on private key objects.
1789-1797: Including CKA_START_DATE/CKA_END_DATE in the RSA private key template is correct and aligned with PKCS#11Placing the date attributes in the creation template ensures they are persisted and subsequently retrievable without decryption. This directly covers the regression described in the linked issue.
1821-1821: Verification step correctly asserts date presence and lengthPassing sizeof(CK_DATE) into checkCommonKeyAttributes ensures both value and length are checked post-retrieval. This should catch any inadvertent decryption attempts.
nb-tech5
force-pushed
the
fix-fixes-reading-public-attribute-from-private-object
branch
from
44f941e to
54eee02
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/lib/test/ObjectTests.cpp (2)
1794-1797: Including CKA_START_DATE/CKA_END_DATE in the private RSA key template is appropriateThis exercises the new retrieval path on private objects where these attributes are public. Nice.
Optional enhancements:
- Extend coverage to RSA public keys and secret keys to prove symmetry across key classes (PKCS#11 defines these attributes for all key objects). This helps prevent regressions limited to one class.
If you want, I can draft a follow-up patch that:
- Adds start/end dates to testDefaultRSAPubAttributes’ creation template and updates its checkCommonKeyAttributes call accordingly.
- Adds a small secret-key test that sets CKA_START_DATE/CKA_END_DATE and verifies retrieval via checkCommonKeyAttributes.
1821-1821: Asserting CKA_START_DATE/CKA_END_DATE retrieval on a private key — LGTMThe updated checkCommonKeyAttributes invocation validates both length and value for start/end dates on a private key, aligning with the PR goals.
Consider adding a companion scenario where the private key remains sensitive (CKA_SENSITIVE = CK_TRUE) to demonstrate that start/end dates are still retrievable even when other key material is not. This isolates the intended “public on private objects” behavior from sensitivity of key material.
I can provide a minimal test function (e.g., testStartEndDateOnSensitiveRSAPriv) that creates a sensitive private RSA key with dates set and asserts:
- CKA_START_DATE/CKA_END_DATE: CKR_OK with expected bytes
- CKA_PRIVATE_EXPONENT: CKR_ATTRIBUTE_SENSITIVE
Would you like me to draft it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/lib/P11Attributes.cpp(4 hunks)src/lib/P11Attributes.h(3 hunks)src/lib/test/ObjectTests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/P11Attributes.cpp
- src/lib/P11Attributes.h
🔇 Additional comments (1)
src/lib/test/ObjectTests.cpp (1)
1785-1786: CK_DATE test vectors for private RSA key look correctYear/month/day are set as ASCII digits per PKCS#11 and the struct layout matches CK_DATE. Good choice of distinct values to assert roundtrip.
|
@jschlyter, I've already pressed Thanks. |
nb-tech5
force-pushed
the
fix-fixes-reading-public-attribute-from-private-object
branch
from
54eee02 to
7ab2b97
Compare
|
Hello, |
nb-tech5
force-pushed
the
fix-fixes-reading-public-attribute-from-private-object
branch
from
7ab2b97 to
70c7e94
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/P11Attributes.h (1)
125-143: Virtual ByteString retrieval hook and non‑private base are well‑factoredIntroducing
retrieveAttrByteStringonP11Attributeand theP11NonPrivateAttributeoverride cleanly separates “decrypt if private” from “always return raw value”, which is exactly what’s needed for public attributes stored on private objects. The signatures and access levels look consistent with existing style, and the override correctly ignoresisPrivatewhile still going through the usual access checks inretrieve(). Only a tiny nit: the comment “Retrie the value if allowed” has a typo if you ever touch this again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/P11Attributes.cpp(4 hunks)src/lib/P11Attributes.h(3 hunks)src/lib/test/ObjectTests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/P11Attributes.cpp
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
📚 Learning: 2025-09-11T16:54:00.370Z
Learnt from: bukka
Repo: softhsm/SoftHSMv2 PR: 815
File: src/lib/SoftHSM.cpp:6543-6546
Timestamp: 2025-09-11T16:54:00.370Z
Learning: SoftHSM PR #815 (pkcs11.h 3.1.0 update, CK_RSA_AES_KEY_WRAP_PARAMS fix): Maintainer bukka prefers to keep scope minimal (header sync and param rename only); broader OAEP parameter enforcement or backend plumbing is out of scope for this PR and should be handled in a follow-up issue.
Applied to files:
src/lib/P11Attributes.h
🧬 Code graph analysis (1)
src/lib/P11Attributes.h (1)
src/lib/P11Attributes.cpp (8)
retrieveAttrByteString(72-87)retrieveAttrByteString(72-72)retrieveAttrByteString(503-507)retrieveAttrByteString(503-503)attr(2315-2315)attr(2414-2414)P11Attribute(43-49)P11Attribute(52-54)
🔇 Additional comments (3)
src/lib/test/ObjectTests.cpp (2)
1785-1796: Good coverage of non‑empty CK_DATE on private RSA keyDefining concrete
startDate/endDatevalues and wiring them into the private key template viaCKA_START_DATE/CKA_END_DATElooks correct and matchesCK_DATElayout. This should reliably exercise the new retrieval path on private key objects (and fail if the dates are incorrectly decrypted or treated as private).
1819-1822: checkCommonKeyAttributes invocation correctly validates stored datesPassing
startDate,sizeof(startDate)andendDate,sizeof(endDate)intocheckCommonKeyAttributesaligns with how that helper reads and compares the corresponding attributes. This will assert both length and content ofCKA_START_DATE/CKA_END_DATEon the private key, which is exactly what’s needed to guard the new retrieval behavior.src/lib/P11Attributes.h (1)
471-493: Start/end date attributes correctly switched to non‑private retrieval semanticsHaving
P11AttrStartDateandP11AttrEndDatederive fromP11NonPrivateAttributeand call its constructor ensures these date attributes are retrieved as plain ByteStrings even for private/secret key objects, while write behavior remains unchanged. This aligns with PKCS#11 semantics for CKA_START_DATE/CKA_END_DATE and matches the new tests that assert non‑empty dates on a private RSA key.
|
@antoinelochet, done. |
According to the PKCS#11 spec, all key objects have defined a semantic for CKA_START_DATE and CKA_END_DATE, although, for key objects that are private (e.g. CKO_PRIVATE_KEY, CKO_SECRET_KEY), the retrieval of their values fail. This is happening when an attribute with a specialized class, e.g. P11AttrStartDate, gets added to a private object, its value is always written in clear, due to the updateAttr method overload, although, upon retrieving the value, due to the retrieve method not being symmetrically overloaded, and because the object is private, the attribute value is decrypted and fails. This change adds protected virtual method `retrieveAttrByteString` to allow overloading the default behavior, namely for public attributes (written in clear), from private object. Also add a base class `P11NonPrivateAttribute` to be extended by public attributes specializations.
nb-tech5
force-pushed
the
fix-fixes-reading-public-attribute-from-private-object
branch
from
70c7e94 to
4bb88ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/test/ObjectTests.cpp`:
- Around line 1789-1797: The date-attribute regression test adds
CKA_START_DATE/CKA_END_DATE but doesn’t force the object to be a private object,
so it can pass due to token defaults; update the attribute template used in
ObjectTests.cpp (the attributes array that contains CKA_CLASS, CKA_KEY_TYPE,
CKA_SENSITIVE, etc.) to include CKA_PRIVATE set to CK_TRUE (use the existing
bTrue variable or define one if missing) so the template explicitly creates a
private object and the test exercises private-object retrieval behavior.
4 participants
According to the PKCS#11 spec, all key objects have defined a semantic for CKA_START_DATE and CKA_END_DATE, although, for key objects that are private (e.g. CKO_PRIVATE_KEY, CKO_SECRET_KEY), the retrieval of their values fail.
This is happening when an attribute with a specialized class, e.g. P11AttrStartDate, gets added to a private object, its value is always written in clear, due to the updateAttr method overload, although, upon retrieving the value, due to the retrieve method not being symmetrically overloaded, and because the object is private, the attribute value is decrypted and fails.
This change adds protected virtual method
retrieveAttrByteStringto allow overloading the default behavior, namely for public attributes (written in clear), from private object.Also add a base class
P11NonPrivateAttributeto be extended by public attributes specializationsFixes #804
Summary by CodeRabbit
New Features
Refactor
Tests