Replace password in URI by stars if present to avoid leaking secrets in logs

[mickours]

Currently, if you use a git URI containing a password like https://username:pass@git.example.com/project and an exception occurs during a git clone the password is printed in the logs. It also happen when the logging is at debug level.

To avoid secrets to leak in the logs, this patch replace the password by a fix amount of stars (******).

[Byron]

Thanks a lot for the initiative, it's much appreciated.

My early feedback is that the URL parsing is probably prone to failures even though it should work for most URLs. To be save, one would probably catch parsing exceptions.
Maybe it would be easier to use an existing URL parser and/or serializer if one exists in the python standard library.

[mickours]

Thanks for the feedback! I've change the implementation to use urllib and handle parsing errors.

I'm still unable to make the test works on the CI despite the fact that it works locally. It seems that the failing command raise an assert which is not in my test. The problem is that I need the exception to be raise in order to check if the password is inside. Any clue on that?

[Byron]

Thanks for the update!

The failing test mentions the entire command-line, here it is:

 /usr/bin/git clone -v --progress ***fakerepo.example.com/testrepo /tmp/test_leaking_password_in_clone_logsam5xqh6q'

It looks like a replacement has happened even though I would expect an additional *, the password has been removed but it still seems to see the password in the error message. Since finalize_proc is only handed the process proc, this really only works if the args list is editable. The latter might be the case in some python versions, but not in others.

Something you could try is to reassign proc.args entirely. If that works, I have a few more requests just to be very very safe 😅.

[Byron]

Thanks a lot for bearing with me, and good luck with the CI issue. The test succeeds for me as well when running it locally.

[mickours]

I track down every occurrence of the command line in the logs and it seems to be OK now. With this patch it will on every commands try to redact password out of the command line before logging it.

[mickours]

I just realize that we do not have a succesfull clone test and when I tried it manually it's not working because of the in-place replacement. I've to rework this.

[mickours]

Ok, I've change the in-place password replacement with a copy implementation which avoid side effects on the command line arguments. Also, I've added a successful git clone with a password from an empty project.

[Byron]

Thanks a lot, this looks so much better already, great work! It's particularly helpful to see more tests, as now there is one in particular for redacting passwords in command-lines. I bet this helped during development.

Please excuse the use of the word 'nicer' in the line comments below, I simply lacked a more scientific explanation so I guess it comes down to 'taste'. This is not to be understood as judgement towards your code, it's just as correct and since it's Python you could make a point towards the current version being idiomatic and I would go with it.

Let's get this one to the finishing line :) 🏇

[Byron]

This might be a danger as it breaks the self-isolation even more (after all, GitPythons test rely on its own repository already), but I think it's OK to go with it and fix it when it breaks.
Maybe it never does.

[mickours]

I agree! I simply did not find another way to test the working use case. Maybe we can use a deploy token with the GitPython repository on Github but I think it is not available for public repo. Maybe you can create a private repo inside the gitpython-developers account in order to keep the responsibility in the same place?

[Byron]

I thought about that too and will do that when it breaks. It's certainly the lazy way of doing things and it's basically a mine that is primed to blow sometime in the future, but… it's going to be okay ;).

While writing this I thought to myself that I don't want to be that lazy and took a look at the github ways of doing this. It turns out it only supports repository deploy keys, which indeed are full blown ssh keys that I don't want to deal with right now. This makes your account one of the pillars of the happiness of GitPython's CI, something not everyone can say about themselves :D.

[mickours]

Thanks a lot, this looks so much better already, great work! It's particularly helpful to see more tests, as now there is one in particular for redacting passwords in command-lines. I bet this helped during development.

You're right! :)

Please excuse the use of the word 'nicer' in the line comments below, I simply lacked a more scientific explanation so I guess it comes down to 'taste'. This is not to be understood as judgement towards your code, it's just as correct and since it's Python you could make a point towards the current version being idiomatic and I would go with it.

No problem, I know that sometimes it's a matter of beauty ;)

Let's get this one to the finishing line :) horse_racing

Thanks a lot for all you feedback!! 🏁

[Byron]

And it's done :)! Thanks again, I think it was worth it, too!