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

git_pull: sanitize git_pull add-on to avoid deleting /config, fix some nits #3596

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

reynico
Copy link

@reynico reynico commented May 12, 2024

"The risk of complete loss is possible." is a real thing with git_pull! I wiped out my /config folder and found that I am not alone. I did a backup just before putting my hands on git_pull, so it was innocuous for my HA install.

I added some error handling using set e so the script would stop running if an error occurred (like a misconfiguration or a connectivity problem). I also found using ~ a bit misleading when trying to set $HOME; for example, git couldn't find the ssh key when using ~ as $HOME.

Also, I decided to pass the SSH deployment key base64 encoded; otherwise, parsing the single-line SSH key would be a lot of struggle.

Last, the repo is cloned to a fresh folder called /new_config before destroying the working /config one.

Copy link

@home-assistant home-assistant bot left a comment

Choose a reason for hiding this comment

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

Hi @reynico

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft May 12, 2024 01:22
@reynico reynico marked this pull request as ready for review May 12, 2024 01:24
Copy link
Member

@agners agners left a comment

Choose a reason for hiding this comment

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

In general, these are some nice improvements! Thanks 👍

I think I'd suggest to undo the deployment_key_base64_encoded changes here, so we can merge the other improvements, and then tackle the deployment key changes separately.

@@ -1,4 +1,10 @@
# Changelog
# Changelogs
Copy link
Member

Choose a reason for hiding this comment

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

It is only a single log no?

Suggested change
# Changelogs
# Changelog

@@ -12,9 +12,8 @@ arch:
- amd64
- i386
boot: manual
hassio_api: true
homeassistant_api: true
Copy link
Member

Choose a reason for hiding this comment

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

This is not the same API. Do we actually use any of those? 🤔

See: https://developers.home-assistant.io/docs/add-ons/configuration

Copy link
Author

Choose a reason for hiding this comment

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

Uhm, I had many problems when trying to use hassio_api. After reading a bit, I used homeassistant_api, which worked out of the box. Given my ignorance, I thought hassio_api was replaced with homeassistant_api 😄

It's been a while, so I don't exactly recall what my issue was here, but it had to be with the validate-config function.

I can dig deeper with hassio_api and try to make it work if you want!

@@ -28,7 +27,7 @@ options:
- .gitignore
git_command: pull
git_prune: false
deployment_key: []
deployment_key_base64_encoded: ""
Copy link
Member

Choose a reason for hiding this comment

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

Uh, this will break the setup for many people 😢

Can we not make it work with the old config option? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Also, I decided to pass the SSH deployment key base64 encoded; otherwise, parsing the single-line SSH key would be a lot of struggle.

Uh I see. Well but this is weird too, asking the user to base64 encode their private keys yet again. The PEM (-----BEGIN... -----END...) format is already base64 encoded.

Now granted, how the multiple lines are handled here is a bit funky, even in YAML terms. E.g. instead of:

deployment_key:
  - "-----BEGIN RSA PRIVATE KEY-----"
  - MIIEowIBAAKCAQEAv3hUrCvqGZKpXQ5ofxTOuH6pYSOZDsCqPqmaGBdUzBFgauQM
  - xDEcoODGHIsWd7t9meAFqUtKXndeiKjfP0MMKsttnDohL1kb9mRvHre4VUqMsT5F
  - "..."
  - i3RUtnIHxGi1NqknIY56Hwa3id2yk7cEzvQGAAko/t6PCbe20AfmSQczs7wDNtBD
  - HgXRyIqIXHYk2+5w+N2eunURIBqCI9uWYK/r81TMR6V84R+XhtvM
  - "-----END RSA PRIVATE KEY-----"

We could use multiline YAML (note the pipe after the colon):

deployment_key: |
  -----BEGIN RSA PRIVATE KEY-----
  MIIEowIBAAKCAQEAv3hUrCvqGZKpXQ5ofxTOuH6pYSOZDsCqPqmaGBdUzBFgauQM
  xDEcoODGHIsWd7t9meAFqUtKXndeiKjfP0MMKsttnDohL1kb9mRvHre4VUqMsT5F
  ...
  i3RUtnIHxGi1NqknIY56Hwa3id2yk7cEzvQGAAko/t6PCbe20AfmSQczs7wDNtBD
  HgXRyIqIXHYk2+5w+N2eunURIBqCI9uWYK/r81TMR6V84R+XhtvM
  -----END RSA PRIVATE KEY-----

But I guess you want to address the UI representation. I am not aware that we can request multi-line string from the frontend.

So we have a bit unfortunate limitations from the UI side currently 😢

However, what we could do as a work around is using sub-option. Those are displayed as YAML fields. How about getting rid of deployment_key and deployment_key_protocol, and move to:

schema:
  deployment_identity:
    type: match(rsa|dsa|ecdsa|ed25519)
    key: str

Adjust the docs to show an example how to do the multiline string thing:

image

🤔

In any case. This would need a major number bump as it breaks backwards compatibility (and set the breaking_versions config).

@home-assistant home-assistant bot marked this pull request as draft June 21, 2024 10:01
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.

None yet

2 participants