GH App private key can be printed to debug logs if one made mistake in helm values or intentionally removed every newline character in the pem file

[stefangluszek]

Describe the bug
When GitHub Client authentication fails the private key can end up being printed in full plain text form in debug logs.

Splitting the private key string by new line can be dangerous, consider a simple mistake of putting the content of the PEM file in github_app_private_key but with stripped new lines. We would end up with the entire key being printed in the debug logs.

Checks

• My actions-runner-controller version (v0.x.y) does support the feature
• I'm using an unreleased version of the controller I built from HEAD of the default branch

To Reproduce
Steps to reproduce the behavior:

1. Create the controller-manager secret:

$ kubectl create secret generic controller-manager \
    -n actions-runner-system \
    --from-literal=github_app_id=app1337 \
    --from-literal=github_app_installation_id=installation1338 \
    --from-literal=github_app_private_key=THIS_IS_SECRET_NEVER_PRINT

2. Use the above secret with actions-runner-controller and watch THIS_IS_SECRET_NEVER_PRINT being printed in the pod debug logs

Error: Client creation failed. authentication failed: using private key of size 26 (THIS_IS_SECRET_NEVER_PRINT...): could not parse private key: Invalid Key: Key must be PEM encoded PKCS1 or PKCS8 private key

Expected behavior
Never print the App private key to debug. The hash of the string should be printed, or the string should be replaced by ***** literal.

Environment (please complete the following information):

• Controller Version [0.21.1]
• Deployment Method [Helm ]
• Helm Chart Version [0.16.1]

Additional context
Add any other context about the problem here.

[mumoshu]

@stefangluszek Hey! Thanks for reporting. I'm just confused, but how did you confirm this in a real scenario? In a standard case your private key pem data would start with -----BEGIN RSA PRIVATE KEY----- and printing that won't be a big issue, right? 🤔

[stefangluszek]

@stefangluszek Hey! Thanks for reporting. I'm just confused, but how did you confirm this in a real scenario? In a standard case your private key pem file would start with -----BEGIN RSA PRIVATE KEY----- and printing that won't be a big issue, right? thinking

@mumoshu I made a mistake in my helm values and used >- instead of |

authSecret:
    github_app_private_key: >-
        ---BEGIN RSA PRIVATE KEY----- ...

One could argue that it user error, on the other hand I don't think printing any part of k8s secret strings into debug logs is advisable.

[mumoshu]

@stefangluszek Hmm yeah, that's definitely a good point. It would be great if I could make it human-mistake proof :)

FWIW, the background of the current implementation is that I've been asked to troubleshoot why one's github app auth is not working for ARC, dozens of times, and I was tired of that!

I added everything I thought needed to troubleshoot it by themselves. BTW, I found three cases in the causes of their problems. One missed using | in the helm value and that resulted in only first line of the app key was passed to ARC. Another had extra characters in the key content. The third case was due to that one had fed a completely different file to it.. The intent was to help all those three cases.

If we put a hash of the possibly incorrect app key in the log, can I expect everyone to read it correctly? Does everyone know how to compute a hash sum of their own key? 🤔

Well but apparently, the current implementation failed to point out your issue (usage of >-). So the current implementation isn't perfect either. Perhaps we're good to change the current behavior once we find another method that is generally better, although not perfect.