Skip to content

Check feature flag before registering JWT settings#84740

Merged
elasticsearchmachine merged 6 commits intoelastic:masterfrom
tvernum:feature/jwt/feature-flag-settings
Mar 18, 2022
Merged

Check feature flag before registering JWT settings#84740
elasticsearchmachine merged 6 commits intoelastic:masterfrom
tvernum:feature/jwt/feature-flag-settings

Conversation

@tvernum
Copy link
Copy Markdown
Contributor

@tvernum tvernum commented Mar 7, 2022

The JWT feature flag would control whether the JWT realm was defined
as a configurable internal realm, but was not checked before the JWT
realm settings were registered.

This means that it was possible to defined a JWT realm in YML even if
the realm wasn't enabled in this build

This change adds the feature flag check to the settings registration

The JWT feature flag would control whether the JWT realm was defined
as a configurable internal realm, but was not checked before the JWT
realm settings were registered.

This means that it was possible to defined a JWT realm in YML even if
the realm wasn't enabled in this build

This change adds the feature flag check to the settings registration
@tvernum tvernum added >non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) test-release Trigger CI checks against release build v8.2.0 labels Mar 7, 2022
@tvernum tvernum requested a review from justincr-elastic March 7, 2022 22:57
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Mar 7, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Mar 7, 2022

The release tests will fail until #84738 is merged

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Mar 9, 2022

@elasticmachine update branch

@justincr-elastic
Copy link
Copy Markdown
Contributor

Can you confirm there are no other missing checks?

It might be worthwhile to set XpackSettings.JWT_REALM_FEATURE_FLAG_ENABLED=false locally and run all tests, or put that in a throwaway PR and let CI tests verify it.

After you confirm no other changes, I will update #84845 for removing this feature flag.

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Mar 14, 2022

@elasticmachine update branch

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Mar 14, 2022

It might be worthwhile to set XpackSettings.JWT_REALM_FEATURE_FLAG_ENABLED=false locally and run all tests

That doesn't do anything. You can only turn on the feature flag - you can't turn it off.

The release-tests are what we use to test with the feature flag disabled, but triggering a failure because a setting was registered when it wasn't expected doesn't happen automatically.

@tvernum tvernum added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 18, 2022
@elasticsearchmachine elasticsearchmachine merged commit 4e02d6f into elastic:master Mar 18, 2022
@tvernum tvernum deleted the feature/jwt/feature-flag-settings branch March 18, 2022 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team test-release Trigger CI checks against release build v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants