Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(ffi): PKCS7 C# bindings #255

Merged
merged 77 commits into from
Mar 22, 2024
Merged

feat(ffi): PKCS7 C# bindings #255

merged 77 commits into from
Mar 22, 2024

Conversation

irvingoujAtDevolution
Copy link
Contributor

@irvingoujAtDevolution irvingoujAtDevolution commented Mar 10, 2024 •
edited
Loading

Hello @CBenoit , this is a draft, not completely finished yet but it have the basic structure here. I would appreciate your feedback to make sure if I am on the right track!

Thanks 😄

CBenoit
CBenoit reviewed Mar 11, 2024
View reviewed changes
ffi/Cargo.toml Outdated Show resolved Hide resolved
CBenoit
CBenoit reviewed Mar 11, 2024
View reviewed changes
ffi/src/lib.rs Outdated Show resolved Hide resolved
CBenoit
CBenoit reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

Yes, I think you are on the right track! 👍

ffi/src/pkcs7.rs Outdated Show resolved Hide resolved
ffi/src/octet_string.rs Outdated Show resolved Hide resolved
ffi/src/x509.rs Outdated Show resolved Hide resolved
ffi/src/x509.rs Outdated Show resolved Hide resolved
ffi/src/x509.rs Outdated Show resolved Hide resolved
ffi/src/x509.rs Outdated Show resolved Hide resolved
@CBenoit CBenoit changed the title feat(ffi):pkcs7 bindings for C# feat(ffi): PKCS7 C# bindings Mar 12, 2024
@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Mar 13, 2024 •
edited
Loading

For some reason, I updated the branch, but the PR is not synchronized. the actual commit is here master...ARC-167-PKCS7-binding

Update: fixed it

CBenoit
CBenoit reviewed Mar 13, 2024
View reviewed changes
ffi/src/buffer.rs Outdated Show resolved Hide resolved
CBenoit
CBenoit reviewed Mar 13, 2024
View reviewed changes
ffi/src/buffer.rs Outdated Show resolved Hide resolved
CBenoit
CBenoit reviewed Mar 13, 2024
View reviewed changes
ffi/src/buffer.rs Outdated Show resolved Hide resolved
CBenoit
CBenoit reviewed Mar 13, 2024
View reviewed changes
ffi/src/buffer.rs Outdated Show resolved Hide resolved
CBenoit
CBenoit reviewed Mar 13, 2024
View reviewed changes
ffi/src/pkcs7.rs Outdated Show resolved Hide resolved
@CBenoit
Copy link
Member

CBenoit commented Mar 19, 2024

Can you add a few tests? Create a new Pkcs7Tests.cs file for that.
https://github.com/Devolutions/picky-rs/tree/da1f48ce8416655d9a7e6b84718eb79c5fc55120/ffi/dotnet/Devolutions.Picky.Tests
Poke me on Slack if you have trouble building and running the tests.

CBenoit
CBenoit reviewed Mar 21, 2024
View reviewed changes
ffi/src/utils.rs Outdated
Comment on lines 10 to 11
#[diplomat::opaque] // Named RsBuffer to avoid conflict with the Diplomat runtime Buffer
pub struct RsBuffer(pub Vec<u8>);
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: VecU8 (I know it’s not ideal either)

CBenoit
CBenoit reviewed Mar 21, 2024
View reviewed changes
ffi/src/utils.rs Outdated
Comment on lines 38 to 39
#[diplomat::opaque]
pub struct BufferIterator(pub Vec<Buffer>);
pub struct BufferIterator(pub Vec<RsBuffer>);
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: VecU8Iterator

@irvingoujAtDevolution
Copy link
Contributor Author

found out I missed a important part of what is exposed in Pkcs7, will push up a new commit today.

@CBenoit
Copy link
Member

CBenoit commented Mar 21, 2024

found out I missed a important part of what is exposed in Pkcs7, will push up a new commit today.

No problem! I’ll merge when I wake up tomorrow then!

@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Mar 21, 2024 •
edited
Loading

@CBenoit 😕 , I can't pass the locally created Pkcs7 test, the SelfSignedAuthenticodeSignatureBasicValidation one. but seems CI thinks it's ok, does CI test it at all?

Edit: I need a few tips on how to debug the DLL, eprintln! seems not printing anything at all.

CBenoit
CBenoit reviewed Mar 22, 2024
View reviewed changes
ffi/Cargo.toml Outdated Show resolved Hide resolved
@CBenoit
Copy link
Member

CBenoit commented Mar 22, 2024

but seems CI thinks it's ok, does CI test it at all?

CI is not running the C# tests at all for now. It required manual modification of the csproj file at some point. We should be able to add it to the CI soon though. But yeah, you have to ensure the tests are green manually for now.

@CBenoit
Copy link
Member

CBenoit commented Mar 22, 2024

Edit: I need a few tips on how to debug the DLL, eprintln! seems not printing anything at all.

Try to use println! instead, I think stderr is being eaten by the C# test harness.

@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Mar 22, 2024 •
edited
Loading

Edit: I need a few tips on how to debug the DLL, eprintln! seems not printing anything at all.

Try to use println! instead, I think stderr is being eaten by the C# test harness.

Thanks, I think I have fixed it

PS C:\Users\jou\code\picky-rs\ffi\dotnet\Devolutions.Picky.Tests> dotnet test --filter Devolutions.Picky.Tests.Pkcs7Tests.SelfSignedAuthenticodeSignatureBasicValidation
  Determining projects to restore...
  All projects are up-to-date for restore.
 .......
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.

Passed!  - Failed:     0, Passed:     1, Skipped:     0, Total:     1, Duration: < 1 ms - Devolutions.Picky.Tests.dll (net6.0)

Workload updates are available. Run `dotnet workload list` for more information.

@irvingoujAtDevolution
Copy link
Contributor Author

I think it is good to merge now @CBenoit

CBenoit
CBenoit approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@CBenoit CBenoit left a comment

Choose a reason for hiding this comment

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

LGTM! Good job!

@CBenoit CBenoit merged commit 7466973 into master Mar 22, 2024
10 checks passed
@CBenoit CBenoit deleted the ARC-167-PKCS7-binding branch March 22, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CBenoit CBenoit CBenoit approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@irvingoujAtDevolution @CBenoit