Bug #6827
closed
arvados-git-httpd prints plaintext user passwords to its output
Description
sudo -u git arvados-git-httpd -address=:9001 -git-command="$(which git)" -repo-root=/opt/arvados_git/repositories
2015/07/30 18:59:49 Listening at [::]:9001
2015/07/30 18:59:49 Repository root /opt/arvados_git/repositories
2015/07/30 19:41:45 "127.0.0.1:39585" "" "" 401 "no credentials provided" "" "GET" "/arvados.git/info/refs"
2015/07/30 19:42:03 "127.0.0.1:39671" "" "" 401 "no credentials provided" "" "GET" "/arvados.git/info/refs"
2015/07/30 19:42:03 MakeArvadosClient: Missing required environment variable ARVADOS_API_HOST
2015/07/30 19:42:03 "127.0.0.1:39672" "jr17@sanger.ac.uk" "my_ldap_password" 500 "connection pool failed" "arvados" "GET" "/arvados.git/info/refs"
(actual password redacted for obvious reasons)
This is particularly bad in our case, in which we are using LDAP for auth so it is spewing our actual systemwide password to the server logs!
Updated by Brett Smith over 8 years ago
- Target version set to 2015-08-19 sprint
- Story points set to 0.5
Updated by Joshua Randall over 8 years ago
Ok, so mostly this is me being stupid because I hadn't realised that I was supposed to send my arvados username and api token rather than my actual SSO username/password to arvados-git-httpd.
After reading Tom's updated documentation in issue 6663, that suddenly became clear to me.
Leaking Arvados API tokens to the log is probably not so bad, but I suspect that log will end up accidentlaly containing a lot of actual user passwords (because git does prompt for 'password').
Perhaps you could check the provided API token to verify that it appears to be in the shape of an API token, and if so it could be printed.. otherwise assume it might be some other password and omit it? I suspect not very many end users will have 50 character passwords...
Updated by Tom Clegg over 8 years ago
Joshua Randall wrote:
Perhaps you could check the provided API token to verify that it appears to be in the shape of an API token, and if so it could be printed.. otherwise assume it might be some other password and omit it? I suspect not very many end users will have 50 character passwords...
Perhaps we should log only the first few characters of the token if we get as far as getting an API response and the API server response indicates it's a valid token, otherwise the string "<invalid>"?
(The first few characters should be enough to cross-reference for troubleshooting purposes, without being enough to cause any trouble if the log file is leaked.)
Updated by Tom Clegg over 8 years ago
- Assigned To changed from Tom Clegg to Radhika Chippada
Updated by Radhika Chippada over 8 years ago
- Status changed from New to In Progress
Updated by Radhika Chippada over 8 years ago
3959d7afff8bb3c3b8da9eb7d178919275180f2a
Per Joshua Randall's and Tom's comments above:- Log only when it is a valid token
- And, even then, only log the first 10 characters of that valid token
- Also, added a two more tests. One with an invalid / non-existing token, and another with an expired token.
Updated by Nico César over 8 years ago
I'm reviewing 3959d7afff8bb3c3b8da9eb7d178919275180f2a
After reading the issue. It's unclear to me from the code that is the token being truncated to the first 10 characters.
Probably renaming "password" or adding a comment to that section of the code will make it more clear (and avoid future confusion)
Besides that the branch looks good to me .
Updated by Radhika Chippada over 8 years ago
Thanks Nico. Added a comment in auth_handler.go as suggested.
Updated by Radhika Chippada over 8 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|commit:e0213cbec6a151e077b8cca00700815c3c3d18e7.