Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Watcher: Ensure correct method is used to read secure settings #31753

Merged
merged 1 commit into from
Jul 4, 2018
Merged

Watcher: Ensure correct method is used to read secure settings #31753

merged 1 commit into from
Jul 4, 2018

Conversation

spinscale
Copy link
Contributor

As SecureSetting is extended from Setting, you can easily accidentally
use SecureSetting.simpleString() to read a secure setting instead of
SecureSetting.secureString(). This commit changes this behaviour in
some watcher notification services.

@spinscale
Watcher: Ensure correct method is used to read secure settings
9581367 
As SecureSetting is extended from Setting, you can easily accidentally
use `SecureSetting.simpleString()` to read a secure setting instead of
`SecureSetting.secureString()`. This commit changes this behaviour in
some watcher notification services.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

cbuescher
cbuescher approved these changes Jul 3, 2018
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just wondering if it would make sense to overwrite SecureString#simpleString to throw an exception to prevent accidental usages. But maybe there are legitimate uses of it?

@rjernst
Copy link
Member

rjernst commented Jul 3, 2018

would make sense to overwrite SecureString#simpleString

This (and all the other helper methods for constructing settings) is a static method, so it cannot be overridden.

rjernst
rjernst approved these changes Jul 3, 2018
View reviewed changes
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change, but can the duplicate setting objects also be removed? eg HipChatAccount.SECURE_AUTH_TOKEN_SETTING. Can all those use the affix setting instead of having duplicate secure settings without the prefix?

@spinscale
Copy link
Contributor Author

This requires more changes in the NotificationService the account creation in that method uses grouped account-only settings, but we need to supply all the settings to make this work properly with affix settings.

Valid point though, I'll create a follow up issue for that.

@spinscale spinscale merged commit 4328470 into elastic:master Jul 4, 2018
spinscale added a commit that referenced this pull request Jul 4, 2018
@spinscale
Watcher: Ensure correct method is used to read secure settings (#31753)
c268e0b 
As SecureSetting is extended from Setting, you can easily accidentally
use `SecureSetting.simpleString()` to read a secure setting instead of
`SecureSetting.secureString()`. This commit changes this behaviour in
some watcher notification services.
dnhatn added a commit that referenced this pull request Jul 5, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
7d4a74e 
* 6.x:
  Test: Do not remove xpack templates when cleaning (#31642)
  SQL: Allow long literals (#31777)
  SQL: Fix incorrect message for aliases (#31792)
  Detach Transport from TransportService (#31727)
  6.3.1 release notes (#31829)
  Add unreleased version 6.3.2
  [ML][TEST] Use java 11 valid time format in DataDescriptionTests (#31817)
  [ML] Don't treat stale FAILED jobs as OPENING in job allocation (#31800)
  [ML] Fix calendar and filter updates from non-master nodes (#31804)
  Fix license header generation on Windows (#31790)
  mark XPackRestIT.test {p0=monitoring/bulk/10_basic/Bulk indexing of monitoring data} as AwaitsFix
  Add JDK11 support without enabling in CI (#31644)
  Watcher: Fix check for currently executed watches (#31137)
  [DOCS] Fixes 6.3.0 release notes (#31771)
  Watcher: Ensure correct method is used to read secure settings (#31753)
  [ML] Rate limit established model memory updates (#31768)
  SQL: Update CLI logo
dnhatn added a commit that referenced this pull request Jul 5, 2018
@dnhatn
Merge branch 'master' into ccr
6dd3434 
* master:
  REST high-level client: add get index API (#31703)
  SQL: Allow long literals (#31777)
  SQL: Fix incorrect message for aliases (#31792)
  Test: Do not remove xpack templates when cleaning (#31642)
  Reduce more raw types warnings (#31780)
  Add unreleased version 6.3.2
  Scripting: Remove support for deprecated StoredScript contexts (#31394)
  [ML][TEST] Use java 11 valid time format in DataDescriptionTests (#31817)
  [ML] Don't treat stale FAILED jobs as OPENING in job allocation (#31800)
  [ML] Fix calendar and filter updates from non-master nodes (#31804)
  Fix license header generation on Windows (#31790)
  mark RollupIT.testTwoJobsStartStopDeleteOne as AwaitsFix
  mark SearchAsyncActionTests.testFanOutAndCollect as AwaitsFix
  Correct exclusion of test on JDK 11
  Fix doclint jdk 11
  Add JDK11 support and enable in CI (#31644)
  Watcher: Fix check for currently executed watches (#31137)
  Watcher: Ensure correct method is used to read secure settings (#31753)
  SQL: Update CLI logo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbuescher cbuescher cbuescher approved these changes

@rjernst rjernst rjernst approved these changes

Assignees
No one assigned
Labels
:Data Management/Watcher >non-issue v6.4.0 v7.0.0-beta1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@spinscale @elasticmachine @rjernst @cbuescher @colings86