Security: fix token bwc with pre 6.0.0-beta2 #31254

jaymode · 2018-06-11T19:50:53Z

This commit fixes a backwards compatibility bug in the token service
that causes token decoding to fail when there is a pre 6.0.0-beta2 node
in the cluster. The token encoding is actually the culprit as a version
check is missing around the serialization of the key hash bytes. This
value was added in 6.0.0-beta2 and cannot be sent to nodes that do not
know about this value. The version check has been added and the token
service unit tests have been enhanced to randomly run with some 5.6.x
nodes in the cluster service.

Additionally, a small change was made to the way we check to see if the
token metadata needs to be installed. Previously we would pass the
metadata to the install method and check that the token metadata is
null. This null check is now done prior to checking if the metadata can
be installed.

Relates #30743
Closes #31195

This commit fixes a backwards compatibility bug in the token service that causes token decoding to fail when there is a pre 6.0.0-beta2 node in the cluster. The token encoding is actually the culprit as a version check is missing around the serialization of the key hash bytes. This value was added in 6.0.0-beta2 and cannot be sent to nodes that do not know about this value. The version check has been added and the token service unit tests have been enhanced to randomly run with some 5.6.x nodes in the cluster service. Additionally, a small change was made to the way we check to see if the token metadata needs to be installed. Previously we would pass the metadata to the install method and check that the token metadata is null. This null check is now done prior to checking if the metadata can be installed. Relates elastic#30743 Closes elastic#31195

elasticmachine · 2018-06-11T19:50:54Z

Pinging @elastic/es-security

nik9000

The change makes sense to me but I really don't have the background to properly review it.

nik9000 · 2018-06-11T19:55:42Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

