feat: Added 2 methods for listing crypto algorithms#3729
feat: Added 2 methods for listing crypto algorithms#3729jirihnidek wants to merge 4 commits intomainfrom
Conversation
Reviewer's GuideAdds two new C-extension methods to the rhsm._certificate module to enumerate OpenSSL-provided public key and signature algorithms (as OIDs), wires them into module initialization, and introduces Python type stubs and unit tests validating their behavior. Updated class diagram for rhsm._certificate C-extension stubsclassDiagram
class module_rhsm__certificate {
+get_public_key_algorithms(*args, **kwargs) list
+get_signature_algorithms(*args, **kwargs) list
+load(*args, **kwargs) None
+load_private_key(*args, **kwargs) None
}
class X509 {
}
class PrivateKey {
}
class OpenSSLCertificateLoadingError {
}
module_rhsm__certificate ..> X509 : returns
module_rhsm__certificate ..> PrivateKey : returns
module_rhsm__certificate ..> OpenSSLCertificateLoadingError : raises
Flow diagram for get_public_key_algorithms implementationflowchart TD
A[Python_calls_get_public_key_algorithms] --> B[PyList_New_empty_list]
B --> C[sk_EVP_KEYMGMT_new_with_key_manager_cmp]
C --> D[EVP_KEYMGMT_do_all_provided_with_app_libctx]
D --> E[collect_key_managers_push_into_km_stack]
E --> F[sk_EVP_KEYMGMT_sort]
F --> G[for_each_EVP_KEYMGMT_in_km_stack]
G --> H[sk_OPENSSL_CSTRING_new_with_name_cmp]
H --> I[EVP_KEYMGMT_names_do_all_collect_names]
I --> J[get_algorithm_oid_from_names]
J --> K{oid_is_not_NULL}
K -- yes --> L[PyString_FromString_oid]
L --> M[PyList_Append_to_result_list]
M --> N[Py_DECREF_oid_str]
K -- no --> O[skip_append_for_this_keymgmt]
N --> P[sk_OPENSSL_CSTRING_free_names]
O --> P
P --> Q{more_keymgmt_in_stack}
Q -- yes --> G
Q -- no --> R[sk_EVP_KEYMGMT_pop_free_km_stack]
R --> S[return_PyObject_list_to_Python]
Flow diagram for get_signature_algorithms implementationflowchart TD
A[Python_calls_get_signature_algorithms] --> B[PyList_New_empty_list]
B --> C[sk_EVP_SIGNATURE_new_with_signature_cmp]
C --> D[EVP_SIGNATURE_do_all_provided_with_app_libctx]
D --> E[collect_signatures_push_into_sig_stack]
E --> F[sk_EVP_SIGNATURE_sort]
F --> G[for_each_EVP_SIGNATURE_in_sig_stack]
G --> H[sk_OPENSSL_CSTRING_new_with_name_cmp]
H --> I[EVP_SIGNATURE_names_do_all_collect_names]
I --> J[get_algorithm_oid_from_names]
J --> K{oid_is_not_NULL}
K -- yes --> L[PyString_FromString_oid]
L --> M[PyList_Append_to_result_list]
M --> N[Py_DECREF_oid_str]
K -- no --> O[skip_append_for_this_signature]
N --> P[sk_OPENSSL_CSTRING_free_names]
O --> P
P --> Q{more_signature_in_stack}
Q -- yes --> G
Q -- no --> R[sk_EVP_SIGNATURE_pop_free_sig_stack]
R --> S[return_PyObject_list_to_Python]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
0221328 to
6edead3
Compare
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
get_public_key_algorithmsandget_signature_algorithms,PyList_New(0)and the stack allocations are not checked for NULL before use, so you may want to add error-handling and early returns to avoid dereferencing NULL on allocation failures. - The
get_algorithm_oidhelper silently returns NULL when there is only a single name; this causes algorithms with a single OpenSSL name to be dropped entirely, so consider returning that name (or documenting why they should be ignored) instead of skipping them. - Both new Python-exposed functions are declared as
METH_VARARGS | METH_KEYWORDSbut ignore their arguments; if they are not intended to take any parameters, consider switching toMETH_NOARGSor validating that no arguments are provided to avoid confusing API misuse.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `get_public_key_algorithms` and `get_signature_algorithms`, `PyList_New(0)` and the stack allocations are not checked for NULL before use, so you may want to add error-handling and early returns to avoid dereferencing NULL on allocation failures.
- The `get_algorithm_oid` helper silently returns NULL when there is only a single name; this causes algorithms with a single OpenSSL name to be dropped entirely, so consider returning that name (or documenting why they should be ignored) instead of skipping them.
- Both new Python-exposed functions are declared as `METH_VARARGS | METH_KEYWORDS` but ignore their arguments; if they are not intended to take any parameters, consider switching to `METH_NOARGS` or validating that no arguments are provided to avoid confusing API misuse.
## Individual Comments
### Comment 1
<location path="src/certificate.c" line_range="431-433" />
<code_context>
+static PyObject *
+get_public_key_algorithms (PyObject *self, PyObject *args, PyObject *keywords)
+{
+ PyObject *list = PyList_New(0);
+
+ STACK_OF(EVP_KEYMGMT) *km_stack = sk_EVP_KEYMGMT_new(key_manager_cmp);
+
+ EVP_KEYMGMT_do_all_provided(app_get0_libctx(), collect_key_managers, km_stack);
</code_context>
<issue_to_address>
**issue (bug_risk):** Missing allocation failure checks for `PyList_New` and `sk_EVP_KEYMGMT_new`.
Both calls can return `NULL`, and the current code dereferences them unconditionally. Please add checks for `PyList_New` and `sk_EVP_KEYMGMT_new`, set an appropriate Python exception and clean up on failure (e.g., `Py_DECREF(list)` if `km_stack` allocation fails after `list` succeeds), and return `NULL` from the function in those cases.
</issue_to_address>
### Comment 2
<location path="src/certificate.c" line_range="447-448" />
<code_context>
+ const char *oid = get_algorithm_oid(names);
+
+ if (oid != NULL) {
+ PyObject *oid_str = PyString_FromString(oid);
+ PyList_Append(list, oid_str);
+ Py_DECREF(oid_str);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** Errors from `PyString_FromString` / `PyList_Append` are ignored, potentially leaving an inconsistent list and leaked resources.
Both calls can fail and set a Python exception. Please check the return values of `PyString_FromString` and `PyList_Append`; on failure, clean up (free the stacks, `Py_DECREF(list)`), and return `NULL` so the existing exception is propagated instead of returning a partially-built list and leaking references.
</issue_to_address>
### Comment 3
<location path="src/certificate.c" line_range="388-392" />
<code_context>
+ return OPENSSL_strcasecmp(*a, *b);
+}
+
+void collect_names(const char *name, void *vdata)
+{
+ STACK_OF(OPENSSL_CSTRING) *names = vdata;
+
+ /* A failure to push cannot be handled, so we ignore the result. */
+ (void)sk_OPENSSL_CSTRING_push(names, name);
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Ignoring `sk_OPENSSL_CSTRING_push` failures can silently truncate the results.
Because the callback can’t return an error, ignoring `sk_OPENSSL_CSTRING_push` failures can drop names under memory pressure. Consider storing both the stack and an error flag in `vdata`, setting the flag on push failure, and checking it after iteration to raise an error in Python instead of returning an incomplete list.
Suggested implementation:
```c
typedef struct name_collect_context_st {
STACK_OF(OPENSSL_CSTRING) *names;
int error;
} NAME_COLLECT_CONTEXT;
void collect_names(const char *name, void *vdata)
{
NAME_COLLECT_CONTEXT *ctx = vdata;
if (!sk_OPENSSL_CSTRING_push(ctx->names, name))
ctx->error = 1;
}
```
To fully implement the behavior described in your review comment, the following additional changes are needed elsewhere in the file (or in the corresponding caller code):
1. Wherever `collect_names` is used as a callback, change the `void *` argument passed to the OpenSSL iterator from a bare `STACK_OF(OPENSSL_CSTRING) *` to a `NAME_COLLECT_CONTEXT` instance, e.g.:
- Declare and initialize:
- `NAME_COLLECT_CONTEXT ctx;`
- `ctx.names = names;` (where `names` is the existing `STACK_OF(OPENSSL_CSTRING) *`)
- `ctx.error = 0;`
- Pass `&ctx` as the `void *` argument to the OpenSSL iteration function instead of `names`.
2. After the iteration completes, check `ctx.error`:
- If `ctx.error != 0`, treat this as an out-of-memory / push-failure condition.
- Convert this into a Python-level error (e.g., set an exception and return `NULL` or an error code), **instead of** returning the partially-filled name list.
3. Ensure any code that previously assumed `vdata` was a `STACK_OF(OPENSSL_CSTRING) *` is updated to treat it as a `NAME_COLLECT_CONTEXT *` consistently.
</issue_to_address>
### Comment 4
<location path="src/rhsm/_certificate.pyi" line_range="11-20" />
<code_context>
+class PrivateKey: ...
+class OpenSSLCertificateLoadingError: ...
+
+def get_public_key_algorithms(*args, **kwargs) -> list:
+ """return a list of public key algorithms"""
+
+def get_signature_algorithms(*args, **kwargs) -> list:
+ """return a list of signature algorithms"""
+
+def load(*args, **kwargs) -> None:
+ """load a certificate from a file"""
+
+def load_private_key(*args, **kwargs) -> None:
+ """load a private key from a file"""
</code_context>
<issue_to_address>
**suggestion:** Type hints in the stub file are too generic or inaccurate compared to the C API.
`get_public_key_algorithms` and `get_signature_algorithms` return Python lists of strings, so these should be typed as `list[str]` rather than bare `list`. Likewise, `load` and `load_private_key` appear to return `X509`/`PrivateKey` (or similar) instead of `None`. Using these concrete return types will improve IDE/type-checker support and reduce misuse of the API.
Suggested implementation:
```
def get_public_key_algorithms(*args, **kwargs) -> list[str]:
"""return a list of public key algorithms"""
```
```
def get_signature_algorithms(*args, **kwargs) -> list[str]:
"""return a list of signature algorithms"""
```
```
def load(*args, **kwargs) -> X509:
"""load a certificate from a file"""
```
```
def load_private_key(*args, **kwargs) -> PrivateKey:
"""load a private key from a file"""
```
</issue_to_address>
### Comment 5
<location path="test/test_certificate_so.py" line_range="41-47" />
<code_context>
+ pub_key_algos = rhsm._certificate.get_public_key_algorithms()
+ self.helper_test_list_of_algorithms(pub_key_algos)
+
+ def test_get_signature_algorithms(self):
+ """
+ Test that the public key algorithms are returned.
+ The list should contain OIDs of supported algorithms.
+ The list of OIDs could be different on different systems.
+ """
+ signature_algos = rhsm._certificate.get_signature_algorithms()
+ self.helper_test_list_of_algorithms(signature_algos)
</code_context>
<issue_to_address>
**nitpick (typo):** Docstring for `test_get_signature_algorithms` mentions public key algorithms instead of signature algorithms
The current docstring is misleading; it should explicitly say it tests signature algorithms, not public key algorithms, so future readers can clearly distinguish this test from the similar public-key test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def test_get_signature_algorithms(self): | ||
| """ | ||
| Test that the public key algorithms are returned. | ||
| The list should contain OIDs of supported algorithms. | ||
| The list of OIDs could be different on different systems. | ||
| """ | ||
| signature_algos = rhsm._certificate.get_signature_algorithms() |
There was a problem hiding this comment.
nitpick (typo): Docstring for test_get_signature_algorithms mentions public key algorithms instead of signature algorithms
The current docstring is misleading; it should explicitly say it tests signature algorithms, not public key algorithms, so future readers can clearly distinguish this test from the similar public-key test.
6edead3 to
f04bea8
Compare
DuckBoss
left a comment
There was a problem hiding this comment.
Thanks for your PR, I did a basic review and checked unit tests and I have some small requests.
| {"get_public_key_algorithms", (PyCFunction) get_public_key_algorithms, | ||
| METH_VARARGS | METH_KEYWORDS, "return a list of public key algorithms"}, | ||
| {"get_signature_algorithms", (PyCFunction) get_signature_algorithms, | ||
| METH_VARARGS | METH_KEYWORDS, "return a list of signature algorithms"}, |
There was a problem hiding this comment.
What's the purpose of METH_VARARGS | METH_KEYWORDS? These functions don't use or validate any of the args/kwargs that are passed to them. Can this be omitted?
There was a problem hiding this comment.
Well, I was just following existing pattern. You can call these Python methods with arbitrary number of arguments and key arguments and these arguments will be ignored. It gives us freedom to use some argument or key arguments without breaking backward compatibility.
Thus, having this has no risk and it could have some benefits in the future.
* Card ID: CCT-2233 * Added two new methods to _certificate Python module written in C. The first method allows to list public key algorithms and signatures algorithms Signed-off-by: Jiri Hnidek <jhnidek@redhat.com>
* Card ID: CCT-2233 * Introduced a new test_certificate_so.py for unit testing of methods and classes from certificate.c. I was surprised that there was not any unit test for such module. Signed-off-by: Jiri Hnidek <jhnidek@redhat.com>
* Some file with .py* suffix should not be ignored. For example *.pyi containing stubs/interfaces are valid files that you want to have in repository Signed-off-by: Jiri Hnidek <jhnidek@redhat.com>
* Card ID: CCT-2233 * Added src/rhsm/_certificate.pyi interface file for Python module that is written in C language (certificate.c) * This help could help IDEs with type-hinting Signed-off-by: Jiri Hnidek <jhnidek@redhat.com>
f04bea8 to
018bcce
Compare
| #include <openssl/provider.h> | ||
| #include <openssl/bio.h> | ||
| #include <openssl/ssl.h> |
There was a problem hiding this comment.
The only remaining question I have is about these new imports:
- Is an explicit
bio.hinclude required if it's included already viaopenssl/pem.h? - What is this
ssl.hinclude providing for this file? I don't see anything being used from this header. If it's being used for the methods from the crypto header, can we just include thecrypto.hheader directly?
Uh oh!
There was an error while loading. Please reload this page.