Skip to content

Bump libloading version to 0.8.6 #261

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

Merged
merged 1 commit into from
Apr 22, 2025
Merged

Conversation

Jakuje
Copy link
Contributor

@Jakuje Jakuje commented Apr 18, 2025

the libloading version 0.7.0 is a bit dated and makes downstream packaging (for example for Fedora) more complicated. I did not see any huge incompatible changes locally (but it might need some more love for windows -- hope the CI has a coverage).

@Jakuje
Copy link
Contributor Author

Jakuje commented Apr 18, 2025

The kryoptic CI failure is addressed with #258.

@Jakuje Jakuje marked this pull request as ready for review April 18, 2025 11:34
Copy link
Collaborator

@wiktor-k wiktor-k left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks!

@hug-dev
Copy link
Member

hug-dev commented Apr 22, 2025

Kryoptic is not happy 😿

@wiktor-k
Copy link
Collaborator

Kryoptic is not happy 😿

I think the test is overly tied to specific versions. Converting them to check if the version is greater than some number is better. The minor check can be dropped IMO.

As a compromise I think the version could be just printlnd so that we have something in logs (AFAICT Version impls Display) :)

@Jakuje
Copy link
Contributor Author

Jakuje commented Apr 22, 2025

Kryoptic is not happy 😿

As mentioned in the previous comment, the kryoptic failure is fixed in #258, which is now merged so merging this should be green (but I can rebase it here if you prefer).

@Jakuje
Copy link
Contributor Author

Jakuje commented Apr 22, 2025

I think the test is overly tied to specific versions. Converting them to check if the version is greater than some number is better. The minor check can be dropped IMO.

The version numbers should not change that fast with the pace Oasis works on them. So I would be for keeping them as they are now.

Actually, we have now the PKCS#11 3.2 in kryoptic now, while rust-cryptoki is still on 3.1 but with the backward compatibility, it should not matter functionally (just the this test complains).

@wiktor-k
Copy link
Collaborator

(but I can rebase it here if you prefer).

Please do. I work in security so I know "red means bad, green means good" 🫠 😉

Actually, we have now the PKCS#11 3.2 in kryoptic now, while rust-cryptoki is still on 3.1 but with the backward compatibility, it should not matter functionally (just the this test complains).

Ah, right, this is for PKCS#11 version 🤦‍♂️ still, changing the test when the backend returns higher numbers seems kind of off 🤔

@Jakuje
Copy link
Contributor Author

Jakuje commented Apr 22, 2025

(but I can rebase it here if you prefer).

Please do. I work in security so I know "red means bad, green means good" 🫠 😉

done

Actually, we have now the PKCS#11 3.2 in kryoptic now, while rust-cryptoki is still on 3.1 but with the backward compatibility, it should not matter functionally (just the this test complains).

Ah, right, this is for PKCS#11 version 🤦‍♂️ still, changing the test when the backend returns higher numbers seems kind of off 🤔

Actually, its a pkcs11 interface version. There is only 3.0 and 3.2 now.

I took this test failing as a good feedback that the version is properly propagated to the calling application (as for now it is the only indication as we do not have anything else to test) and that I do not mess up in the future for example by not enabling the (currently non-default) pkcs11_3_2 feature of kryoptic in Fedora build in the future.

Doing the comparison on greater_equal or something will match a lot of bad numbers, which is also something I would like to avoid. Just printing would make it really /dev/null as I do not think anyone reads the logs if they pass (they are hidden by the tooling).

So the only reasonable option I can find would be just to allow 3.0 and 3.2 values right now. This will also allow the tests to work for other people who are not on 3.2 yet. I will do it in separate PR as this is a bit off-topic here.

Jakuje added a commit to Jakuje/rust-cryptoki that referenced this pull request Apr 22, 2025
Resolves the discussion that started in the following PR:

parallaxsecond#261 (comment)

Signed-off-by: Jakub Jelen <[email protected]>
@Jakuje
Copy link
Contributor Author

Jakuje commented Apr 22, 2025

Filled #262.

@hug-dev hug-dev merged commit a1680c9 into parallaxsecond:main Apr 22, 2025
8 checks passed
Jakuje added a commit to Jakuje/rust-cryptoki that referenced this pull request Apr 22, 2025
Resolves the discussion that started in the following PR:

parallaxsecond#261 (comment)

Signed-off-by: Jakub Jelen <[email protected]>
Jakuje added a commit to Jakuje/rust-cryptoki that referenced this pull request Apr 22, 2025
Resolves the discussion that started in the following PR:

parallaxsecond#261 (comment)

Signed-off-by: Jakub Jelen <[email protected]>
Jakuje added a commit to Jakuje/rust-cryptoki that referenced this pull request Apr 22, 2025
Resolves the discussion that started in the following PR:

parallaxsecond#261 (comment)

For now, only PKCS#11 3.0 and 3.2 interface versions are defined so just
take these two instead of depending on one of them.

Signed-off-by: Jakub Jelen <[email protected]>
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.

3 participants