TheSpaghettiDetective / moonraker-obico

GNU Affero General Public License v3.0
153 stars 41 forks source link

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

Open whi-tw opened 1 year ago

whi-tw commented 1 year ago

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.

kennethjiang commented 1 year ago

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

whi-tw commented 1 year ago

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 commented 1 year ago

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

kennethjiang commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.