Remove interception of unix domain sockets connections #17884
Conversation
Signed-off-by: Gulshan Kumar <kumargu@amazon.com>
kumargu
requested review from
Bukhtawar,
CEHENKLE,
Rishikesh1159,
VachaShah,
anasalkouz,
andrross,
ashking94,
bugmakerrrrrr,
cwperks,
dbwiddis,
gbbafna,
jainankitk,
kotwanikunal,
linuxpi,
mch2,
msfroh,
nknize,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
|
@kumargu What about this one? |
|
Strong 👎 We could just drop security manager and agent with zero churn. I would have better work to help to fix the issues, since we are very likely would add more (not less!) interceptors in the future, and such kind of changes are guarantee to happen. |
@reta I am not saying we drop it forever. The idea was to get the builds moving forward to identify what else is broken. Given, your other PR has worked for codec build, I can close this one. |
On this: re-quoting my earlier stances: #1687 (comment) #16729 (comment) |
|
❌ Gradle check result for d9fef15: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
@reta @kumargu Honestly my concern here is with the platform-specific behavior we're seeing here. This feels like very low level details leaking into the security policy. Given we're seeing this only on Windows, this will likely introduce development friction as Windows is not the common development platform. Failures will only show up later in PRs or other integration test environments. I'd like to explore whether we can infer the appropriate permissions without needing to explicitly call out |
|
@jmazanec15 Just sent me a k-nn failure. I see that the k-NN repo already does make this grant. I'm planning to take a deeper dive, but on the surface it looks like we are trying every ProtectionDomain on the call stack instead of just ensuring that at least one of the ProtectionDomain's on the call stack has the grant. Is my understanding correct? Full stack trace |
@andrross this is not a call for one platform but it is a permission to use UNIX domain sockets. We are seeing Windows failures here because recent JDKs do use UNIX domain sockets on Windows for loopback (and I think it is Windows 10 or above). JDK makes this decision inside the socket impl, but because we are not part of it anymore - we have to be much more explicit / precise with permissions |
that failure is from the tests, right? I don't think |
@reta I understand. This is the point I'm pushing on. Do we actually need to be so precise. Can our policy enable generic "socket" permissions then the validation code can apply it to whatever we deem appropriate (in this case it would be both InetSocketAddress and UnixDomainSocketAddress). |
@andrross in my opinion, being precise is a must , and it does not hurt - if our goal is to build a minimal but robust foundation. If our goal is to release something that pretend to do right thing - sure, we don't need to be precise. Yes, being precise requires some work. |
4 participants
The major motivation to remove the interception of unix domain sockets connections is to reduce the churn of granting allow access to domain sockets across multiple places in core and more importantly across plugins. A recent stream of updates haven't had been able to resolve errors on window build (see opensearch-project/custom-codecs#235).
While this could be a temporary change: there are arguments that this change is generally safe since we can trust IPC communications. See more discussion at
At this point in time, we would like to get the windows build making some progress to be able to identify what else is broken and needs to be patched across various plugins.
If we are not comfortable removing this interception, we can come back to it in RC-2 with the right sets of plumbings needed.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.