Address license state update/read thread safety #33396
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change addresses some issues regarding thread safety around updates and method calls on the XPackLicenseState object. There exists a possibility that there could be a concurrent update to the XPackLicenseState when there is a scheduled check to see if the license is expired and a cluster state update. In order to address this, the update method now has a synchronized block where member variables are updated. Each method that reads these variables is now also synchronized. Along with the above change, there was a consistency issue around security calls to the license state. The majority of security checks make two calls to the license state, which could result in incorrect behavior due to the checks being made against different license states. The majority of this behavior was introduced for 6.3 with the inclusion of x-pack in the default distribution. In order to resolve the majority of these cases, the `isSecurityEnabled` method is no longer public and the logic is also included in individual methods about security such as `isAuthAllowed`. There were a few cases where this did not remove multiple calls on the license state, so a new method has been added which creates a copy of the current license state that will not change. Callers can use this copy of the license state to make decisions based on a consistent view of the license state.
jaymode
added
>bug
blocker
v7.0.0
:Security/License
Collaborator
|
Pinging @elastic/es-core-infra |
jaymode
added a commit
that referenced
this pull request
Sep 12, 2018
This change addresses some issues regarding thread safety around updates and method calls on the XPackLicenseState object. There exists a possibility that there could be a concurrent update to the XPackLicenseState when there is a scheduled check to see if the license is expired and a cluster state update. In order to address this, the update method now has a synchronized block where member variables are updated. Each method that reads these variables is now also synchronized. Along with the above change, there was a consistency issue around security calls to the license state. The majority of security checks make two calls to the license state, which could result in incorrect behavior due to the checks being made against different license states. The majority of this behavior was introduced for 6.3 with the inclusion of x-pack in the default distribution. In order to resolve the majority of these cases, the `isSecurityEnabled` method is no longer public and the logic is also included in individual methods about security such as `isAuthAllowed`. There were a few cases where this did not remove multiple calls on the license state, so a new method has been added which creates a copy of the current license state that will not change. Callers can use this copy of the license state to make decisions based on a consistent view of the license state.
jaymode
added a commit
that referenced
this pull request
Sep 12, 2018
This change addresses some issues regarding thread safety around updates and method calls on the XPackLicenseState object. There exists a possibility that there could be a concurrent update to the XPackLicenseState when there is a scheduled check to see if the license is expired and a cluster state update. In order to address this, the update method now has a synchronized block where member variables are updated. Each method that reads these variables is now also synchronized. Along with the above change, there was a consistency issue around security calls to the license state. The majority of security checks make two calls to the license state, which could result in incorrect behavior due to the checks being made against different license states. The majority of this behavior was introduced for 6.3 with the inclusion of x-pack in the default distribution. In order to resolve the majority of these cases, the `isSecurityEnabled` method is no longer public and the logic is also included in individual methods about security such as `isAuthAllowed`. There were a few cases where this did not remove multiple calls on the license state, so a new method has been added which creates a copy of the current license state that will not change. Callers can use this copy of the license state to make decisions based on a consistent view of the license state.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change addresses some issues regarding thread safety around
updates and method calls on the XPackLicenseState object. There exists
a possibility that there could be a concurrent update to the
XPackLicenseState when there is a scheduled check to see if the license
is expired and a cluster state update. In order to address this, the
update method now has a synchronized block where member variables are
updated. Each method that reads these variables is now also
synchronized.
Along with the above change, there was a consistency issue around
security calls to the license state. The majority of security checks
make two calls to the license state, which could result in incorrect
behavior due to the checks being made against different license states.
The majority of this behavior was introduced for 6.3 with the inclusion
of x-pack in the default distribution. In order to resolve the majority
of these cases, the
isSecurityEnabledmethod is no longer public andthe logic is also included in individual methods about security such as
isAuthAllowed. There were a few cases where this did not removemultiple calls on the license state, so a new method has been added
which creates a copy of the current license state that will not change.
Callers can use this copy of the license state to make decisions based
on a consistent view of the license state.