Remove combo security and license helper from license state by rjernst · Pull Request #55366 · elastic/elasticsearch

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

            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

            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

                        }
                        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


    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

            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


    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

            // 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 merged 5 commits intoelastic:masterfrom
rjernst:refactor_license9

rjernst commented Apr 17, 2020

Uh oh!

elasticmachine commented Apr 17, 2020

Uh oh!

rjernst commented Apr 17, 2020

Uh oh!

tvernum commented Apr 17, 2020 •

edited

Loading

Uh oh!

tvernum Apr 17, 2020

Uh oh!

tvernum Apr 17, 2020

Uh oh!

tvernum Apr 17, 2020

Uh oh!

tvernum Apr 17, 2020

Uh oh!

tvernum Apr 17, 2020

Uh oh!

tvernum Apr 17, 2020

Uh oh!

tvernum Apr 17, 2020

Uh oh!

rjernst commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rjernst commented Apr 17, 2020

Uh oh!

elasticmachine commented Apr 17, 2020

Uh oh!

rjernst commented Apr 17, 2020

Uh oh!

tvernum commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvernum Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

tvernum Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

tvernum Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

tvernum Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

tvernum Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

tvernum Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

tvernum Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

rjernst commented Apr 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tvernum commented Apr 17, 2020 •

edited

Loading