Add proper DN construction #55

csucu · 2017-10-27T16:46:27Z

Added RDN construction for TLS authentication.

tadukurow · 2017-10-30T09:53:43Z

session.go

@@ -847,6 +853,14 @@ func (s *Session) Login(cred *Credential) error {
 	defer socket.Release()

 	credCopy := *cred
+	if cred.Certificate != nil && cred.Username != "" {


This probably breaks the deployment for people that specify the Username themselves.
Perhaps
if cred.Mechanism == "MONGODB-X509" && cred.Username == "" {
credCopy.Username = getRFC2253NameString(cred.Certificate)
}
is enough.

@domodwyer and @szank you know more about mgo auth then me.

cred.Certificate is new so this is fine

Still shouldn't the check rely on the mechanism specified? rather than the presence of a certificate?

I'd say. If the database is "$external" and the mechanism is "MONGODB-X509" then use the certificate. Overwrite the user name in the copy of the credentials. Otherwise use credentials as is.

Users might want to use both forms of authentication, for example. This would be the safest way.
Otherwise you could create an LoginWithCert method and make everyone's life easier.

The only reason to pass a cert is to use it for authentication - if present, it's using cert-based auth, there's no reason to then set the mechanism to something else...

This code should set the username/mechanism/database as appropriate when a cert is given.

tadukurow · 2017-10-30T09:55:34Z

session.go

+// getRFC2253NameString converts from an ASN.1 structured representation of the certificate
+// to a UTF-8 string representation(RDN) and returns it.
+func getRFC2253NameString(certificate *x509.Certificate) string {
+	var RDNElementsASN1 = pkix.RDNSequence{}


/RDNElementsASN1/rdnSequence

The elements in there are not ASN1 anymore.

tadukurow · 2017-10-30T09:57:02Z

session.go

+// to a UTF-8 string representation(RDN) and returns it.
+func getRFC2253NameString(certificate *x509.Certificate) string {
+	var RDNElementsASN1 = pkix.RDNSequence{}
+	asn1.Unmarshal(certificate.RawSubject, &RDNElementsASN1)


Better to not swallow the error and return it to the caller as this indicates a broken/bad certificate and would be good to know.

tadukurow · 2017-10-30T10:01:57Z

session.go

+	asn1.Unmarshal(certificate.RawSubject, &RDNElementsASN1)
+
+	var RDNElementsString = []string{}
+	for i := len(RDNElementsASN1) - 1; i >= 0; i-- {


Is there a reason to iterate from the back? DN ordering?
If so this will need a comment

There is. Something about AD field ordering.

the sequence is in reverse order, in the RFC it says "the output consists of the string encodings of each RelativeDistinguishedName in the RDNSequence, starting with the last element of the sequence and moving backwards toward the first."

tadukurow · 2017-10-30T10:04:31Z

session.go

+		for _, attribute := range RDNElementsASN1[i] {
+			var shortAttributeName = rdnOIDToShortName(attribute.Type)
+			if len(shortAttributeName) > 0 {
+				var attributeValueString = attribute.Value.(string)


It's not immediately clear to me, why certain DN elements need sanitizing and the ones we don't understand don't. The logic here could be much tidier if we sanitize everything.
@szank @domodwyer ?

according to the RFC if the element OID is not one of the "well known ones" you hex encode the DER representation of the element. Please see my comment below.
The safest option here is to return an error on any RDN attribute type we don't know about (any outside the "well known ones".

Users can still use such certs, they just need to use openssl to return a proper string representation of the RDN.
Also, IIRC in mgo 3.4 the server itself converts the RDN to string, so setting the user name in the client is not required.

tadukurow · 2017-10-30T10:05:57Z

session.go

+
+	var RDNElementsString = []string{}
+	for i := len(RDNElementsASN1) - 1; i >= 0; i-- {
+		var nameAndValueList = []string{}


This is not a critical path I think, but as a tip:

var nameAndValueList = make([]string,len(RDNElementsASN1[i))

tadukurow · 2017-10-30T10:07:54Z

session.go

+					attributeValueString = "\\" + attributeValueString
+				}
+				// escape trailing space (unless the trailing space is also the first (unescaped) character)
+				if len(attributeValueString) > 2 && attributeValueString[len(attributeValueString)-1] == ' ' {


@szank mgo doesn't handle trailing spaces in RDN?

IIRC this is RFC requirement. Nothing to do with mgo. Leading and trailing spaces needs to be escaped.
But if the RDN contains one character, " ", then we don't want to escape it twice.

domodwyer · 2017-10-30T10:26:31Z

session.go

@@ -847,6 +853,14 @@ func (s *Session) Login(cred *Credential) error {
 	defer socket.Release()

 	credCopy := *cred
+	if cred.Certificate != nil && cred.Username != "" {
+		return fmt.Errorf("Failed to login, both certifcate and credentials are given.")


It's more efficient to use errors.New() when you don't require a formatter, and errors typically (should) start with a lowercase letter.

Have a quick read of https://github.com/golang/go/wiki/CodeReviewComments though the existing mgo code doesn't exactly follow it

domodwyer · 2017-10-30T10:32:23Z

session.go

@@ -825,6 +828,9 @@ type Credential struct {
 	// Mechanism defines the protocol for credential negotiation.
 	// Defaults to "MONGODB-CR".
 	Mechanism string
+
+	// Certificate defines an x509 certificate for authentication at login.


The documentation here should specify the constraints - specifically:

The Username field is populated from the cert and should not be set

The Mechanism field should be MONGODB-X509 or not set (automatically populate in this case)

The Source field should be $external or not set (auto populate too)

I would also add link to
https://docs.mongodb.com/manual/tutorial/configure-x509-client-authentication/

domodwyer · 2017-10-30T10:34:41Z

session.go

+		var nameAndValueList = []string{}
+		for _, attribute := range RDNElementsASN1[i] {
+			var shortAttributeName = rdnOIDToShortName(attribute.Type)
+			if len(shortAttributeName) > 0 {


Invert conditional and continue to avoid the if...else...

domodwyer · 2017-10-30T10:35:43Z

session.go

+				}
+
+				// escape , = + < > # ;
+				var replacer = strings.NewReplacer(",", "\\,", "=", "\\=", "+", "\\+", "<", "\\<", ">", "\\>", "#", "\\#", ";", "\\;")


Initialise replacer outside of the loop and reuse

domodwyer · 2017-10-30T10:44:46Z

session.go

+
+// getRFC2253NameString converts from an ASN.1 structured representation of the certificate
+// to a UTF-8 string representation(RDN) and returns it.
+func getRFC2253NameString(certificate *x509.Certificate) string {


This should have unit tests covering at the very least, the examples in section 5 of the RFC - if you make a function that accepts the unmarshaled pkix.RDNSequence and does the actual work you should be able to unit test by the logic of this func.

If you do the above, getRFC2253NameString() becomes a helper func accepting a x509.Certificate that calls the actual logic func - maybe the helper func can be getRFC2253NameStringFromCert() that calls getRFC2253NameString() or something - up to you.

domodwyer · 2017-10-30T10:46:08Z

session.go

+			if len(shortAttributeName) > 0 {
+				var attributeValueString = attribute.Value.(string)
+				// escape leading space or #
+				if strings.IndexAny(attributeValueString, " #") == 0 {


strings.HasPrefix() would be more efficient here.

tadukurow · 2017-10-31T11:01:55Z

session.go

+			}
+
+			// escape , = + < > # ;
+			replacer = strings.NewReplacer(",", "\\,", "=", "\\=", "+", "\\+", "<", "\\<", ">", "\\>", "#", "\\#", ";", "\\;")


The list of strings to replace does not change, strings.NewReplacer can be initialised once outside of all the loops.

tadukurow · 2017-10-31T11:03:33Z

session.go

+			}
+			var attributeValueString = attribute.Value.(string)
+			// escape leading space or #
+			if strings.HasPrefix(attributeValueString, " #") == true {


The comment says space or #, the HasPrefix checks for space and #.
Should be
if strings.HasPrefix(attributeValueString, " ") || strings.HasPrefix(attributeValueString, "#") {

Also == true comparison in if statements are redundant.

tadukurow · 2017-10-31T11:11:16Z

session.go

+				attributeValueString = "\\" + attributeValueString
+			}
+			// escape trailing space (unless the trailing space is also the first (unescaped) character)
+			if len(attributeValueString) > 2 && attributeValueString[len(attributeValueString)-1] == ' ' {


This check is wrong. It should escape a trailing space, unless it is already escaped.
if strings.HasSuffix(attributeValueString, " ") && !strings.HasSuffix(attributeValueString, "\ ") {

tadukurow · 2017-10-31T15:15:00Z

session.go

+	//The elements in the sequence needs to be reversed when converting them
+	for i := len(*RDNElements) - 1; i >= 0; i-- {
+		var nameAndValueList = make([]string,len((*RDNElements)[i]))
+		var replacer = strings.NewReplacer(",", "\\,", "=", "\\=", "+", "\\+", "<", "\\<", ">", "\\>", "#", "\\#", ";", "\\;")


the replacer can be outside the outer loop

tadukurow · 2017-10-31T15:16:19Z

session.go

+			}
+			var attributeValueString = attribute.Value.(string)
+			// escape leading space or #
+			if strings.HasPrefix(attributeValueString, " ") || strings.HasPrefix(attributeValueString, "#") {


strings.HasPrefix(attributeValueString, "#") can be removed as the replacer already takes care of that one.

tadukurow · 2017-10-31T15:20:35Z

session.go

+			if strings.HasPrefix(attributeValueString, " ") || strings.HasPrefix(attributeValueString, "#") {
+				attributeValueString = "\\" + attributeValueString
+			}
+			// escape trailing space (unless the trailing space is also the first (unescaped) character)


change comment to read
// escape trailing space, unless it's already escaped

tadukurow · 2017-10-31T16:31:34Z

session.go

+				continue
+			}
+			var attributeValueString = attribute.Value.(string)
+			// escape leading space or #


comment needs updating

szank · 2017-10-27T17:03:31Z

session.go

@@ -847,6 +853,14 @@ func (s *Session) Login(cred *Credential) error {
 	defer socket.Release()

 	credCopy := *cred
+	if cred.Certificate != nil && cred.Username != "" {


I am 99.99% sure that mongodb 3.4 doesn't need it. It will extract the user name from the client cert.

szank · 2017-10-30T11:14:15Z

session.go

@@ -825,6 +828,9 @@ type Credential struct {
 	// Mechanism defines the protocol for credential negotiation.
 	// Defaults to "MONGODB-CR".
 	Mechanism string
+
+	// Certificate defines an x509 certificate for authentication at login.


I would also add link to
https://docs.mongodb.com/manual/tutorial/configure-x509-client-authentication/

szank · 2017-10-30T11:22:22Z

session.go

@@ -847,6 +853,14 @@ func (s *Session) Login(cred *Credential) error {
 	defer socket.Release()

 	credCopy := *cred
+	if cred.Certificate != nil && cred.Username != "" {


I'd say. If the database is "$external" and the mechanism is "MONGODB-X509" then use the certificate. Overwrite the user name in the copy of the credentials. Otherwise use credentials as is.

Users might want to use both forms of authentication, for example. This would be the safest way.
Otherwise you could create an LoginWithCert method and make everyone's life easier.

szank · 2017-10-30T11:25:13Z

session.go

+	asn1.Unmarshal(certificate.RawSubject, &RDNElementsASN1)
+
+	var RDNElementsString = []string{}
+	for i := len(RDNElementsASN1) - 1; i >= 0; i-- {


There is. Something about AD field ordering.

szank · 2017-10-30T11:33:13Z

session.go

+					attributeValueString = "\\" + attributeValueString
+				}
+				// escape trailing space (unless the trailing space is also the first (unescaped) character)
+				if len(attributeValueString) > 2 && attributeValueString[len(attributeValueString)-1] == ' ' {


IIRC this is RFC requirement. Nothing to do with mgo. Leading and trailing spaces needs to be escaped.
But if the RDN contains one character, " ", then we don't want to escape it twice.

szank · 2017-10-30T11:46:11Z

session.go

+				attributeValueString = replacer.Replace(attributeValueString)
+				nameAndValueList = append(nameAndValueList, fmt.Sprintf("%s=%s", shortAttributeName, attributeValueString))
+			} else {
+				nameAndValueList = append(nameAndValueList, fmt.Sprintf("%s=%X", attribute.Type.String(), attribute.Value.([]byte)))


From the RFC:
If the AttributeValue is of a type which does not have a string
representation defined for it, then it is simply encoded as an
octothorpe character ('#' ASCII 35) followed by the hexadecimal
representation of each of the bytes of the BER encoding of the X.500
AttributeValue. This form SHOULD be used if the AttributeType is of
the dotted-decimal form.

Dotted decimal is OID, in human terms. We are missing the # before hex encoding tho.

FWIW, this code was not designed to handle cases like newline char 0x0A (\n) either. I have written a internal method that was good at converting our own cert's RDNs that weren't supposed to contain any stupid input, and had limited list of RDN field types.

This is the reason for few decisions here, like the one from line 5257. I am not sure I am handling unknown OIDS correctly ( even if we add the '#' sign there.

szank · 2017-10-30T12:00:18Z

session.go

+		for _, attribute := range RDNElementsASN1[i] {
+			var shortAttributeName = rdnOIDToShortName(attribute.Type)
+			if len(shortAttributeName) > 0 {
+				var attributeValueString = attribute.Value.(string)


according to the RFC if the element OID is not one of the "well known ones" you hex encode the DER representation of the element. Please see my comment below.
The safest option here is to return an error on any RDN attribute type we don't know about (any outside the "well known ones".

Users can still use such certs, they just need to use openssl to return a proper string representation of the RDN.
Also, IIRC in mgo 3.4 the server itself converts the RDN to string, so setting the user name in the client is not required.

szank · 2017-10-31T17:23:58Z

session.go

+// from the certificate to a UTF-8 string representation(RDN) and returns it.
+func getRFC2253NameString(RDNElements *pkix.RDNSequence) string {
+	var RDNElementsString = []string{}
+	var replacer = strings.NewReplacer(",", "\\,", "=", "\\=", "+", "\\+", "<", "\\<", ">", "\\>", "#", "\\#", ";", "\\;")


According to the RFC # has to be escaped only if it is the first character in the string.

Yea, i'm not sure about this one, because in section 3 it has it listed as one of the special chars that needs to be escaped i think, unless i'm reading the grammer wrong. "special = "," / "=" / "+" / "<" / ">" / "#" / ";" " I also had looked at the way its done in java's openjdk and they escape the # throughout the string.

It's slightly confusing, but Section 3 is for parsing a string back to DN. the section above states only leading '#' should be escaped, so I'd say '#' has to be dropped from the replacer and the HasPrefix('#') be added back in.

domodwyer

Code is much cleaner, but I think we should set the source, mechanism & DB when using a cert for auth.

domodwyer · 2017-11-01T09:58:51Z

session.go

@@ -847,6 +853,14 @@ func (s *Session) Login(cred *Credential) error {
 	defer socket.Release()

 	credCopy := *cred
+	if cred.Certificate != nil && cred.Username != "" {


The only reason to pass a cert is to use it for authentication - if present, it's using cert-based auth, there's no reason to then set the mechanism to something else...

This code should set the username/mechanism/database as appropriate when a cert is given.

domodwyer · 2017-11-01T09:59:52Z

session.go

+
+	// Certificate defines an x509 certificate for authentication at login.
+	// If providing a certificate:
+	// The Username field is populated from the cert and should not be set


You would have to implement the "not specified" case and set the field for the user - it's just a helper thing.

domodwyer · 2017-11-01T10:01:12Z

session.go

@@ -847,6 +857,17 @@ func (s *Session) Login(cred *Credential) error {
 	defer socket.Release()

 	credCopy := *cred
+	if cred.Certificate != nil && cred.Username != "" {
+		return errors.New("failed to login, both certifcate and credentials are given")


certificate is spelt wrong - sorry!

domodwyer · 2017-11-01T10:06:53Z

session.go

+			}
+			var attributeValueString = attribute.Value.(string)
+			// escape leading space
+			if strings.HasPrefix(attributeValueString, " ") {


This is all much clearer now - good job 👍

What happens if it's prefixed by two spaces though? I agree it's an edge case, I'd be inclined to to fix my cert rather than the code.

Currently it just escapes the last space, and the rest are left. The rfc isn't very clear on what todo with more then one trailing space. Should i be escaping all of them?

The escaping of leading and trailing spaces, is to prevent trimming. By escaping the leading and trailing space, the trimming is no longer an issue, regardless of how many spaces it has after/before that.

tadukurow

Looking good. Just a small update to check for leading '#' and not escape everywhere and it should be good.

old review

@bachue

Includes: * Reduced memory in bulk operations (#56) * Native x509 authentication (#55) * Better connection recovery (#69) * Example usage (#75 and #78) Thanks to: * @bachue * @csucu * @feliixx --- [Throughput overview](https://user-images.githubusercontent.com/9275968/34954403-3d3253dc-fa18-11e7-8eef-0f2b0f21edc3.png) Select throughput has increased by ~600 requests/second with slightly increased variance: ``` x => r2017.11.06-select-zipfian-throughput.log y => 9acbd68-select-zipfian-throughput.log n min max median average stddev p99 x 3600 49246 71368 66542 66517.26 2327.675 70927.01 y 3600 53304 72005 67151 67145.36 2448.534 71630.00 62000 64000 66000 68000 70000 72000 |----------+-----------+-----------+------------+-----------+-----------+-----| +---------+--------+ 1 -------------------| | |-------------------- +---------+--------+ +---------+---------+ 2 ----------------------------| | |-------------------- +---------+---------+ Legend: 1=data$x, 2=data$y At 95% probablitiy: ===> average is statistically significant (p=0.000000, diff ~628.094444) ===> variance is statistically significant (p=0.002398) ``` * [insert-latency.txt](https://github.com/globalsign/mgo/files/1632474/insert-latency.txt) * [insert-throughput.txt](https://github.com/globalsign/mgo/files/1632475/insert-throughput.txt) * [select-zipfian-latency.txt](https://github.com/globalsign/mgo/files/1632476/select-zipfian-latency.txt) * [select-zipfian-throughput.txt](https://github.com/globalsign/mgo/files/1632477/select-zipfian-throughput.txt) * [update-zipfian-latency.txt](https://github.com/globalsign/mgo/files/1632478/update-zipfian-latency.txt) * [update-zipfian-throughput.txt](https://github.com/globalsign/mgo/files/1632479/update-zipfian-throughput.txt) Note: latencies are approximations calculated from grouped data

@bachue

Includes: * Reduced memory in bulk operations (globalsign#56) * Native x509 authentication (globalsign#55) * Better connection recovery (globalsign#69) * Example usage (globalsign#75 and globalsign#78) Thanks to: * @bachue * @csucu * @feliixx --- [Throughput overview](https://user-images.githubusercontent.com/9275968/34954403-3d3253dc-fa18-11e7-8eef-0f2b0f21edc3.png) Select throughput has increased by ~600 requests/second with slightly increased variance: ``` x => r2017.11.06-select-zipfian-throughput.log y => 9acbd68-select-zipfian-throughput.log n min max median average stddev p99 x 3600 49246 71368 66542 66517.26 2327.675 70927.01 y 3600 53304 72005 67151 67145.36 2448.534 71630.00 62000 64000 66000 68000 70000 72000 |----------+-----------+-----------+------------+-----------+-----------+-----| +---------+--------+ 1 -------------------| | |-------------------- +---------+--------+ +---------+---------+ 2 ----------------------------| | |-------------------- +---------+---------+ Legend: 1=data$x, 2=data$y At 95% probablitiy: ===> average is statistically significant (p=0.000000, diff ~628.094444) ===> variance is statistically significant (p=0.002398) ``` * [insert-latency.txt](https://github.com/globalsign/mgo/files/1632474/insert-latency.txt) * [insert-throughput.txt](https://github.com/globalsign/mgo/files/1632475/insert-throughput.txt) * [select-zipfian-latency.txt](https://github.com/globalsign/mgo/files/1632476/select-zipfian-latency.txt) * [select-zipfian-throughput.txt](https://github.com/globalsign/mgo/files/1632477/select-zipfian-throughput.txt) * [update-zipfian-latency.txt](https://github.com/globalsign/mgo/files/1632478/update-zipfian-latency.txt) * [update-zipfian-throughput.txt](https://github.com/globalsign/mgo/files/1632479/update-zipfian-throughput.txt) Note: latencies are approximations calculated from grouped data

Add proper DN construction

4420aa2

tadukurow requested review from szank, brknstrngz, tadukurow and domodwyer October 27, 2017 16:51

Added openssl check to test

53fe0ae

tadukurow suggested changes Oct 30, 2017

View reviewed changes

domodwyer assigned csucu Oct 30, 2017

domodwyer added the enhancement label Oct 30, 2017

domodwyer suggested changes Oct 30, 2017

View reviewed changes

Addressed code review comments

92809d8

tadukurow suggested changes Oct 31, 2017

View reviewed changes

Changes to RDNformatting, and test

9782b3d

tadukurow previously approved these changes Oct 31, 2017

View reviewed changes

type/value fields to tests

37421cd

csucu dismissed tadukurow’s stale review via 37421cd October 31, 2017 13:49

tadukurow suggested changes Oct 31, 2017

View reviewed changes

csucu added 2 commits October 31, 2017 15:29

Changes to RDN formatting

f1bcfbe

Corrected comment in getRFC2253NameString

214fef7

tadukurow suggested changes Oct 31, 2017

View reviewed changes

session.go

continue

}

var attributeValueString = attribute.Value.(string)

// escape leading space or #

Copy link

tadukurow Oct 31, 2017

Choose a reason for hiding this comment

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

comment needs updating

Corrected comment in getRFC2253NameString

97eb860

tadukurow previously approved these changes Oct 31, 2017

View reviewed changes

szank previously requested changes Oct 31, 2017

View reviewed changes

domodwyer suggested changes Nov 1, 2017

View reviewed changes

Changes to login and rdn formatting

04e0f06

csucu dismissed tadukurow’s stale review via 04e0f06 November 1, 2017 17:12

tadukurow suggested changes Nov 2, 2017

View reviewed changes

Changed escaping of #

14e1c13

tadukurow approved these changes Nov 2, 2017

View reviewed changes

domodwyer approved these changes Nov 2, 2017

View reviewed changes

domodwyer merged commit 663dfe5 into development Nov 2, 2017

domodwyer deleted the feature/rdnconstruction branch November 2, 2017 14:52

domodwyer added the needs stg test label Dec 27, 2017

domodwyer removed the needs stg test label Jan 9, 2018

domodwyer mentioned this pull request Jan 15, 2018

Merge Development #79

Merged

Add proper DN construction #55

Add proper DN construction #55

Conversation

csucu commented Oct 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szank Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domodwyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csucu Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

tadukurow Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

tadukurow left a comment

Choose a reason for hiding this comment

szank Oct 31, 2017 •

edited

Loading

csucu Nov 1, 2017 •

edited

Loading

tadukurow Nov 2, 2017 •

edited

Loading