-
Notifications
You must be signed in to change notification settings - Fork 623
Add CRL creation to FFI #5166
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
base: master
Are you sure you want to change the base?
Add CRL creation to FFI #5166
Conversation
e3227a2 to
97f4590
Compare
97f4590 to
3f051b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds CRL (Certificate Revocation List) creation, updating, and viewing functionality to Botan's FFI, enhancing the library's X.509 certificate management capabilities. The changes include new FFI functions for CRL operations, corresponding Python bindings, comprehensive tests, and documentation.
Key Changes
- Added FFI functions for CRL creation (
botan_x509_crl_create), updating (botan_x509_crl_update), and inspection - Implemented Python wrapper classes (
X509CRL,X509CRLReason,X509CRLEntry) with methods for CRL operations - Introduced FFI API version 20260203 for Botan 3.11.0
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/ffi/ffi_cert.h |
New internal header declaring FFI certificate/CRL structure types |
src/lib/ffi/ffi_cert.cpp |
Implementation of CRL creation, update, viewing, and inspection functions |
src/lib/ffi/ffi.h |
Public FFI API declarations for new CRL functions and reason code enum |
src/lib/ffi/ffi.cpp |
FFI version support registration for API 20260203 |
src/lib/ffi/info.txt |
Updated FFI version constant and added internal header |
src/tests/test_ffi.cpp |
C++ tests for CRL creation, updating, and verification |
src/tests/data/x509/crl/ca.crt |
Test CA certificate for CRL testing |
src/tests/data/x509/crl/sub1.crt |
Test subordinate certificate 1 for CRL testing |
src/tests/data/x509/crl/sub2.crt |
Test subordinate certificate 2 for CRL testing |
src/python/botan3.py |
Python bindings with X509CRL class, reason enum, and entry class |
src/scripts/test_python.py |
Python tests for CRL creation, revocation, and verification |
doc/api_ref/ffi.rst |
Documentation for new CRL-related FFI functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
22c282f to
7f9f2a1
Compare
7f9f2a1 to
b26c0d5
Compare
|
@randombit I have removed the dependency on the python doc, so this is now a standalone PR. I hope this makes it a bit easier to take a look here. |
randombit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the rebase that helps a lot. Generally looks fine, I think though for the CRL entry getters we'll go with the approach added in #5220. It's probably easiest to just rebase this PR onto #5220 (or alternately wait for #5220 to merge then rebase onto master, which is a bit slower but likely to be less aggravating since there is a lot of FFI work going in at once for 3.11 it seems)
|
|
||
| #include <botan/ffi.h> | ||
|
|
||
| #include <botan/internal/ffi_cert.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be making FFI hard-depend on x509, which is probably fine tbh, but should be codified by updated the dependencies in info.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This I don't quite understand, all the methods, including ffi_cert.h have #if defined(BOTAN_HAS_X509_CERTIFICATES) guards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frankly, I didn't realize that this PR had so much overlap with our work on #5220 and #5188. I'm sorry to barge in like that after it's been open for like six weeks. 😨
Anyway, I think we should ensure that the APIs for reading and writing CRLs are aligning nicely. I added some suggestions below.
Please note that despite having merged #5220 already, I don't consider those changes to be set in stone before they make it into a release. I'm definitely open to re-evaluate them as needed.
6d666d2 to
4098d59
Compare
5ef7637 to
a34fc29
Compare
| def to_pem(self) -> str: | ||
| # todo! | ||
| return _call_fn_viewing_str(lambda vc, vfn: _DLL.botan_x509_crl_view_pem(self.__obj, vc, vfn)) | ||
|
|
||
| def to_der(self) -> bytes: | ||
| # todo! | ||
| return _call_fn_viewing_vec(lambda vc, vfn: _DLL.botan_x509_crl_view_der(self.__obj, vc, vfn)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adds methods to create, update and view CRLs to the FFI.
CRL part of #4877.