Avoid NPE in set_security_user without security #52691
Conversation
If security was disabled (explicitly), then the SecurityContext would be null, but the set_security_user processor was still registered. Attempting to define a pipeline that used that processor would fail with an (intentional) NPE. This behaviour, introduced in elastic#52032, is a regression from previous releases where the pipeline was allowed, but was no usable. This change restores the previous behaviour (with a new warning).
tvernum
added
>non-issue
:Security/Security
|
Pinging @elastic/es-security (:Security/Security) |
|
I added 2 new QA tests for running the security plugin but not enabling it. At the moment this Marked |
There was a problem hiding this comment.
LGTM.
I understand that this change is to keep the existing behaviour. But I wonder whether there are some guidelines on how we should deal with these kinda of situations in general, i.e. user configures something that is not yet available. Here we are trying to be lenient. But issue #48773 suggests we should be strict at it (it is a different use case but spirit is similar) and I have PR up to fix it. Now I am not sure whether my PR should be labeled as "breaking" change (or even necessary).
| setting 'xpack.security.authc.token.enabled', 'true' | ||
| setting 'xpack.security.authc.api_key.enabled', 'true' |
There was a problem hiding this comment.
Are these two settings necessary?
There was a problem hiding this comment.
No, I'll remove them, I copied the build from another project, and missed them.
| logger.warn("Creating processor [{}] (tag [{}]) on field [{}] but no security context is available" + | ||
| " - this processor will fail at runtime if it is used", TYPE, tag, field); |
There was a problem hiding this comment.
Not sure how feasible or necessary it is to test the warning message. Another nitpick is maybe change "no security context is available" to something like "security is not enabled" for consistency with other exception message and easier to understand for users.
There was a problem hiding this comment.
I'll come up with something. My concern is trying to diagnose a root cause from secondary symptoms (I know there's no security context, I don't know for sure exactly why that is), but I'll sort something out.
| this.field = field; | ||
| this.properties = properties; | ||
| } | ||
|
|
||
| @Override | ||
| public IngestDocument execute(IngestDocument ingestDocument) throws Exception { | ||
| if (this.securityContext == null) { | ||
| throw new IllegalStateException("No security context available (is security enabled on this cluster?)"); |
There was a problem hiding this comment.
If we are sure, we could state instead of frame it as a Q
|
@elasticmachine update branch |
If security was disabled (explicitly), then the SecurityContext would be null, but the set_security_user processor was still registered. Attempting to define a pipeline that used that processor would fail with an (intentional) NPE. This behaviour, introduced in elastic#52032, is a regression from previous releases where the pipeline was allowed, but was no usable. This change restores the previous behaviour (with a new warning). Backport of: elastic#52691
If security was disabled (explicitly), then the SecurityContext would be null, but the set_security_user processor was still registered. Attempting to define a pipeline that used that processor would fail with an (intentional) NPE. This behaviour, introduced in #52032, is a regression from previous releases where the pipeline was allowed, but was no usable. This change restores the previous behaviour (with a new warning). Backport of: #52691
If security was disabled (explicitly), then the SecurityContext would
be null, but the
set_security_userprocessor was still registered.Attempting to define a pipeline that used that processor would fail
with an (intentional) NPE. This behaviour, introduced in #52032, is a
regression from previous releases where the pipeline was allowed, but
was no usable.
This change restores the previous behaviour (with a new warning).
Resolves: #52474