That's quite huge change to the GPO code,
It will take some time to review changes and MSDN documentation.

Firstly I will try to run some regression tests.
Anyway thank you very much for patches.

@thor thank you indeed would you also mind creating an issue? The commit message explanation in the merge commit is excellent, so just copying the message there should be good enough.

(I can create the issue as well if you're busy, just tell me your preference)

@jhrozek you will find the issue under ID #3324

Rebase of the patches and update according to Michals review fetched back from https://github.com/SSSD/sssd/pull/412.

Squashed following minor updates into the patches based on my review and test:
Commit 9df1c37 GPO: Support group policy file main folders with upper case name
Line 33 furute -> future

Commit 8fd1ba6 MAN: Provide minimum information on GPO access control
Line 54 'function data' -> 'trace functions'
Line 56 sss_debuglevel -> sssctl

I have rebased the patches on top of the current master.

@thor thank you very much for rebased paches. And I would like to apologize for longer review.

You found problems with Samba AD server (4.3.11) at least based on ticket #3324. I assume you did everything from command line therefore it should not be difficult to provide steps how to reproduce these failures.

Because I would like to write regressions tests + also confirm that it really fixed your issue. And we have samba-ad-dc-4.7 in f27 so it would be trivial for us :-)

@lslebodn: Thank you for taking care of regression testing.

For my tests I created simple GPOs and modified allow/deny ACEs as well as read access to the GPO itself on a Windows machine. Then I went to the Linux server and tried to login.
Would this be helpful for you?

New patch added:

When evavluating some GPOs for lslebodn, SSSD crashed (free(): invalid pointer). It seems that the free() function that came in during Michals review causes this. Replaced free() by sss_idmap_free_sid() to free the mapped trustee SID.

@thor: Hi Thomas,

good that it crashed for you. I clearly added wrong 'free' function there but somehow managed to pass the evaluations without crashing.

Could you please squash the new patch to the patch that introduced the wrong free's? These patches are not yet in master so it does not make much sense to add new patch to fix an issue that is in another unpushed patch. It is better to fix the wrong patch.

You can use 'git rebase -i' then change the order of the last patch to follow the patch that introduced the wrong frees and use the 'f' for 'fixup' the new patch.

Thanks.
Michal

On (24/11/17 01:35), Thomas Reim wrote:

thor commented on the pull-request: Fix group policy security filtering and download that you are following:
``
@lslebodn: Thank you for taking care of regression testing.

For my tests I created simple GPOs and modified allow/deny ACEs as well as read access to the GPO itself on a Windows machine. Then I went to the Linux server and tried to login.
Would this be helpful for you?

It would be good to provide a little bit more details about GPOs.
Ideally with exact commands which you used when creating them with samba-ad-dc.

It will be helpful twice
* verifying that it really fixed you bug :-)
* and prevent any regressions/bugs in future.

Because bugs in GPO would be considered as security bugs due to deny feature.
ad_gpo_map_deny.

LS

Could you please squash the new patch to the patch that introduced the wrong free's? These patches are not yet in master so it does not make much sense to add new patch to fix an issue that is in another unpushed patch. It is better to fix the wrong patch.

Commit GPO: Do not crash during group policy evaluation sqashed into commit a53f4fbda as mzidek suggested. Thank you for the request, looks much better now. :wink:

Please, replace "follwing" with "following".

Please, replace "i. e." with "i.e.".

"is" should be replaced with "are", shouldn't it?

I believe that 'is' is correct here, as we are talking about the debug level, which has a plural name 'Trace messages for operation functions'.

Updated in patch

updated in patch

Shall I rebase the PR and push the corrected commits again?

Thank you for explanation. I was not aware of that.

From my point of view (not an SSSD developer) it would be good to rebase and push the corrected commits.

From my point of view (not an SSSD developer) it would be good to rebase and push the corrected commits.

Your comments have been integrated and updated PR has been rebased.

Test cases will follow in cw52 (hopefully!)

It would be good to provide a little bit more details about GPOs.
Ideally with exact commands which you used when creating them with samba-ad-dc.
It will be helpful twice
verifying that it really fixed you bug :-)
and prevent any regressions/bugs in future.
Because bugs in GPO would be considered as security bugs due to deny feature.
ad_gpo_map_deny.

