-
Notifications
You must be signed in to change notification settings - Fork 24.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding a warning header when a license is about to expire #64948
Conversation
@elasticmachine update branch |
@elasticmachine update branch |
@elasticmachine run elasticsearch-ci/bwc |
1 similar comment
@elasticmachine run elasticsearch-ci/bwc |
@elasticmachine update branch |
@elasticmachine update branch |
@elasticmachine update branch |
…on to the responses of _security/oidc/authenticate and _security/oidc/authenticate APIs Resolves elastic#53161
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments. I also think we need to elaborate on the PR description ( it's not clear what through SECURITY_IP_FILTER
means ) and add a couple of sentences on why we add the check on checkFeature
and more especially why we think this is enough for solving the issue this PR is meant to.
We also need some tests that ensure we are indeed adding the warning header under the circumstances we expect it to be added
x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
Outdated
Show resolved
Hide resolved
Pinging @elastic/es-security (Team:Security) |
…aml/metadata/{realm} Related to elastic#49018
@elasticmachine update branch |
@elasticmachine update branch |
x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
Outdated
Show resolved
Hide resolved
.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java
Show resolved
Hide resolved
messages; changing "today" calculation; adding a test case for failing authN to make sure we remove the warning header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I'd like to hear from @tbrooks8 before we merge.
(and if we can switch to assertThat
, that would be good too)
...k/plugin/security/src/internalClusterTest/java/org/elasticsearch/license/LicensingTests.java
Outdated
Show resolved
Hide resolved
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some nits suggestions and a question regarding if removing all warning headers on authentication failure is intentional
.../plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java
Show resolved
Hide resolved
x-pack/plugin/core/src/test/java/org/elasticsearch/test/SecuritySettingsSourceField.java
Show resolved
Hide resolved
...k/plugin/security/src/internalClusterTest/java/org/elasticsearch/license/LicensingTests.java
Show resolved
Hide resolved
...k/plugin/security/src/internalClusterTest/java/org/elasticsearch/license/LicensingTests.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java
Show resolved
Hide resolved
...k/plugin/security/src/internalClusterTest/java/org/elasticsearch/license/LicensingTests.java
Outdated
Show resolved
Hide resolved
...k/plugin/security/src/internalClusterTest/java/org/elasticsearch/license/LicensingTests.java
Outdated
Show resolved
Hide resolved
@jkakavas Yes, it is intentional (see Tim's comment above). If Authentication fails it does make sense to not reveal any additional information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@elasticmachine update branch |
) * This change adds a warning header when a license is about to expire Resolves elastic#60562 * This change adds realm name of the realm used to perform authentication to the responses of _security/oidc/authenticate and _security/oidc/authenticate APIs Resolves elastic#53161 * Adding doc for the new API introduced by elastic#64517 - /_security/saml/metadata/{realm} Related to elastic#49018 * Adding a warning header when a license is about to expire Resolves elastic#60562 * Addressing the PR feedback * Switching back to adding the header during featureCheck to allow warnings when authentication is disabled as well. Adding filterHeader implementation to SecurityRestFilter exception handling to remove all the warnings if authentication fails. * Changing the wording for "expired" message to be consistent with the log messages; changing "today" calculation; adding a test case for failing authN to make sure we remove the warning header * Small changes in the way we verify header in tests * Nit changes Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
) * Adding a warning header when a license is about to expire (#64948) * This change adds a warning header when a license is about to expire Resolves #60562 * This change adds realm name of the realm used to perform authentication to the responses of _security/oidc/authenticate and _security/oidc/authenticate APIs Resolves #53161 * Adding doc for the new API introduced by #64517 - /_security/saml/metadata/{realm} Related to #49018 * Adding a warning header when a license is about to expire Resolves #60562 * Addressing the PR feedback * Switching back to adding the header during featureCheck to allow warnings when authentication is disabled as well. Adding filterHeader implementation to SecurityRestFilter exception handling to remove all the warnings if authentication fails. * Changing the wording for "expired" message to be consistent with the log messages; changing "today" calculation; adding a test case for failing authN to make sure we remove the warning header * Small changes in the way we verify header in tests * Nit changes Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * Resolving backporting issue: adding copyMapWithRemovedEntry() util function Fixing unused imports Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
This implementation will add the warning header
if the license is going to expire in less than
{
LICENSE_EXPIRATION_WARNING_PERIOD
} days.The messages added:
Warning: 299 Elasticsearch-8.0.0-###"Your license will expire in [N] days. Contact your administrator or update your license for continued use of features"
or
Warning: 299 Elasticsearch-8.0.0-### "Your license expires today. Contact your administrator or update your license for continued use of features"
If license has expired less than
{
GRACE_PERIOD_DURATION
} days ago followingwarning is added:
Warning: 299 Elasticsearch-8.0.0-### "Your license expired on ["EEEE, MMMM dd, yyyy" ]. Contact your administrator or update your license for continued use of features"
Both {
LICENSE_EXPIRATION_WARNING_PERIOD
}and {
GRACE_PERIOD_DURATION
} are currently 7 days.The message will be added to each request unless
authentication fails.
Note: with this change all warning headers will be removed
from a response if authentication fails.
Resolves #60562