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.

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

Disallow fetching the ticket salt via REST API #7863

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

dnsmichi
Copy link
Contributor

For security reasons, we should disable fetching this credential specific constant.

@lippserd
Copy link
Member

We have to coordinate this with @Thomas-Gelf since afaik the Director makes use of this salt.

@lippserd lippserd added the needs feedback We'll only proceed once we hear from you again label Feb 27, 2020
@dnsmichi
Copy link
Contributor Author

@lippserd Thanks for mentioning, I forgot to copy the commit message into this PR.

The Director currently relies on this object attribute. For this reason
we've implemented /v1/actions/generate-ticket in Icinga 2 v2.5 which only needs
a hostname and hides the ticket salt from the caller.

Needs Icinga/icingaweb2-module-director#401

@Al2Klimov Al2Klimov marked this pull request as draft June 29, 2020 10:10
@Al2Klimov Al2Klimov added this to the 2.13.0 milestone Nov 23, 2020
@julianbrost julianbrost removed this from the 2.13.0 milestone May 31, 2021
@julianbrost
Copy link
Contributor

Well, some time later, this issue was rediscovered and considered more urgent which lead to GHSA-98wp-jc6q-x5q5. Commit 4fca009 is therefore obsolete now. I'd still consider including 6a566ef though.

@dnsmichi
Copy link
Contributor Author

@julianbrost Thanks for finally fixing the security problem after 5 years (Icinga/icingaweb2-module-director#401 #4485). It had been tiring to argue for the change, disallowing auto-signing tricked by agent setups for convenience.

Yet I imagine that there are countless installations out there which have the raw ticket_salt private key fetch mechanism embedded in their scripts, following the bad practice suggested by upstream.

Feel free to cherry-pick the mentioned commit and/or close the PR, I don't have access to this repository anymore.

@Al2Klimov Al2Klimov force-pushed the bugfix/disallow-receiving-ticket-salt-via-api branch from 6a566ef to 4d57de2 Compare January 20, 2023 11:38
@cla-bot
Copy link

cla-bot bot commented Jan 20, 2023

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Michael Friedrich.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

@Al2Klimov Al2Klimov self-requested a review January 20, 2023 11:41
@Al2Klimov Al2Klimov marked this pull request as ready for review January 20, 2023 11:42
@Al2Klimov
Copy link
Member

@bobapple If you can and want to do something about this, please do it. Otherwise feel free not to do it. We all know the situation from law POV and I don’t care for that one red CI result.

@lippserd
Copy link
Member

lippserd commented Jan 20, 2023

Create a new PR with that change and remove this one. Or reset the author.

Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

curl -ksSu root:123456 -H 'Accept: application/json' https://127.0.0.1:5665/v1/variables\?pretty\=1 |grep TicketSalt

Before

            "name": "TicketSalt",

After

@Al2Klimov Al2Klimov added this to the 2.14.0 milestone Jan 20, 2023
@Al2Klimov Al2Klimov removed the needs feedback We'll only proceed once we hear from you again label Jan 20, 2023
@Al2Klimov Al2Klimov changed the title WIP: Disallow fetching the ticket salt via REST API Disallow fetching the ticket salt via REST API Jan 20, 2023
@Al2Klimov
Copy link
Member

Even resetting the author so that GitHub recognises him doesn’t help (not to mention .mailmap): Al2Klimov#15

Not the first time we ignore some checks knowing that actually everything is OK. 🤷‍♂️

@Al2Klimov Al2Klimov merged commit bb99106 into master Jan 25, 2023
@icinga-probot icinga-probot bot deleted the bugfix/disallow-receiving-ticket-salt-via-api branch January 25, 2023 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants