Skip to content

Comments

Do not fail fetches on unknown attributes#306

Merged
Jakuje merged 2 commits intolatchset:mainfrom
simo5:nss_fetchfail
Jul 29, 2025
Merged

Do not fail fetches on unknown attributes#306
Jakuje merged 2 commits intolatchset:mainfrom
simo5:nss_fetchfail

Conversation

@simo5
Copy link
Member

@simo5 simo5 commented Jul 29, 2025

Description

Just ignore the attributes we do not know anythign about.

This is causing a CKR_DEVICE_ERROR on unknown attributes (like CKA_PAREMETER_SET) instead of just returning they are unknown.

Checklist

  • Test suite updated with functionality tests
  • Test suite updated with negative tests
  • Rustdoc string were added or updated
  • CHANGELOG and/or other documentation added or 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
  • Doc string are properly updated

Just ignore the attributes we do not know anythign about.

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5 simo5 requested a review from Jakuje July 29, 2025 12:25
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.

code looks good .
For RPM build, we need to bump the toml version. Either bump to 0.9 or just relax the requirements to work with >0.8, <0.10 (preferred).,

@simo5
Copy link
Member Author

simo5 commented Jul 29, 2025

code looks good . For RPM build, we need to bump the toml version. Either bump to 0.9 or just relax the requirements to work with >0.8, <0.10 (preferred).,

Can't support both 0.8 and 0.9, as 0.9 breaks API, so I need to fix more things now ...

@Jakuje
Copy link
Contributor

Jakuje commented Jul 29, 2025

then in can be done in separate PR. This is good to go.

@simo5
Copy link
Member Author

simo5 commented Jul 29, 2025

it was just a matter of explicitly adding a new feature in Cargo.toml not a real API breakage, so I added it to this PR.

@Jakuje
Copy link
Contributor

Jakuje commented Jul 29, 2025

it was just a matter of explicitly adding a new feature in Cargo.toml not a real API breakage, so I added it to this PR.

I think it needs to come also here:

https://github.com/latchset/kryoptic/blob/main/.github/workflows/rpm.yml#L58

@Jakuje
Copy link
Contributor

Jakuje commented Jul 29, 2025

The other test looks like the flaky one

@simo5
Copy link
Member Author

simo5 commented Jul 29, 2025

@Jakuje Sounds like just upgrading toml does not resolve the RPM issue, so I will drop the commit and leave it to you to investigate

Signed-off-by: Simo Sorce <simo@redhat.com>
@simo5
Copy link
Member Author

simo5 commented Jul 29, 2025

Ah I saw the comment about the workfloow just now, lemme see if that fixes it

@Jakuje
Copy link
Contributor

Jakuje commented Jul 29, 2025

And it does. Merging. Thanks!

@Jakuje Jakuje merged commit e974e64 into latchset:main Jul 29, 2025
86 of 87 checks passed
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.

2 participants