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.

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

ipa_sam: remove dependency to sid_string_talloc() and sid_string_dbg() #2943

Closed
wants to merge 1 commit into from

Conversation

mastersin
Copy link

ipa_sam uses Samba's sid to string construction functions. Recent Samba
versions removed this functions from libsmbconf.so and replace new functions
to internal libsamba-security-samba4.so

Thus, we reimplement new style function dom_sid_str_buf() from Samba-4.10.

Resolves: https://pagure.io/freeipa/issue/7893

Copy link
Contributor

@abbra abbra left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, @mastersin. I have a few comments and also please rebase on top of #2926.

@@ -100,12 +101,15 @@ struct unixid {
enum id_type type;
}/* [public] */;

/* from samba/libcli/security/dom_sid.h */
#define MAX(a,b) ((a)>(b)?(a):(b))
Copy link
Contributor

Choose a reason for hiding this comment

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

You can get MAX from #include <sys/param.h>.

@@ -283,6 +287,62 @@ static bool is_null_sid(const struct dom_sid *sid)
return true;
}

static int dom_sid_string_buf(const struct dom_sid *sid, char *buf, int buflen)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename to _domain_sid_string_buf to avoid possible conflicts when loading in the same process.

return ofs;
}

static char *dom_sid_str_buf(const struct dom_sid *sid, struct dom_sid_buf *dst)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, please rename to _domain_sid_str_buf

@abbra abbra added the re-run Trigger a new run of PR-CI label Mar 27, 2019
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Mar 27, 2019
@abbra
Copy link
Contributor

abbra commented Mar 27, 2019

Actually, I have a better idea. We are linking anyway to SSSD's libsss_idmap library which has all the same code already. See calls to sss_idmap_sid_to_smb_sid() -- this converts stringified SID version to struct dom_sid. There is a reverse function too: sss_idmap_smb_sid_to_sid(). It takes idmap context which we already have initialized in the ipasam state and we use talloc to allocate resources. This would make it consistent with other places we have.

@mastersin mastersin force-pushed the master branch 2 times, most recently from 0cdeca2 to e16bc89 Compare March 28, 2019 00:01
@mastersin
Copy link
Author

Fix and rebase current variant. Thinks about better idea as next step.

@freeipa-pr-ci freeipa-pr-ci added the needs rebase Pull Request cannot be automatically merged - needs to be rebased label Mar 28, 2019
@mastersin mastersin force-pushed the master branch 2 times, most recently from 080b49f to 284d380 Compare March 28, 2019 15:40
ipa_sam uses Samba's sid to string construction functions. Recent Samba
versions removed this functions from libsmbconf.so and replace new functions
to internal libsamba-security-samba4.so

Thus, we reimplement new style function dom_sid_str_buf() from Samba-4.10.

Resolves: https://pagure.io/freeipa/issue/7893
@abbra
Copy link
Contributor

abbra commented Mar 29, 2019

@mastersin let me know if you are able to work on the proposed approach. If not, I'll do it myself next week.

@mastersin
Copy link
Author

@abbra I try it and understand, that I don't know enough clearly how works talloc contexts in this code, and also I don't have time to debug it yet. I afraid that I may add memory leaks in proposed approach.

@abbra
Copy link
Contributor

abbra commented Mar 31, 2019

@mastersin I submitted #2966 that should solve the issue and obsoletes this PR. Please review.

@tiran tiran added rejected Pull Request has been rejected and removed needs rebase Pull Request cannot be automatically merged - needs to be rebased labels Apr 1, 2019
@tiran
Copy link
Member

tiran commented Apr 1, 2019

PR #2966 with an alternative fix has landed.

@tiran tiran closed this Apr 1, 2019
@tiran
Copy link
Member

tiran commented Apr 1, 2019

Thanks for your patch any way! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rejected Pull Request has been rejected
Projects
None yet
4 participants