Bug #6827

arvados-git-httpd prints plaintext user passwords to its output

Added by Joshua Randall about 4 years ago. Updated about 4 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Git hosting
Target version:
Start date:
08/05/2015
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

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" "" "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!


Subtasks

Task #6895: Review branch: 6827-no-passwords-in-logsResolvedRadhika Chippada

Associated revisions

Revision e0213cbe
Added by Radhika Chippada about 4 years ago

closes #6827
Merge branch '6827-no-passwords-in-logs'

Revision 86284313
Added by Radhika Chippada about 4 years ago

refs #6827
Merge branch '6827-short-token'

History

#1 Updated by Brett Smith about 4 years ago

  • Target version set to 2015-08-19 sprint
  • Story points set to 0.5

#2 Updated by Joshua Randall about 4 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...

#3 Updated by Tom Clegg about 4 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.)

#4 Updated by Tom Clegg about 4 years ago

  • Assigned To set to Tom Clegg

#5 Updated by Tom Clegg about 4 years ago

  • Assigned To changed from Tom Clegg to Radhika Chippada

#6 Updated by Tom Clegg about 4 years ago

  • Category set to Git hosting

#7 Updated by Radhika Chippada about 4 years ago

  • Status changed from New to In Progress

#8 Updated by Radhika Chippada about 4 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.

#9 Updated by Nico C├ęsar about 4 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 .

#10 Updated by Radhika Chippada about 4 years ago

Thanks Nico. Added a comment in auth_handler.go as suggested.

#11 Updated by Radhika Chippada about 4 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:e0213cbec6a151e077b8cca00700815c3c3d18e7.

Also available in: Atom PDF