Remove combo security and license helper from license state #55366

rjernst · 2020-04-17T01:31:50Z

Security features in the license state currently do a dynamic check on
whether security is enabled. This is because the license level can
change the default security enabled state. This commit splits out the
check on security being enabled, so that the combo method of security
enabled plus license allowed is no longer necessary.

Security features in the license state currently do a dynamic check on whether security is enabled. This is because the license level can change the default security enabled state. This commit splits out the check on security being enabled, so that the combo method of security enabled plus license allowed is no longer necessary.

elasticmachine · 2020-04-17T01:31:52Z

Pinging @elastic/es-security (:Security/License)

rjernst · 2020-04-17T01:35:36Z

Note that I have intentionally not added new uses of the "copy license state" method. I think that method is making the code more convoluted, and not actually fully protecting from race conditions because several different services within a request may check the license state, yet that method only copies within a single method. We should instead completely rework how the license state is frozen within a request once #53909 is complete.

tvernum · 2020-04-17T03:37:25Z

I have intentionally not added new uses of the "copy license state" method.

In the abstract, that concerns me (subject to how long #53909 will take). This behaviour was added to solve real race conditions that affected users.

However, I think in most (maybe all) cases here it's fine.
Generally the checks have been written as

licenseState.isSecurityEnabled() && licenseState.isFeatureAllowed()

The only situation were need to worry about is moving between a license where security is enabled by default (Gold, Platinum, Enterprise) to one where it is not (Trial, Basic), so that the first condition occurs on the old license and the second condition checks the new license. If the security-enabled state doesn't change then there's no race condition between isSecurityEnabled would be constant, and the fact that the first condition was checked on a different license state wouldn't actually matter.

If you move from not-enabled to enabled (install a paid license), then this should be safe because the first condition fails, so the second check is never performed, so there is no issue.

Moving from platinum/enterprise to trial should have no effect, because the set of features are identical.

Moving from platinum/enterprise to basic could have an effect, because the second check would fail where it would have passed on the first license, but since the basic license would have failed the first check, the overall result is as if the check was performed on a basic license.

So the only case that could matter is moving from Gold to Trial. In that case we could have a situation where we detected security-enabled && feature-allowed even though that didn't hold true for either license independently.
Given that going from Gold to Trial is exceedingly rare, I think we can tolerate the issue while we wait for #53909

tvernum · 2020-04-17T03:48:39Z

...ava/org/elasticsearch/xpack/security/authz/interceptor/IndicesAliasesRequestInterceptor.java

@@ -51,7 +51,7 @@ public void intercept(RequestInfo requestInfo, AuthorizationEngine authorization
 final XPackLicenseState frozenLicenseState = licenseState.copyCurrentLicenseState();
 final AuditTrail auditTrail = auditTrailService.get();
 if (frozenLicenseState.isSecurityEnabled()) {
- if (frozenLicenseState.isDocumentAndFieldLevelSecurityAllowed()) {
+ if (frozenLicenseState.isSecurityEnabled() && frozenLicenseState.isDocumentAndFieldLevelSecurityAllowed()) {


The isSecurityEnabled is covered in the enclosing if

tvernum · 2020-04-17T03:49:38Z

...c/main/java/org/elasticsearch/xpack/security/authz/interceptor/ResizeRequestInterceptor.java

@@ -47,7 +47,7 @@ public void intercept(RequestInfo requestInfo, AuthorizationEngine authorization
 final XPackLicenseState frozenLicenseState = licenseState.copyCurrentLicenseState();
 final AuditTrail auditTrail = auditTrailService.get();
 if (frozenLicenseState.isSecurityEnabled()) {
- if (frozenLicenseState.isDocumentAndFieldLevelSecurityAllowed()) {
+ if (frozenLicenseState.isSecurityEnabled() && frozenLicenseState.isDocumentAndFieldLevelSecurityAllowed()) {


isSecurityEnabled is covered in the enclosing if

tvernum · 2020-04-17T03:51:00Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

@@ -165,7 +165,7 @@ public void roles(Set<String> roleNames, ActionListener<Role> roleActionListener
 rolesRetrievalResult.getMissingRoles()));
 }
 final Set<RoleDescriptor> effectiveDescriptors;
- if (licenseState.isDocumentAndFieldLevelSecurityAllowed()) {
+ if (licenseState.isSecurityEnabled() && licenseState.isDocumentAndFieldLevelSecurityAllowed()) {


I think in this context we should ignore isSecurityEnabled()
It's a check to disable roles with DLS/FLS is the license does not permit it. I think it confuses the code to explicitly include enabled in that check.

tvernum · 2020-04-17T03:52:04Z

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

@@ -319,8 +319,9 @@ private void roleDescriptors(Set<String> roleNames, ActionListener<RolesRetrieva

 private void loadRoleDescriptorsAsync(Set<String> roleNames, ActionListener<RolesRetrievalResult> listener) {
 final RolesRetrievalResult rolesResult = new RolesRetrievalResult();
+ boolean allAllowed = licenseState.isSecurityEnabled() && licenseState.isCustomRoleProvidersAllowed();


As above, I'd skip the enabled check here. It's not clear why we'd prefer one behaviour over another if we ended up in deep in this method but security was disabled.

tvernum · 2020-04-17T03:52:28Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java

@@ -175,7 +175,7 @@ public static Path resolveFile(Environment env) {
 if (Files.exists(path)) {
 try {
 List<String> roleSegments = roleSegments(path);
- final boolean flsDlsLicensed = licenseState.isDocumentAndFieldLevelSecurityAllowed();
+ final boolean flsDlsLicensed = licenseState.isSecurityEnabled() && licenseState.isDocumentAndFieldLevelSecurityAllowed();


As above, I'd skip enabled

tvernum · 2020-04-17T03:52:49Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

@@ -188,7 +188,7 @@ public void onFailure(Exception e) {
 }

 public void putRole(final PutRoleRequest request, final RoleDescriptor role, final ActionListener<Boolean> listener) {
- if (licenseState.isDocumentAndFieldLevelSecurityAllowed()) {
+ if (licenseState.isSecurityEnabled() && licenseState.isDocumentAndFieldLevelSecurityAllowed()) {


tvernum · 2020-04-17T03:53:05Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

@@ -369,7 +369,7 @@ static RoleDescriptor transformRole(String id, BytesReference sourceBytes, Logge
 // we pass true as last parameter because we do not want to reject permissions if the field permissions
 // are given in 2.x syntax
 RoleDescriptor roleDescriptor = RoleDescriptor.parse(name, sourceBytes, true, XContentType.JSON);
- if (licenseState.isDocumentAndFieldLevelSecurityAllowed()) {
+ if (licenseState.isSecurityEnabled() && licenseState.isDocumentAndFieldLevelSecurityAllowed()) {


rjernst · 2020-04-17T19:20:18Z

Thanks for the thorough review @tvernum!

…55366) Security features in the license state currently do a dynamic check on whether security is enabled. This is because the license level can change the default security enabled state. This commit splits out the check on security being enabled, so that the combo method of security enabled plus license allowed is no longer necessary.

…55417) Security features in the license state currently do a dynamic check on whether security is enabled. This is because the license level can change the default security enabled state. This commit splits out the check on security being enabled, so that the combo method of security enabled plus license allowed is no longer necessary.

rjernst added >refactoring :Security/License License functionality for commercial features v8.0.0 v7.8.0 labels Apr 17, 2020

rjernst added 2 commits April 16, 2020 18:57

fix bad enabled logic in FLS

6f1532e

test fixes

6ecc6d4

tvernum self-requested a review April 17, 2020 03:12

tvernum approved these changes Apr 17, 2020

View reviewed changes

rjernst added 2 commits April 17, 2020 10:14

Merge branch 'master' into refactor_license9

b0b86c0

address feedback

dc5b796

rjernst merged commit 6271922 into elastic:master Apr 17, 2020

rjernst deleted the refactor_license9 branch April 17, 2020 19:20

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove combo security and license helper from license state #55366

Remove combo security and license helper from license state #55366

rjernst commented Apr 17, 2020

elasticmachine commented Apr 17, 2020

rjernst commented Apr 17, 2020

tvernum commented Apr 17, 2020 •

edited

Loading

tvernum Apr 17, 2020

tvernum Apr 17, 2020

tvernum Apr 17, 2020

tvernum Apr 17, 2020

tvernum Apr 17, 2020

tvernum Apr 17, 2020

tvernum Apr 17, 2020

rjernst commented Apr 17, 2020

Remove combo security and license helper from license state #55366

Remove combo security and license helper from license state #55366

Conversation

rjernst commented Apr 17, 2020

elasticmachine commented Apr 17, 2020

rjernst commented Apr 17, 2020

tvernum commented Apr 17, 2020 • edited Loading

tvernum Apr 17, 2020

Choose a reason for hiding this comment

tvernum Apr 17, 2020

Choose a reason for hiding this comment

tvernum Apr 17, 2020

Choose a reason for hiding this comment

tvernum Apr 17, 2020

Choose a reason for hiding this comment

tvernum Apr 17, 2020

Choose a reason for hiding this comment

tvernum Apr 17, 2020

Choose a reason for hiding this comment

tvernum Apr 17, 2020

Choose a reason for hiding this comment

rjernst commented Apr 17, 2020

tvernum commented Apr 17, 2020 •

edited

Loading