Skip to content
This repository was archived by the owner on Dec 12, 2025. It is now read-only.

Conversation

@chatton
Copy link
Contributor

@chatton chatton commented Jul 13, 2020

All Submissions:

  • Have you signed our CLA?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

This PR moves the Credential generating logic from the enterprise code base to community. There are no functional changes. I've added additional unit tests for Sha256 secret generation.

@chatton chatton requested a review from rodrigovalin July 13, 2020 16:15
Comment on lines +17 to +22
func TestScramSha256SecretsMatch(t *testing.T) {
assertSecretsMatch(t, sha256.New, "Gy4ZNMr-SYEsEpAEZv", 15000, "ajdf1E1QTsNAQdBEodB4vzQOFuvcw9K6PmouVg==", "/pBk9XBwSm9UyeQmyJ3LfogfHu9Z/XTjGmRhQDHx/4I=", "Avm8mjtMyg659LAyeD4VmuzQb5lxL5iy3dCuzfscfMc=")
assertSecretsMatch(t, sha256.New, "Y9SPYSJYUJB_", 15000, "Oplsu3uju+lYyX4apKb0K6xfHpmFtH99Oyk4Ow==", "oTJhml8KKZUSt9k4tg+tS6D/ygR+a2Xfo8JKjTpQoAI=", "SUfA2+SKL35u665WY5NnJJmA9L5dHu/TnWXX/0nm42Y=")
assertSecretsMatch(t, sha256.New, "157VDZr0h-Pz-wj72", 15000, "P/4xs3anygxu3/l2p35CSBe4Z47IV/FtE/e44A==", "jOb27nFF72SQoY7WUqKXOTR4e8jETXxMS67SONrcbjA=", "3FnslkgUweautAfPRCOEjhS+YbUYUNmdDQUGxB+oaFE=")
assertSecretsMatch(t, sha256.New, "P8z1sDfELCePTNbVqX", 15000, "RPNhenwTHlqW5OE597XpuwvPLaiecPpYFa58Pg==", "sJ8UhQRszLNo15cOe62+HLjt2NxmSkJGjdJpclTIMBs=", "CSg02ODAvh9+swUHoimXcDsT9lLp/A5IhQXavXl7+qA=")
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test cases are using the salt, stored key and sever keys of users created through the mongo shell. E.g.

 db.createUser({user: "user4", 
pwd: "P8z1sDfELCePTNbVqX", 
roles: [ { role: "readWrite", db: "admin" } ]})

db.system.users.findOne({user: "user4"})

outputs

example-mongodb:PRIMARY> db.system.users.findOne({user: "user4"})
{
	"_id" : "admin.user4",
	"userId" : UUID("6b5aec1f-312b-491a-aeaa-b00cc374cf52"),
	"user" : "user4",
	"db" : "admin",
	"credentials" : {
		"SCRAM-SHA-1" : {
			"iterationCount" : 10000,
			"salt" : "Cd9RJHbdRt5b+kPf8YR0WQ==",
			"storedKey" : "v3BB0CLs5AwLiAgIfE/yqDujoI4=",
			"serverKey" : "yLqlYgg5GW04nAeBmFpgLU/vtto="
		},
		"SCRAM-SHA-256" : {
			"iterationCount" : 15000,
			"salt" : "RPNhenwTHlqW5OE597XpuwvPLaiecPpYFa58Pg==",
			"storedKey" : "sJ8UhQRszLNo15cOe62+HLjt2NxmSkJGjdJpclTIMBs=",
			"serverKey" : "CSg02ODAvh9+swUHoimXcDsT9lLp/A5IhQXavXl7+qA="
		}
	},
	"roles" : [
		{
			"role" : "readWrite",
			"db" : "admin"
		}
	]
}

@chatton chatton changed the title added unit tests for SHA256 Add functions to generate Scram Credentials Jul 13, 2020
Copy link
Contributor

@rodrigovalin rodrigovalin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a small comment to get rid of one function indirection, otherwise LGTM

}

func ComputeScramSha256Creds(username, password string, salt []byte) (ScramCreds, error) {
return computeCreds(username, password, salt, sha256mechanismName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function only dispatches the request to computeCreds and then, there's branching in computeCreds depending on that..

Why not:

func ComputeScramSha256Creds(username, password string, salt []byte) (ScramCreds, error) {
	base64EncodedSalt := base64.StdEncoding.EncodeToString(salt)
	return computeScramCredentials(sha256.New, scramSha256Iterations, base64EncodedSalt, password)
}

func ComputeScramSha1Creds(username, password string, salt []byte) ScramCreds, error) { 
	base64EncodedSalt := base64.StdEncoding.EncodeToString(salt)
	password = md5Hex(username + ":mongo:" + password)
	return computeScramCredentials(sha1.New, scramSha1Iterations, base64EncodedSalt, password)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is actually much nicer, as the Sha256 function doesn't actually require username 👍

@chatton chatton merged commit 658585c into master Jul 14, 2020
@chatton chatton deleted the introduce_scram_credential_code branch July 14, 2020 07:42
rodrigovalin pushed a commit that referenced this pull request Feb 15, 2022
rodrigovalin pushed a commit that referenced this pull request Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants