Skip to content

[Entitlements] Log message for troubleshooting if not entitled #122948

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.

Sign up for GitHub

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

Merged
elasticsearchmachine merged 2 commits into from
Feb 19, 2025
Merged

[Entitlements] Log message for troubleshooting if not entitled #122948

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbf0033
[Entitlements] Log message for troubleshooting if not entitled
mosche Feb 19, 2025
7f324cf
add exception
mosche Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions ...entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import org.elasticsearch.core.Strings;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.entitlement.bootstrap.EntitlementBootstrap;
import org.elasticsearch.entitlement.instrumentation.InstrumentationService;
import org.elasticsearch.entitlement.runtime.api.NotEntitledException;
import org.elasticsearch.entitlement.runtime.policy.entitlements.CreateClassLoaderEntitlement;
Expand Down Expand Up @@ -215,7 +216,8 @@ private void neverEntitled(Class<?> callerClass, Supplier<String> operationDescr
requestingClass.getModule().getName(),
requestingClass,
operationDescription.get()
)
),
callerClass
);
}

Expand Down Expand Up @@ -274,7 +276,8 @@ public void checkFileRead(Class<?> callerClass, Path path) {
requestingClass.getModule().getName(),
requestingClass,
path
)
),
callerClass
);
}
}
Expand All @@ -299,7 +302,8 @@ public void checkFileWrite(Class<?> callerClass, Path path) {
requestingClass.getModule().getName(),
requestingClass,
path
)
),
callerClass
);
}
}
Expand Down Expand Up @@ -348,14 +352,15 @@ public void checkAllNetworkAccess(Class<?> callerClass) {
}

var classEntitlements = getEntitlements(requestingClass);
checkFlagEntitlement(classEntitlements, InboundNetworkEntitlement.class, requestingClass);
checkFlagEntitlement(classEntitlements, OutboundNetworkEntitlement.class, requestingClass);
checkFlagEntitlement(classEntitlements, InboundNetworkEntitlement.class, requestingClass, callerClass);
checkFlagEntitlement(classEntitlements, OutboundNetworkEntitlement.class, requestingClass, callerClass);
}

private static void checkFlagEntitlement(
ModuleEntitlements classEntitlements,
Class<? extends Entitlement> entitlementClass,
Class<?> requestingClass
Class<?> requestingClass,
Class<?> callerClass
) {
if (classEntitlements.hasEntitlement(entitlementClass) == false) {
notEntitled(
Expand All @@ -365,7 +370,8 @@ private static void checkFlagEntitlement(
requestingClass.getModule().getName(),
requestingClass,
PolicyParser.getEntitlementTypeName(entitlementClass)
)
),
callerClass
);
}
logger.debug(
Expand Down Expand Up @@ -405,11 +411,16 @@ public void checkWriteProperty(Class<?> callerClass, String property) {
requestingClass.getModule().getName(),
requestingClass,
property
)
),
callerClass
);
}

private static void notEntitled(String message) {
private static void notEntitled(String message, Class<?> callerClass) {
// don't log self tests in EntitlementBootstrap
if (EntitlementBootstrap.class.equals(callerClass) == false) {
logger.warn(message);
Copy link
Member

@rjernst rjernst Feb 19, 2025

Choose a reason for hiding this comment

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

If you construct the not entitled exception first then the exception can be logged, along with the stack trace.

Copy link
Contributor Author

@mosche mosche Feb 19, 2025

Choose a reason for hiding this comment

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

I thought about doing it, but decided against it as we'd be logging the exception twice most of the time. I often find that more confusing / disturbing than being helpful in rare cases. On the other hand, obviously, those warnings are a lot more noticeable if there's a stacktrace attached.
I don't have a strong preference, happy to follow the crowed here.

Copy link
Contributor

@prdoyle prdoyle Feb 19, 2025

Choose a reason for hiding this comment

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

I don't feel strongly in this case. The message is likely to have enough info to determine the remedy already.

Copy link
Contributor

@ldematte ldematte Feb 19, 2025

Choose a reason for hiding this comment

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

Same, not a strong preference. OK to leave it as-is -- like you said, this is mainly for when we have code that catches Throwable or similar.

Copy link
Contributor Author

@mosche mosche Feb 19, 2025

Choose a reason for hiding this comment

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

👍 added

}
throw new NotEntitledException(message);
}

Expand All @@ -422,7 +433,7 @@ private void checkEntitlementPresent(Class<?> callerClass, Class<? extends Entit
if (isTriviallyAllowed(requestingClass)) {
return;
}
checkFlagEntitlement(getEntitlements(requestingClass), entitlementClass, requestingClass);
checkFlagEntitlement(getEntitlements(requestingClass), entitlementClass, requestingClass, callerClass);
}

ModuleEntitlements getEntitlements(Class<?> requestingClass) {
Expand Down