Add certificate mapping library #192
Conversation
|
There are few missing files:
|
|
Which library/project will be an external consument of |
sumit-bose
force-pushed
the
certificate_mapping
branch
from
e122330
to
623dcd7
Compare
|
oops, I'm sorry, I'm really sure I did run 'make distcheck' before sending the PR. The library be used by IPA freeipa/freeipa#575. |
|
There is a valgring error in unit test.
```
==11568== Invalid read of size 1
==11568== at 0x4C32CC4: strcmp (vg_replace_strmem.c:842)
==11568== by 0x50412EA: _assert_string_equal (in /usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x401F3E: test_sss_cert_get_content_2 (test_certmap.c:815)
==11568== by 0x5041968: ??? (in /usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x5042250: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x401631: main (test_certmap.c:1305)
==11568== Address 0xb9deaee is 0 bytes after a block of size 110 alloc'd
==11568== at 0x4C2EB1B: malloc (vg_replace_malloc.c:299)
==11568== by 0x5AFF26E: _talloc_memdup (in /usr/lib64/libtalloc.so.2.1.9)
==11568== by 0x5D1380F: add_string_other_name_to_san_list (sss_cert_content_nss.c:434)
==11568== by 0x5D1380F: get_san (sss_cert_content_nss.c:790)
==11568== by 0x5D142AC: sss_cert_get_content (sss_cert_content_nss.c:986)
==11568== by 0x401B0A: test_sss_cert_get_content_2 (test_certmap.c:758)
==11568== by 0x5041968: ??? (in /usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x5042250: _cmocka_run_group_tests (in /usr/lib64/libcmocka.so.0.4.0)
==11568== by 0x401631: main (test_certmap.c:1305)
==11568==
```
I think taht problem is in uni test; because it tries to compare
`item->bin_val` with a string. But IIUC `bin_val` is not null terminated.
We should use `assert_memory_equal`
+ there are few warnins reported by static analyzers
```
Error: NULL_RETURNS (CWE-476): [#def1]
sssd-1.15.2/src/db/sysdb_certmap.c:149: var_assigned: Assigning: "el->values" = null return value from "_talloc_zero_array".
sssd-1.15.2/src/db/sysdb_certmap.c:152: dereference: Dereferencing a null pointer "el->values".
# 150|
# 151| for (c = 0; certmap->domains[c] != NULL; c++) {
# 152|-> el->values[c].data = (uint8_t *) talloc_strdup(el->values,
# 153| certmap->domains[c]);
# 154| if (el->values[c].data == NULL) {
Error: NULL_RETURNS (CWE-476): [#def4]
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:892: var_assigned: Assigning: "current" = null return value from "CERT_GetNextGeneralName".
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:788: dereference: Dereferencing a null pointer "current".
# 786| current = name_list;
# 787| do {
# 788|-> switch (current->type) {
# 789| case certOtherName:
# 790| ret = add_string_other_name_to_san_list(mem_ctx,
Error: PW.SET_BUT_NOT_USED: [#def5]
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:923: set_but_not_used: variable "parameters" was set but never used
# 921| SECItem der_item;
# 922| NSSInitContext *nss_ctx;
# 923|-> NSSInitParameters parameters = { 0 };
# 924| parameters.length = sizeof (parameters);
# 925|
Error: CHECKED_RETURN (CWE-252): [#def6]
sssd-1.15.2/src/lib/certmap/sss_cert_content_nss.c:1003: check_return: Calling "NSS_ShutdownContext" without checking return value (as is done elsewhere 2 out of 3 times).
sssd-1.15.2/src/p11_child/p11_child_nss.c:486: example_assign: Example 1: Assigning: "rv" = return value from "NSS_ShutdownContext(nss_ctx)".
sssd-1.15.2/src/p11_child/p11_child_nss.c:487: example_checked: Example 1 (cont.): "rv" has its value checked in "rv != SECSuccess".
sssd-1.15.2/src/util/cert/nss/cert.c:404: example_assign: Example 2: Assigning: "rv" = return value from "NSS_ShutdownContext(nss_ctx)".
sssd-1.15.2/src/util/cert/nss/cert.c:405: example_checked: Example 2 (cont.): "rv" has its value checked in "rv != SECSuccess".
# 1001|
# 1002| CERT_DestroyCertificate(cert);
# 1003|-> NSS_ShutdownContext(nss_ctx);
# 1004|
# 1005| if (ret == EOK) {
Error: CLANG_WARNING: [#def11]
sssd-1.15.2/src/lib/certmap/sss_certmap.c:586:9: warning: Function call argument is an uninitialized value
# talloc_free(exp);
# ^
/usr/include/talloc.h:228:26: note: expanded from macro 'talloc_free'
##define talloc_free(ctx) _talloc_free(ctx, __location__)
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:9: note: Assuming '_filter' is not equal to null
# if (_filter == NULL || _domains == NULL) {
# ^~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:9: note: Left side of '||' is false
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:28: note: Assuming '_domains' is not equal to null
# if (_filter == NULL || _domains == NULL) {
# ^~~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:906:5: note: Taking false branch
# if (_filter == NULL || _domains == NULL) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:911:9: note: Assuming 'ret' is equal to 0
# if (ret != 0) {
# ^~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:911:5: note: Taking false branch
# if (ret != 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true. Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is equal to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is false. Execution continues on line 916
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true. Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is equal to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is false. Execution continues on line 916
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true. Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is equal to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is false. Execution continues on line 916
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:30: note: Assuming 'p' is not equal to null
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:916:5: note: Loop condition is true. Entering loop body
# for (p = ctx->prio_list; p != NULL; p = p->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:32: note: Assuming 'r' is not equal to null
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:917:9: note: Loop condition is true. Entering loop body
# for (r = p->rule_list; r != NULL; r = r->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:919:17: note: Assuming 'ret' is equal to 0
# if (ret == 0) {
# ^~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:919:13: note: Taking true branch
# if (ret == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:921:23: note: Calling 'get_filter'
# ret = get_filter(ctx, r->parsed_mapping_rule, cert_content,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:603:9: note: Assuming 'result' is not equal to null
# if (result == NULL) {
# ^~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:603:5: note: Taking false branch
# if (result == NULL) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:607:44: note: Assuming 'comp' is not equal to null
# for (comp = parsed_mapping_rule->list; comp != NULL; comp = comp->next) {
# ^~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:607:5: note: Loop condition is true. Entering loop body
# for (comp = parsed_mapping_rule->list; comp != NULL; comp = comp->next) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:608:9: note: Taking false branch
# if (comp->type == comp_string) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:610:16: note: Taking true branch
# } else if (comp->type == comp_template) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:611:19: note: Calling 'expand_template'
# ret = expand_template(ctx, comp->parsed_template, cert_content,
# ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:553:5: note: 'exp' declared without an initial value
# char *exp;
# ^~~~~~~~~
sssd-1.15.2/src/lib/certmap/sss_certmap.c:555:5: note: Taking false branch
# if (strcmp("issuer_dn", parsed_template->name) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:558:12: note: Taking false branch
# } else if (strcmp("subject_dn", parsed_template->name) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:561:12: note: Taking false branch
# } else if (strncmp("subject_", parsed_template->name, 8) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:563:12: note: Taking false branch
# } else if (strcmp("cert", parsed_template->name) == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:568:9: note: Control jumps to line 583
# goto done;
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:583:5: note: Taking false branch
# if (ret == 0) {
# ^
sssd-1.15.2/src/lib/certmap/sss_certmap.c:586:9: note: Function call argument is an uninitialized value
# talloc_free(exp);
# ^ ~~~
/usr/include/talloc.h:228:26: note: expanded from macro 'talloc_free'
##define talloc_free(ctx) _talloc_free(ctx, __location__)
# ^ ~~~
# 584| *expanded = exp;
# 585| } else {
# 586|-> talloc_free(exp);
# 587| }
# 588|
```
And I am also not sure about few regex related warnings
```
Error: FORWARD_NULL (CWE-476): [#def12]
sssd-1.15.2/src/lib/certmap/sss_certmap.c:661: var_deref_model: Passing "NULL" to "regexec", which dereferences it.
# 659| return false;
# 660| }
# 661|-> match = (regexec(®exp, tmp_str, 0, NULL, 0) == 0);
# 662| talloc_free(tmp_str);
# 663| } else {
Error: FORWARD_NULL (CWE-476): [#def13]
sssd-1.15.2/src/lib/certmap/sss_certmap.c:665: var_deref_model: Passing "NULL" to "regexec", which dereferences it.
# 663| } else {
# 664| match = (item->val != NULL
# 665|-> && regexec(®exp, item->val, 0, NULL, 0) == 0);
# 666| }
# 667| if (!match) {
```
man 3 `regex` says:
```
REG_NOSUB
Do not report position of matches. The nmatch and pmatch argu‐
ments to regexec() are ignored if the pattern buffer supplied
was compiled with this flag set.
```
But I think we use `regcomp` only in file `src/lib/certmap/sss_certmap_krb5_match.c`
without cflag `REG_NOSUB` and we do not allow pass regex as an input in API.
I think we should be safe.
LS
|
sumit-bose
force-pushed
the
certificate_mapping
branch
from
623dcd7
to
4a27b6e
Compare
|
Hi, Lukas, thank you for the review. I fixed the invalid read the two NULL RETURNS, the PW.SET_BUT_NOT_USED and the CLANG WARNING. It would be possible to save the result in a variable for the CHECKED_RETURN
but there is nothing which can be done with the variable so it would just produce a different warning, so I would suggest to ignore it. Same with the regexec warning, add an otherwise unused variable would just cause a different warning. |
|
retest this please |
1 similar comment
|
retest this please |
sumit-bose
force-pushed
the
certificate_mapping
branch
from
4a27b6e
to
6badb93
Compare
| DEBUG(SSSDBG_CRIT_FAILURE, | ||
| "Failed to read user name hint option, skipping.\n"); | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Is it expected that we continue on both success and failure here?
There was a problem hiding this comment.
Yes, I think the log messages is sufficient here and the issue is not important enough to cause an error which would terminate the whole request.
| } | ||
|
|
||
| ret = sysdb_attrs_get_uint32_t(reply[c], IPA_CERTMAP_PRIORITY, | ||
| &m->priority); |
There was a problem hiding this comment.
Just checking here -- won't reading a nonexistent attribute set some priority we don't want (iow, is the default value, I guess zero, what we expect if the priority is not there?
There was a problem hiding this comment.
good catch, fixed
| goto done; | ||
| } | ||
|
|
||
| sss_certmap_free_ctx(sdap_opts->certmap_ctx); |
There was a problem hiding this comment.
Is this expected that we first free the context and then steal it?
There was a problem hiding this comment.
yes, this updates the context stored in sdap_opts by first freeing any existing one and then adding the new one.
src/providers/ipa/ipa_subdomains.c
Outdated
| return NULL; | ||
| } | ||
|
|
||
| if (sd_ctx->ranges_search_bases == NULL) { |
There was a problem hiding this comment.
Is it needed to check the ranges_search_bases here?
src/providers/ipa/ipa_subdomains.c
Outdated
| ret = ipa_subdomains_certmap_recv(subreq); | ||
| talloc_zfree(subreq); | ||
| if (ret != EOK) { | ||
| DEBUG(SSSDBG_CRIT_FAILURE, "Unable to get IPA ranges " |
src/providers/ipa/ipa_hbac_private.h
Outdated
| @@ -23,6 +23,7 @@ | |||
| #ifndef IPA_HBAC_PRIVATE_H_ | |||
| #define IPA_HBAC_PRIVATE_H_ | |||
|
|
|||
| #include "providers/ipa/ipa_config.h" | |||
There was a problem hiding this comment.
Is it needed to touch the HBAC header in this patch?
src/db/sysdb_certmap.c
Outdated
| } | ||
|
|
||
| static errno_t sysdb_certmap_add(struct sysdb_ctx *sysdb, | ||
| struct certmap_info *certmap) |
There was a problem hiding this comment.
If you end up changing this patch, please check the alignment here
src/db/sysdb_certmap.c
Outdated
|
|
||
| done: | ||
| if (ret) { | ||
| DEBUG(SSSDBG_TRACE_FUNC, "Error: %d (%s)\n", ret, strerror(ret)); |
There was a problem hiding this comment.
Please use sss_strerror in new code
Makefile.am
Outdated
| $(NULL) | ||
| libsss_certmap_la_LDFLAGS = \ | ||
| -Wl,--version-script,$(srcdir)/src/lib/certmap/sss_certmap.exports \ | ||
| -version-info 0:0:0 |
There was a problem hiding this comment.
Is this a valid version or a placeholder until the API is stable? Would it make sense to add a ticket to increase the version info if it's supposed to be bumped later?
There was a problem hiding this comment.
I think 0:0:0 is ok for an initial version of a library. After we released it with a new version of SSSD we can follow the libtool version update rules when we add changes.
src/lib/certmap/sss_certmap.h
Outdated
| /** | ||
| * Typedef for external debug callback | ||
| */ | ||
| typedef void (sss_certmap_ext_debug)(void *private, |
There was a problem hiding this comment.
This is a nitpick, but I would prefer a different argument name (pvt?) to avoid using a C++ keyword, because now my editor highlights the private word :)
| * - ENOENT: certificate does not match | ||
| * - EINVAL: internal error | ||
| */ | ||
| int sss_certmap_get_search_filter(struct sss_certmap_ctx *ctx, |
There was a problem hiding this comment.
Is it expected that this call doesn't accept any talloc context and all the resulting filters are allocated atop the sss_certmap_ctx? I'm looking at get_filter for example. Is the expectation that the caller must free the filter by e.g. stealing it onto some private context?
There was a problem hiding this comment.
I'm sorry, I should have read the description of ss_certmap_free_filter_and_domains first :)
There was a problem hiding this comment.
Yes, I wanted to use talloc internally but thought the options we have in other libraries to allow to use different allocators make things just more complicates without a real benefit.
To free the data the caller should use sss_certmap_free_filter_and_domains(). I added a comment about it to the doctext for sss_certmap_get_search_filter.
|
On (21/03/17 08:59), Jakub Hrozek wrote:
jhrozek commented on this pull request.
> + src/util/crypto/nss/nss_base64.c \
+ src/util/cert/nss/cert.c \
+ src/util/crypto/nss/nss_util.c \
+ $(NULL)
+libsss_certmap_la_CFLAGS = \
+ $(AM_CFLAGS) \
+ $(TALLOC_CFLAGS) \
+ $(NSS_CFLAGS) \
+ $(NULL)
+libsss_certmap_la_LIBADD = \
+ $(TALLOC_LIBS) \
+ $(NSS_LIBS) \
+ $(NULL)
+libsss_certmap_la_LDFLAGS = \
+ -Wl,--version-script,$(srcdir)/src/lib/certmap/sss_certmap.exports \
+ -version-info 0:0:0
Is this a valid version or a placeholder until the API is stable? Would it make sense to add a ticket to increase the version info if it's supposed to be bumped later?
I think it is valid. And many libraries has version 0.0.0
check yourself :-)
ls -l /usr/lib64/lib*.0.0.0
LS
|
| * @param[in] domains string array of domains returned by | ||
| * sss_certmap_get_search_filter | ||
| */ | ||
| void sss_certmap_free_filter_and_domains(char *filter, char **domains); |
There was a problem hiding this comment.
I don't see this function being called anywhere..
There was a problem hiding this comment.
oops, in the test it is not needed because I free the complete context there. But I added it to sss_cert_derb64_to_ldap_filter() where is was missing.
|
I won't pretend I read the certmap library code carefully, I more or less checked the API, ran the tests and scrolled through the library code. My only concern there is the memory allocations so that we don't grow the library memory context. From functional side, the patches break D-Bus lookups by certificate for me. I didn't see any other issues against a (legacy, considering this functionality) IPA 4.4 server. I don't have any other comments except those in this review and the D-Bus lookups. |
sumit-bose
force-pushed
the
certificate_mapping
branch
from
6badb93
to
292f5f2
Compare
|
Thank you for the review, the new versions also contains a fix for the default matching rule which we discussed on irc. |
To be able to include split_on_separator() without additional dependencies (only talloc), it is moved into a separate file. Related to https://pagure.io/SSSD/sssd/issue/3050
To be able to include string_in_list() without additional dependencies it is moved into a separate file. Related to https://pagure.io/SSSD/sssd/issue/3050
With this library it would be possible to map certificates and users not only by adding the full certificate to the user's LDAP object but by adding e.g. only parts like the issuer and subject name. Additionally the library is also able to flexible select/match certificates based on values in the certificate. Details about mapping and matching rules can be found in the included man page. Related to https://pagure.io/SSSD/sssd/issue/3050
sumit-bose
force-pushed
the
certificate_mapping
branch
from
292f5f2
to
f676497
Compare
|
Thank you, the patches now look good to me. I'm only waiting for the CI run before the final ACK, but it should be noted again that the review was mostly based on reading the code and checking for regressions. I didn't do a very thorough review of the cert mapping library, but given that it requires knowledge I don't have, it's well tested and the API looks OK, then I think the review is sufficient. |
|
btw the bug with the D-Bus lookups is fixed with the new version. |
|
There is an autoconf warning: And there are libsss_certmap_openssl_la* flags even though libsss_certmap_openssl.so is not build. and what about small lines de-duplication :-) Not tested. |
|
And there are few missing header files when compiling with libcrypto. diff: |
mapped_attrs can be a list of sysdb_attrs which are not available on the server side but should be store with the cached user entry. This is needed e.g. when the input to look up the user in LDAP is not an attribute which is stored in LDAP but some data where LDAP attributes are extracted from. The current use case is the certificate mapping library which can create LDAP search filters based on content of the certificate. To allow upcoming cache lookup to use the input directly it is stored in the user object in the cache. Related to https://pagure.io/SSSD/sssd/issue/3050
Store the certificate used to lookup a user as mapped attribute in the cached user object. Related to https://pagure.io/SSSD/sssd/issue/3050
Use certificate mapping library if available to lookup a user by certificate in LDAP. Related to https://pagure.io/SSSD/sssd/issue/3050
Add sysdb calls to write and read data for the certificate mapping library to the cache. Related to https://pagure.io/SSSD/sssd/issue/3050
Read certificate mapping data from the IPA server and configure the certificate mapping library accordingly. Related to https://pagure.io/SSSD/sssd/issue/3050
sumit-bose
force-pushed
the
certificate_mapping
branch
from
f676497
to
14eec3d
Compare
|
Hi Lukas, thank you for your patches, I included both of them in the latest versions. |
This is the last major part related to https://pagure.io/SSSD/sssd/issue/3050.
With this users and certificates cannot only be mapped by adding the full
certificate to the user's LDAP entry but based on rules.
The library itself is in the third patch and the rules are described in the
included sss-certmap man page. I tried to cover most of the functionality by
unit tests.