-                } else {
-                    logger.debug("cannot add token metadata to cluster as the following nodes might not understand the metadata: {}",
-                        () -> XPackPlugin.nodesNotReadyForXPackCustomMetadata(state));
+                if (custom == null) {


tvernum · 2018-06-12T06:03:56Z

What's here makes sense, but why don't we also have a version check in decodeToken for stream with versions that didn't include the key-hash? (i.e. pre 6.0b2)

jaymode · 2018-06-12T13:40:52Z

why don't we also have a version check in decodeToken for stream with versions that didn't include the key-hash? (i.e. pre 6.0b2)

If I understand your comment correctly, that already exists.

elasticsearch/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

Lines 361 to 366 in 5156223

    
           final BytesKey passphraseHash; 
        
           if (version.onOrAfter(Version.V_6_0_0_beta2)) { 
        
               passphraseHash = new BytesKey(in.readByteArray()); 
        
           } else { 
        
               passphraseHash = keyCache.currentTokenKeyHash; 
        
           }

jaymode · 2018-06-12T13:41:40Z

@nik9000 thanks for your effort in adding the rolling upgrade tests that really made this bug show up.

nik9000 · 2018-06-12T13:43:37Z

@nik9000 thanks for your effort in adding the rolling upgrade tests that really made this bug show up.

I'm happy to find things! It feels nice when new tests catch things right away!

@ywelsch was the one that realized I hadn't switched these to three nodes when I did the OSS work so a lot of the credit goes to him for keeping me honest.

tvernum · 2018-06-13T03:08:08Z

If I understand your comment correctly, that already exists.

Sorry, I was looking on master, not 6.x

tvernum

LGTM

This commit fixes a backwards compatibility bug in the token service that causes token decoding to fail when there is a pre 6.0.0-beta2 node in the cluster. The token encoding is actually the culprit as a version check is missing around the serialization of the key hash bytes. This value was added in 6.0.0-beta2 and cannot be sent to nodes that do not know about this value. The version check has been added and the token service unit tests have been enhanced to randomly run with some 5.6.x nodes in the cluster service. Additionally, a small change was made to the way we check to see if the token metadata needs to be installed. Previously we would pass the metadata to the install method and check that the token metadata is null. This null check is now done prior to checking if the metadata can be installed. Relates #30743 Closes #31195

* 6.x: SQL: Fix build on Java 10 [Tests] Mutualize fixtures code in BaseHttpFixture (#31210) [TEST] Fix RemoteClusterClientTests#testEnsureWeReconnect [ML] Update test thresholds to account for changes to memory control (#31289) Reenable Checkstyle's unused import rule (#31270) [ML] Check licence when datafeeds use cross cluster search (#31247) Fix non-REST doc snippet [DOC] Extend SQL docs [DOCS] Shortens ML API intros Use quotes in the call invocation (#31249) move security ingest processors to a sub ingest directory (#31306) SQL: Whitelist SQL utility class for better scripting (#30681) Add 5.6.11 version constant. Fix version detection. [Docs] All Rollup docs experimental, agg limitations, clarify DeleteJob (#31299) Add missing release notes. Security: fix token bwc with pre 6.0.0-beta2 (#31254) Fix compilation error in UpdateSettingsIT (#31304) Test: Remove broken yml test feature (#31255) Add unreleased version 6.3.1 [Rollup] Metric config parser must use builder so validation runs (#31159) Removes experimental tag from scripted_metric aggregation (#31298) [DOCS] Removes coming tag from 6.3.0 release notes 6.3 release notes. Add notion of internal index settings (#31286) REST high-level client: add Cluster Health API (#29331) Remove leftover usage of deprecated client API SyncedFlushResponse to implement ToXContentObject (#31155) Add Get Aliases API to the high-level REST client (#28799) HLRest: Add get index templates API (#31161) Log warnings when cluster state publication failed to some nodes (#31233) Fix AntFixture waiting condition (#31272) [TEST] Mute RecoveryIT.testHistoryUUIDIsGenerated Ignore numeric shard count if waiting for ALL (#31265) Update checkstyle to 8.10.1 (#31269) Set analyzer version in PreBuiltAnalyzerProviderFactory (#31202) Revert upgrade to Netty 4.1.25.Final (#31282) Use armored input stream for reading public key (#31229) [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160) Fix Netty 4 Server Transport tests. Again. [DOCS] Fixed typo. [DOCS] Added release highlights for 6.3 (#31256) [DOCS] Mark SQL feature as experimental [DOCS] Updates machine learning custom URL screenshots (#31222) Fix naming conventions check for XPackTestCase Fix security Netty 4 transport tests Fix race in clear scroll (#31259) [DOCS] Clarify audit index settings when remote indexing (#30923) [ML][TEST] Mute tests using rules (#31204) Support RequestedAuthnContext (#31238) Validate xContentType in PutWatchRequest. (#31088) [INGEST] Interrupt the current thread if evaluation grok expressions take too long (#31024) Upgrade to Netty 4.1.25.Final (#31232) Suppress extras FS on caching directory tests Revert "[DOCS] Added 6.3 info & updated the upgrade table. (#30940)" Revert "Fix snippets in upgrade docs" Fix snippets in upgrade docs [DOCS] Added 6.3 info & updated the upgrade table. (#30940) Enable custom credentials for core REST tests (#31235) Move ESIndexLevelReplicationTestCase to test framework (#31243) Encapsulate Translog in Engine (#31220) [DOCS] Adds machine learning 6.3.0 release notes (#31217) Remove all unused imports and fix CRLF (#31207) [TEST] Fix testRecoveryAfterPrimaryPromotion [Docs] Remove mention pattern files in Grok processor (#31170) Use stronger write-once semantics for Azure repository (#30437) Don't swallow exceptions on replication (#31179) Compliant SAML Response destination check (#31175) Move java version checker back to its own jar (#30708) TEST: Retry synced-flush if ongoing ops on primary (#30978) [test] add fix for rare virtualbox error (#31212)

jaymode added >bug :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.4.0 v6.3.1 labels Jun 11, 2018

jaymode requested review from nik9000 and tvernum June 11, 2018 19:50

nik9000 reviewed Jun 11, 2018

View reviewed changes

tvernum approved these changes Jun 13, 2018

View reviewed changes

jaymode merged commit c63cbc1 into elastic:6.x Jun 13, 2018

jaymode deleted the token_v56_bwc branch June 13, 2018 18:10

jaymode mentioned this pull request Jun 13, 2018

[TEST] TokenBackwardsCompatibilityIT.testMixedCluster failing #31195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security: fix token bwc with pre 6.0.0-beta2 #31254

Security: fix token bwc with pre 6.0.0-beta2 #31254

jaymode commented Jun 11, 2018 •

edited

Loading

elasticmachine commented Jun 11, 2018

nik9000 left a comment

nik9000 Jun 11, 2018

tvernum commented Jun 12, 2018

jaymode commented Jun 12, 2018

jaymode commented Jun 12, 2018

nik9000 commented Jun 12, 2018

tvernum commented Jun 13, 2018

tvernum left a comment

Security: fix token bwc with pre 6.0.0-beta2 #31254

Security: fix token bwc with pre 6.0.0-beta2 #31254

Conversation

jaymode commented Jun 11, 2018 • edited Loading

elasticmachine commented Jun 11, 2018

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Jun 11, 2018

Choose a reason for hiding this comment

tvernum commented Jun 12, 2018

jaymode commented Jun 12, 2018

jaymode commented Jun 12, 2018

nik9000 commented Jun 12, 2018

tvernum commented Jun 13, 2018

tvernum left a comment

Choose a reason for hiding this comment

jaymode commented Jun 11, 2018 •

edited

Loading