squid
squid copied to clipboard
squid-cache
negotiate_kerberos_auth: Support Kerberos PAC-ResourceGroups
Parse the ResourceGroupIds pac-data structure to have information about the user's membership in AD Domain Local groups.
Previously, the helper obtained user groups information only from GroupIds and ExtraSids pac-data structures (of the KERB_VALIDATION_INFO structure). The patch extends the functionality of the helper. Now it additionally parse the ResourceGroupIds pac-data structure where Domain Local AD-group rids are located. It appends these groups to the the list generated by parsing GroupIds and ExtraSids. No changes in existing helper deployments are required.
The new parsing functions are similar to those already used for parsing GroupIds and ExtraSids.
@rousskov Alex, thank you for the comments.
If possible, please adjust PR description to explicitly disclose how/whether these changes affect existing helper deployments and to briefly note how the new feature can be used (or what it can be used for).
I have adjusted the description.
IMO, proposed code violates several basic engineering principles, but I assume it is mimicking existing Kerberos parsing code in Squid and that it has been proven to "work" for some use cases, so I am not going to vote against this PR. One of those principles is reviewability; FWIW, I am unable to properly review this kind of code and, hence, will not be requesting any changes.
I do not know how to improve the reviewwability of the code. The new functions are based on the previous code which parse GroupIds and ExtraSids. I can try to rewrite the code of the negotiate_kerberos_pac.cc but I don't know the mentioned "basic engineering principles".
I suggest declaring the whole helper as unsupported by the Project, so that there is less pressure on the Project to fix this helper when somebody bothers to test this code thoroughly and publish a dozen vulnerability reports against it. Such a declaration would also simplify accepting unreviewed helper code. CC: @yadij, @kinkie.
I think the Kerberos helper is too important for the squid-users, and it would be nice to keep its support.
I also added the definitions of the new functions to negotiate_kerberos.h to fix the github CI build problem.
I think the Kerberos helper is too important for the squid-users, and it would be nice to keep its support.
Since the code of the Kerberos helper has been unchanged for many years, I think the current version fully meets the needs of the squid-community. So we can reject my PR, especially taking into account the lack of resources of the Squid-team.
@ankor2023; please do not be intimidated by @rousskov opinions. IME he has a strong tendency to say these same things about any code he does not personally see people complaining about or wanting constant changes to.
@rousskov; this is one of the primary helpers needed/used by admin managing a network with ActiveDirectory authentication. Lack of feedback is more an indication of how it is working for that group without causing them trouble. AFAIK Markus Moeller is still supporting the code.
Andrey: I have adjusted the description.
Thank you very much! Assuming your commit 8c0b194 fixes CI tests, the ball is now fully on the Squid Project side.
Alex: IMO, proposed code violates several basic engineering principles, but I assume it is mimicking existing Kerberos parsing code in Squid and that it has been proven to "work" for some use cases, so I am not going to vote against this PR. One of those principles is reviewability; FWIW, I am unable to properly review this kind of code and, hence, will not be requesting any changes.
Andrey: I do not know how to improve the reviewwability of the code.
AFAICT, it would require a serious rewrite, and I am not asking for that, especially since such a rewrite is likely to create tensions with the existing Kerberos parsing code.
The new functions are based on the previous code which parse GroupIds and ExtraSids. I can try to rewrite the code of the negotiate_kerberos_pac.cc but I don't know the mentioned "basic engineering principles".
Glad we are on the same page. I do not recommend a rewrite in this case.
Alex: I suggest declaring the whole helper as unsupported by the Project, so that there is less pressure on the Project to fix this helper when somebody bothers to test this code thoroughly and publish a dozen vulnerability reports against it. Such a declaration would also simplify accepting unreviewed helper code.
Andrey: I think the Kerberos helper is too important for the squid-users, and it would be nice to keep its support.
I agree, and my recommendation does not contradict the above assertions. My recommendation takes the current state of code and Project resources into account: There is not much we can do here in the short term to solve the problems that need to be solved. IMO, it is much better to be honest about serious problems than to pretend they do not exist; while awareness of problem existence may intimidate, that negative/unwanted side effect is better than the false sense of security.
Amos: this is one of the primary helpers needed/used by admin managing a network with ActiveDirectory authentication.
My recommendation does not contradict the above fact.
Amos: AFAIK Markus Moeller is still supporting the code.
IMO, if Markus is indeed supporting this code, he should be reviewing this PR. If you can solicit his review, please do so; I am not sure whether the following mention reaches Markus. CC: @huaraz.
Andrey: we can reject my PR, especially taking into account the lack of resources of the Squid-team.
I (still) do not recommend rejection at this time because the proposed feature is obviously useful, and it does not make the helper worse (there are no known new flaws in the proposed code). While I cannot properly review the details, I do support accepting this PR, especially if others can review this code (or at least share my opinion that this code should be accepted). I am approving this PR without clearing it for merging so that @huaraz, @yadij, and @kinkie have plenty of time to properly review this PR.
The change looks fine i.e. is in line with the way I extract SIDs and resource groups are relevant for some cases of forest trusts. I am only surprised nobody mentioned this use case earlier.
https://learn.microsoft.com/en-us/previous-versions/windows/it-pro/windows-server-2003/cc772808(v=ws.10)?redirectedfrom=MSDN
Markus
@huaraz, thank you for reviewing this code!
@yadij, @ankor2023, will you address the minor problems identified in my earlier review or should we merge this PR "as is"?
The change looks fine i.e. is in line with the way I extract SIDs and resource groups are relevant for some cases of forest trusts. I am only surprised nobody mentioned this use case earlier.
@huaraz, thank you, Markus!
@yadij, @ankor2023, will you address the minor problems identified in my earlier review or should we merge this PR "as is"?
@rousskov, I have corrected the code according to your recommendations. Thank you, Alex!
@huaraz: Hello, Markus,
The code uses RPC_UNICODE_STRING data structures:
typedef struct {
uint16_t length;
uint16_t maxlength;
uint32_t pointer;
} RPC_UNICODE_STRING;
It corresponds to the Microsoft article. I can't figure out where the uint32_t pointer points. Can I use it to access the actual data inside the PAC structure without having to iterate through the following KERB_VALIDATION_INFO elements? If it's not too much trouble, could you clarify a little?
@rousskov, @yadij :
Hello, Alex and Amos,
The PR has s-waiting-for-author tag. Do I need to take any action on my part?
Kind regards, Ankor.
The PR has s-waiting-for-author tag. Do I need to take any action on my part?
I added it initially for @rousskov question. Those changes have now been done. If you think all the changes that need to be done are completed please request the re-review and switch to S-waiting-for-reviewer.
Alex: The code should be formatted before clearing it for merging.
Andrey: The PR has s-waiting-for-author tag. Do I need to take any action on my part?
I see two actions that are not required, but could speed things up:
-
If getting your question answered is not required to merge this PR, please say so explicitly. It was not clear to me whether we should wait for that answer...
-
As I said, it would be nice to format new/changed sources before merging, but you are not required to do that. A core developer that clears this PR for merging can do that (by running ./scripts/source-maintenance.sh and then removing out-of-scope changes). FWIW, the minimal/automated formatting fixes are visible as a diff at the end of test-suite/test-sources.sh test log.
Others may request other actions/changes, of course, but I am not aware of any other outstanding requests (or any blocking reviews -- this PR has been approved).
Ignoring most of the formatting for now. There are some potential error cases that seem important to handle.
@yadij
Hello, Amos,
Thank you very much for the review! I will check it.
Ignoring most of the formatting for now. There are some potential error cases that seem important to handle.
@yadij : Hello, Amos,
I posted a new version of the file, where I fixed your comments, renamed some variables to more appropriate ones and added some checks. Could you review it if you have time?
@yadij requested a review from rousskov on December 5, 2023 02:19
My position has not changed since #1597 (review)
I suggest declaring the whole helper as unsupported by the Project, so that there is less pressure on the Project to fix this helper when somebody bothers to test this code thoroughly and publish a dozen vulnerability reports against it. Such a declaration would also simplify accepting unreviewed helper code.
@rousskov : Hello, Alex,
Could you please explain how it will affect the squid users? Maybe accepting unreviewed code is not good for the project, especially considering the importance of this helper? Maybe there are other developers in your core team who could help Markus Moeller support the code?
Alex: I suggest declaring the whole helper as unsupported by the Project, so that there is less pressure on the Project to fix this helper when somebody bothers to test this code thoroughly and publish a dozen vulnerability reports against it. Such a declaration would also simplify accepting unreviewed helper code.
Andrey: Could you please explain how it will affect the squid users?
I expect no short-term effects. Long-term, and in conjunction with various other Project actions, I hope users will get better Squid because more existing resources will be spent on more important matters (it is a zero-sum game) and/or more new resources will be added (to reestablish proper support of an important feature).
To avoid misunderstanding, I do not think any of that really matters because, I bet, my suggestion is not going to be accepted.
Maybe accepting unreviewed code is not good for the project, especially considering the importance of this helper?
Yes, accepting unreviewed code is clearly bad for the Project. Sometimes, we have to pick the lesser evil. In some cases, accepting code without a proper review is less bad than rejecting code without a proper review. Both actions "are not good for the project".
Maybe there are other developers in your core team who could help Markus Moeller support the code?
There are currently three core developers: @kinkie, @yadij, and @rousskov. I do not speak for others, but all core developers are (or, IMHO, should be) reading comments on this PR and, I bet, the three of us disagree what "support the code" really means in this context, whether this particular code is currently "supported" (by anybody), etc. We are also, IMHO, overloaded, so it is not really a question of finding a core developer to assign a new responsibility to; it is a question of what are we willing to sacrifice in exchange for that new assignment.
There are currently three core developers: @kinkie, @yadij, and @rousskov. I do not speak for others, but all core developers are (or, IMHO, should be) reading comments on this PR and, I bet, the three of us disagree what "support the code" really means in this context, whether this particular code is currently "supported" (by anybody), etc. We are also, IMHO, overloaded, so it is not really a question of finding a core developer to assign a new responsibility to; it is a question of what are we willing to sacrifice in exchange for that new assignment.
I agree with what @rousskov wrote, I would like to highlight however how wonderful it would be if more people, expressing their abilities through successful contributions, could join the group of core developers :)
Regarding whether this code should be part of the core distribution or not: our shared goal is to provide the most reliable, powerful, and easy to manage tool we can given our resources.
Maybe a possible way forward is to resurrect a contrib/ subtree, which would highlight how the core developers cannot provide guarantees over the quality of what is contained there, while trying to maximise convenience for downstream administrators, and possibly empowering some contributors to tune their skills there - in the end that's how I approached the project, too
Regarding whether this code should be part of the core distribution or not
Please note that nobody has suggested to remove this helper from Squid distribution. I have suggested to declare the helper unsupported, but that is a very different suggestion.
Maybe a possible way forward is to resurrect a contrib/ subtree
IMO, it is best to avoid placing source code based on its support status (and moving code when its support status changes). A lot of code, especially helper code, requires certain build infrastructure/context/surroundings. We do not want to duplicate that, maintaining, say, two helper collections, in two different source tree locations. There are other, cheaper/lightweight/flexible ways to identify unsupported code (but we better define "supported" before finalizing our indexing approach!).
There are currently three core developers: @kinkie, @yadij, and @rousskov.
@kinkie , @yadij , @rousskov You are three programmers and you support and develop such a complex system. I respect you very much! I'm sure you know how it would be better for the project to declare the helper unsupported, or leave it as it is.
Thank you!
@yadij Hello, Amos,
Thank you so much for such a detailed and valuable review. I will try to take into account all your comments and correct the code in the near future.
@yadij @kinkie @rousskov : I wish you a happy New Year!
@yadij : Hello, Amos,
I've corrected your comments in the code. Let's wait a couple of weeks to test it.
Seeing as there has been no logic updates, nor negative feedback in the past month I am clearing this for merge to Squid-7 as an experimental feature addition.