Skip to content

Conversation

@simo5
Copy link
Member

@simo5 simo5 commented Mar 31, 2025

Description

The database optimized code was not sourcing the CKA_SENSITIVE/CKA_EXTRACTABLE attributes for token objects, this would cause subsequent checks on these attributes to use the object defaults rather than what was actually stored.
Session objects did not suffer from this issue because all attributes are always available for those.

Fixes: #193

Checklist

  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • [ ] Documentation was updated
  • [ ] This is not a code change

Reviewer's checklist:

  • Any issues marked for closing are fully addressed
  • There is a test suite reasonably covering new functionality or modifications
  • This feature/change has adequate documentation added
  • A changelog entry is added if the change is significant
  • Code conform to coding style that today cannot yet be enforced via the check style test
  • Commits have short titles and sensible text

simo5 added 2 commits March 31, 2025 14:11
This is needed to be able to make proper access control decisions in
upper layers when the object is stored in a token.

Signed-off-by: Simo Sorce <[email protected]>
@simo5 simo5 requested a review from Jakuje March 31, 2025 18:14
Copy link
Contributor

@Jakuje Jakuje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you were faster :)
lgtm. I just wanted to make sure we do not need some more in different code paths, but that can be done later if needed.

@simo5
Copy link
Member Author

simo5 commented Mar 31, 2025

you were faster :) lgtm. I just wanted to make sure we do not need some more in different code paths, but that can be done later if needed.

Reasonably confident this is all we need, but we can always add more patches later indeed.

Thanks for the review and all these bugs, each fix make kryoptic more robust and compliant, which I like.

@simo5 simo5 merged commit efc86bd into latchset:main Mar 31, 2025
23 of 24 checks passed
@Jakuje
Copy link
Contributor

Jakuje commented Mar 31, 2025

Thinking if this is the right place to add these though. In get_object_attrs() we are adding already different attributes for the non-logged in users. I think that would be probably better place ...

@simo5
Copy link
Member Author

simo5 commented Mar 31, 2025

Thinking if this is the right place to add these though. In get_object_attrs() we are adding already different attributes for the non-logged in users. I think that would be probably better place ...

No, this is the correct place, the one you see in get_objects_attr() is only for the case where a user is not logged in and mixing the use cases would be confusing.

@Jakuje
Copy link
Contributor

Jakuje commented Mar 31, 2025

Ok. Went through the places where else we fetch the data from db and this is the only one where we missed something, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Reading attributes from objects should pull more related attributes

2 participants