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

Hardcoded secret in config file should be better advertised / excluded from vcs by default #53

Open
whi-tw opened this issue Jan 2, 2023 · 9 comments

Comments

@whi-tw
Copy link

whi-tw commented Jan 2, 2023

As there's a secret in moonraker-obico.cfg, perhaps the installer should also add (or append to) a .gitignore to ensure this file never ends up in VCS. Fortunately I was setting up gitleaks and spotted this.

Obviously it was my fault for not checking what I was pushing, but this shouldn't be such an easy mistake to make!

Also, I have rolled the token.

@whi-tw whi-tw changed the title Hardcoded secret in config file should be better advertised Hardcoded secret in config file should be better advertised / excluded from vcs by default Jan 2, 2023
@whi-tw whi-tw closed this as completed Jan 2, 2023
@whi-tw whi-tw reopened this Jan 2, 2023
@kennethjiang
Copy link
Contributor

Not sure what you mean. moonraker-obico.cfg is not supposed to be in a folder that is git-controlled.

@whi-tw
Copy link
Author

whi-tw commented Jan 3, 2023

Not sure what you mean. moonraker-obico.cfg is not supposed to be in a folder that is git-controlled.

Many people vcs their klipper config directory. By default, the installer drops moonraker-obico.cfg in that directory (adjacent to moonraker.cfg)

Searching the file name on GitHub shows that a fair few people have managed to commit this file.

Poking around at the Obico API, this token has more power than I'd have thought it would have.

@whi-tw
Copy link
Author

whi-tw commented Jan 3, 2023

Happy to elaborate via email, rather than in public GitHub comments - email address on my GitHub profile.

@kennethjiang
Copy link
Contributor

Sorry for the delay. Just back from a vacation.

You brought up a good point. This does sound like a real issue if people version control their config folder.

What do you is a good solution? I'd imagine other tools like moonraker, crowdnest, etc may also have a similar problem if some of they also need some kind of secrets in the config file? How do they handle that?

Let's discuss this issue here. I don't think we need to keep the discussion secret as long as none of us post a real token in the comments.

@whi-tw
Copy link
Author

whi-tw commented Jan 10, 2023

Sorry for the delay. Just back from a vacation.

No trouble - you replied hours before I'm gonna be offline for a week!

You brought up a good point. This does sound like a real issue if people version control their config folder.

Yeah, definitely. I had a half-assed play around with the API and it seems like this could definitely be misused to force a crash or deauth. Also, if foreign-key traversal is possible, it could get messy (I haven't used django in a while, so unsure if this is a valid concern)

What do you is a good solution? I'd imagine other tools like moonraker, crowdnest, etc may also have a similar problem if some of they also need some kind of secrets in the config file? How do they handle that?

Moonraker has the [secrets] config option, but that's not accessible outside of moonraker.

I guess that could be implemented, or, alternatively, simply ensuring that the config is .gitignore'd at install time may work as a first pass.

Let's discuss this issue here. I don't think we need to keep the discussion secret as long as none of us post a real token in the comments.

Sure, I just didn't want to potentially draw too much attention to this and have people mining GitHub for credentials! Also, I planned on detailing the potential misuse methods, but I'm sure you can imagine them based on the available API endpoints this token allows access to, and the operations they allow on the various models.

@kennethjiang
Copy link
Contributor

I guess that could be implemented, or, alternatively, simply ensuring that the config is .gitignored at install time may work as a first pass.

Do you mean adding the config path in the .gitignored in this repo?

@whi-tw
Copy link
Author

whi-tw commented Jan 24, 2023

Sorry, I was offline for another week.

Do you mean adding the config path in the .gitignored in this repo?

I really just mean that as part of the install script, we do something like:

CONFIG_GITIGNORE_PATH="${CONFIG_DIR}/.gitignore"
test -f "${CONFIG_GITIGNORE_PATH}" || touch "${CONFIG_GITIGNORE_PATH}"
grep 'moonraker-obico.cfg' "${CONFIG_GITIGNORE_PATH}" || echo 'moonraker-obico.cfg' >> "${CONFIG_GITIGNORE_PATH}"

(obviously this is pseudo-bash, but this is the kind of logic I'm imagining)

This, as a first pass, would at least make it harder to accidentally commit the file

@kennethjiang
Copy link
Contributor

I see what you mean now. But I am hesitating to add a piece of code like this.

Is there any discussion/code that shows managing klipper_config folder in a public git repo is a moonraker/klipper standard or convention?

@zellneralex
Copy link
Contributor

Moonraker has a Methode of a secret file see https://moonraker.readthedocs.io/en/latest/configuration/#secrets

I am not sure if you can automate to get it in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants