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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[enh] Redact secrets from logs #742

Merged
merged 11 commits into from Jul 2, 2019

Conversation

Projects
None yet
4 participants
@alexAubin
Copy link
Member

commented Jun 19, 2019

The problem

c.f. YunoHost/issues#1363

Note when working on this, I realized that the "python argument" case is already covered, c.f. these :
https://github.com/YunoHost/yunohost/blob/stretch-testing/src/yunohost/log.py#L224
https://github.com/YunoHost/yunohost/blob/stretch-testing/src/yunohost/user.py#L274

Solution

So here's an attempt to redact at least most of the sensitive / secret data in the logs. So far I was able to cover :

  • "static" secrets like /etc/yunohost/mysql
  • secrets provided through the install form (password-type args)
  • secrets generated or fetched(!) during the script execution, like mysql db password.

The last item is the least easiest and least "clean" one. It's all built on the way bash logs or not information ... So for example, we are somewhat lucky that this line only results in showing the secret for the first time as :

local new_db_pwd=the_super_secret

(and there's no weird intermediate output already showing the secret). Otherwise, it would be much more difficult to catch and we might end up having to rewrite already written stuff in the file which would complicates stuff (though not impossible but ugh)

So the current mechanism relies on catching matches for this regex : (db_pwd|password)=(\S{3,})$

So for example : + local new_db_pwd=pT0QepguMxajbmQE5Ad4ZPgd

and therefore covers mysql and postgresql dbs created with the standard helpers. This also covers getting passwords from the setting file, as done with e.g. upload_password=$(ynh_app_setting_get --app=$app --key=upload_password) (as long as the variable name ends with password ...). So there are corner cases that might not be covered like if the variable is instead named admin_pass... To be discussed.

Hopefully that's a relatively understandable explanation 馃槄

PR Status

Tested on the app install of my_webapp on my side ... seems to be kinda working

How to test

Run some app install (and also remove, upgrade, ... any special case that could show a password or other info we would want to redact)

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Jun 19, 2019

(Had to change the output format of _parse_args_from_manifest to also fetch the info of arg type, which makes the whole thing a bit more complicated, but there was no obvious way to do this in a simple way :/ )

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Hello,

We should coordinate better because I also worked on this, so I think there are enough work to don't do two time the same things...

Anyway thanks for your work, it might be better than what I wanted to do.

Show resolved Hide resolved src/yunohost/log.py Outdated
Show resolved Hide resolved src/yunohost/log.py Outdated
@decentral1se
Copy link
Contributor

left a comment

Thanks for working on this 馃敟 馃敟 馃敟

Show resolved Hide resolved src/yunohost/log.py Outdated
@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

We should coordinate better because I also worked on this, so I think there are enough work to don't do two time the same things...

Yea sorry about that... In the end of the meeting it wasn't so clear who was taking care of it and I already had many ideas in mind ... But clearly should have focused on other todo's. We'll try to be more careful next times :/

Thanks for speaking out

alexAubin and others added some commits Jun 27, 2019

[doc] typo
Co-Authored-By: decentral1se <lukewm@riseup.net>
@Psycojoker

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Untested but from here it looks good, I did a quick review and that seems pretty much ok.

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

So uh what do you think folks, shall we include this in the upcoming 3.6 testing release, or do you think if should be tested more deeply before being released ?

@decentral1se

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

go go go go go go go go go go go go

@alexAubin alexAubin added this to the 3.6.x milestone Jul 2, 2019

@alexAubin alexAubin merged commit 3c0e0ca into stretch-unstable Jul 2, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@alexAubin alexAubin deleted the redact-secrets-from-logs branch Jul 2, 2019

@alexAubin

This comment has been minimized.

Copy link
Member Author

commented Jul 2, 2019

Small note before I forget : this PR covered the operation logs (which are the ones usually shared with yunopaste on the forum) but doesn't cover the yunohost-cli and yunohost-api logs which might contain sensitive data as well

@Josue-T

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2019

Small note before I forget : this PR covered the operation logs (which are the ones usually shared with yunopaste on the forum) but doesn't cover the yunohost-cli and yunohost-api logs which might contain sensitive data as well

Well, for the yunohost-cli and yunohost-api, what would be the solution. Will we need to make an other PR ? I started a work which fix also that. This work is still useful ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.