Skip to content

Improve the key validation in secret identifier. #17351

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
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Improve the key validation in secret identifier. #17351

wants to merge 6 commits into from

Conversation

mashhurs
Copy link
Contributor

@mashhurs mashhurs commented Mar 19, 2025 •
edited
Loading

Release notes

[rn:skip]

What does this PR do?

Before this change, key-value can be added to keystore but it might impossible to reference the key because ConfigVariableExpander ignores if key name doesn't align with SUBSTITUTION_PLACEHOLDER_REGEX
Improves user experience with the secret store CLI that when providing key name, LS validates to accept a value which can be used in pipeline (aligns with ConfigVariableExpander#SUBSTITUTION_PLACEHOLDER_REGEX).
If already invalid key(s) were added, it warns to remove/replace the keys.

Why is it important/What is the impact to the user?

No user impact. Keystore CLI restricts adding meaningless keys to keystore.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] I have made corresponding change to the default configuration files (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@mergify Mergify
Copy link

mergify bot commented Mar 19, 2025

This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit.
  • backport-8.x is the label to automatically backport to the 8.x branch.
  • If no backport is necessary, please add the backport-skip label

@mashhurs mashhurs self-assigned this Mar 19, 2025
@mashhurs mashhurs added backport-active-9 Automated backport with mergify to all the active 9.[0-9]+ branches backport-active-8 Automated backport with mergify to all the active 8.[0-9]+ branches labels Mar 19, 2025
@mashhurs mashhurs marked this pull request as ready for review March 19, 2025 16:00
@yaauie
Copy link
Member

yaauie commented Mar 19, 2025

No users should be impacted unless they are using restricted symbols (["?", "..", "/", "\", "'", """, "$", "*", "|", "<", ">", " "]) in the key.

This sounds like a breaking change.

  • What was the prior experience when using a key that includes now-restricted characters?
  • What determined that these specific characters are restricted (while allowing a bunch of non-printing characters in the lower-ascii plane and the gamut of unicode characters above the lower-ascii plane)?

@github-actions GitHub Actions
Copy link
Contributor

📃 DOCS PREVIEW ✨ https://logstash_bk_17351.docs-preview.app.elstc.co/diff

@mashhurs
Copy link
Contributor Author

No users should be impacted unless they are using restricted symbols (["?", "..", "/", "", "'", """, "$", "*", "|", "<", ">", " "]) in the key.

This sounds like a breaking change.

  • What was the prior experience when using a key that includes now-restricted characters?
  • What determined that these specific characters are restricted (while allowing a bunch of non-printing characters in the lower-ascii plane and the gamut of unicode characters above the lower-ascii plane)?

Yeah, didn't think of the case if keystore had invalid keys already included.
a3372c5 commit changes the approach:

  • to restrict adding invalid keys to keystore with CLI. Invalid means here outside of ConfigVariableExpander#SUBSTITUTION_PLACEHOLDER_REGEX ;
  • warns/guides users if their keystore already contains invalid keys;

@mashhurs mashhurs requested a review from yaauie March 24, 2025 15:32
yaauie
yaauie reviewed Apr 8, 2025
View reviewed changes
Copy link
Member

@yaauie yaauie left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Mechanically, this works, and safely guards against adding keys that cannot be referenced in the pipelines without breaking users whose keystores include offending keys.

I've left suggestions about DRY-ing it up a bit, and improving the admitedly-difficult description of the effective key pattern requirement.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

📃 DOCS PREVIEW ✨ https://logstash_bk_17351.docs-preview.app.elstc.co/diff

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

📃 DOCS PREVIEW ✨ https://logstash_bk_17351.docs-preview.app.elstc.co/diff

@mashhurs mashhurs force-pushed the secret-identifier-key-validator branch from 9139460 to 6ac79ce Compare April 9, 2025 18:13
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

📃 DOCS PREVIEW ✨ https://logstash_bk_17351.docs-preview.app.elstc.co/diff

mashhurs and others added 4 commits April 9, 2025 13:33
@mashhurs
Improve the key validation in secret indentifier.
2f205c1
@mashhurs
Restrict adding invalid (from ConfigVariableExpander point of view) k…
440e894 
…ey names to the keystore through keystore CLI. Keysotre now warns on invalid keys if they were already added.
@mashhurs @yaauie
Apply suggestions from code review
3f6f66d 
[doc] better explanation of accepting key format.

Co-authored-by: Rye Biesemeyer <[email protected]>
@mashhurs @yaauie
Key pattern is moved to a central config expander class with its desc…
bfea29d 
…ription.

Co-authored-by: Rye Biesemeyer <[email protected]>
@mashhurs mashhurs force-pushed the secret-identifier-key-validator branch from 6ac79ce to bfea29d Compare April 9, 2025 20:33
@mashhurs
Copy link
Contributor Author

mashhurs commented Apr 9, 2025

Rebased against main as we were having CI issues, fixed by #17531

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 9, 2025

📃 DOCS PREVIEW ✨ https://logstash_bk_17351.docs-preview.app.elstc.co/diff

@mashhurs mashhurs requested a review from yaauie April 9, 2025 21:04
yaauie
yaauie reviewed Apr 10, 2025
View reviewed changes
Copy link
Member

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

👍🏼 looking on-track.

I've left a comment about message format, and suggested more useful random key generation for tests.

@mashhurs @yaauie
Apply suggestions from code review
8e0136a 
Warning and error messages well formatted.

Co-authored-by: Rye Biesemeyer <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

📃 DOCS PREVIEW ✨ https://logstash_bk_17351.docs-preview.app.elstc.co/diff

@github-actions GitHub Actions
Copy link
Contributor

📃 DOCS PREVIEW ✨ https://logstash_bk_17351.docs-preview.app.elstc.co/diff

@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mashhurs

@mashhurs mashhurs requested a review from yaauie April 11, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yaauie yaauie Awaiting requested review from yaauie

At least 1 approving review is required to merge this pull request.

Assignees

@mashhurs mashhurs

Labels
backport-active-8 Automated backport with mergify to all the active 8.[0-9]+ branches backport-active-9 Automated backport with mergify to all the active 9.[0-9]+ branches
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mashhurs @yaauie @elasticmachine