@lslebodn:
Sorry for my late reply. It's pretty complicated to configure GPOs using Samba 4 command-line tools. But finally I was able to setup the required regression test framework and succesfully perform the specified tests on the servers here.

You will find a detailed specification including GPO templates and some configuration files as attachment to my comment for issue #3324 that is related to this PR.

Currently, I'm investigating on a simple way to modify GPO ACLs. As soon as I have found a solution, I will add two or three further regression tests.

In parallel I would appreciate your confirming me the feasibility of implementing the specified regression tests in your environment.

Now that 1.16.0 is out, can this go in?

This PR depends on having clean downstream test results which is something @mzidek is working on.

Got clean test results yet?

Nitpick: perhaps this should go after done: so close will be called if the memory allocation or reading the file has failed

bigpick: why is this still not pulled into sssd?

Correct, Reading the GPO file's content might fail and in this case we would leave the file open. This would prevent Samba client lib from closing the context.
I will update the code, but first need to fix some issues which prevent me from rebasing the PR to the latest SSSD baseline. It's now more than a year that the PR has not found its way into the main stream. :-)

Update on 02.10.2018:
Patch has been rebased and updated as requested.

bigpick: why is this still not pulled into sssd?

The issue seems to have low priority :-(

The issue seems to have low priority :-(

Hi,

I have this on my todo list for a very long time. The issue was that I was too afraid to push these patches without having green test results from our internal downstream tests and there are some issues preventing me smoothly debugging the results from those tests (and tests that use Windows machines are particularly problematic). I always ended up switching to a different task after I spent some time with this. Now, some of those issues with our tests were fixed and some failures were identified as false positives, so I hope that after the most immediate deadlines with higher priority I will get to this task for the last time.

My apologies to everyone who is waiting for these patches to be merged and especially @thor (the contributor who wrote the patches).

Michal

@mzidek , this has really taken way too long.

@jockesssd I know and it will actually have to wait at least another week :/ But given how long it is here already it is not that much time I guess...

@mzidek been a very long week now ...

@mzidek ping?

@jockesssd I do not feel comfortable giving any predictions or promises regarding this anymore, but this task is in my current sprint, so I hope it will be done within the time allocated for it (three weeks).

@thor Hi, sorry for the delay in response. As I will not have much time to work on SSSD in the future, I am trying to bring some things from the past to conclusion.

I spent some time with this recently, but unfortunately as of now (or at least two days ago) I still was not able to use the AD/GPO test suite and get green results (there are some issues with the testing infrastructure, that are being worked on, they do have nothing to do with these patches).

I think there are two things that can be done here.
1. Still block these patches until the tests are available again and then push if they are green
2. rely on just manual testing and push these patches

I really wanted to do 1, but now I think it was probably a mistake. The patches were reviewed long time ago and the issues in tests still can be fixed before the releases even if these patches break something (especially now as major downstream releases are relatively far away) and most importantly these patches do fix some issues.

In both cases I think it would be good to move this PR to Github as most activity happens there now and there are also CI tests (even though they do not test AD/GPO code much).

@thor , I know this PR takes ridiculously long time to process, but could you open the PR again on Github and close it here? It would make things more visible and I think it may speed the process up.

If not then I think the best would be for some SSSD developer to cherry pick patches from this PR and review/push them individually as I think they do have a value.

@mzidek , you should at least assign this PR to one of the other sssd devs, not just dump it.

I'm very sorry this took so long, it certainly sheds a bad light on us. I assigned this PR to Pawel and he will prioritize it so hopefully we will merge this soon. Thank you for your patience.

Any chance it will be in next release?

No promises, but unless there will be issues found with the patches I think it will be included.

This pull request has been rebased to latest master and moved to upstream repo on GitHub:
https://github.com/SSSD/sssd/pull/1022

@pbrezina Should we close it here now or wait for uptream PR to be merged first?
@thor I am very sorry for delay in handling this PR, in case of any comments please feel free to ad them here, on GitHub or email me directly.

Since the PR was moved to Github and thor already contributed there, I'm closing this PR. Thank you.