Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proper DN construction #55

Merged
merged 10 commits into from
Nov 2, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ package mgo_test

import (
"crypto/tls"
"crypto/x509"
"flag"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -963,6 +964,57 @@ func (s *S) TestAuthX509Cred(c *C) {
c.Assert(len(names) > 0, Equals, true)
}

func (s *S) TestAuthX509CredRDNConstruction(c *C) {
session, err := mgo.Dial("localhost:40001")
c.Assert(err, IsNil)
defer session.Close()
binfo, err := session.BuildInfo()
c.Assert(err, IsNil)
if binfo.OpenSSLVersion == "" {
c.Skip("server does not support SSL")
}

clientCertPEM, err := ioutil.ReadFile("harness/certs/client.pem")
c.Assert(err, IsNil)

clientCert, err := tls.X509KeyPair(clientCertPEM, clientCertPEM)
c.Assert(err, IsNil)

clientCert.Leaf, err = x509.ParseCertificate(clientCert.Certificate[0])
c.Assert(err, IsNil)

tlsConfig := &tls.Config{
InsecureSkipVerify: true,
Certificates: []tls.Certificate{clientCert},
}

var host = "localhost:40003"
c.Logf("Connecting to %s...", host)
session, err = mgo.DialWithInfo(&mgo.DialInfo{
Addrs: []string{host},
DialServer: func(addr *mgo.ServerAddr) (net.Conn, error) {
return tls.Dial("tcp", addr.String(), tlsConfig)
},
})
c.Assert(err, IsNil)
defer session.Close()

cred := &mgo.Credential{
Username: "root",
Mechanism: "MONGODB-X509",
Source: "$external",
Certificate: tlsConfig.Certificates[0].Leaf,
}
err = session.Login(cred)
c.Assert(err, NotNil)

cred.Username = ""
c.Logf("Authenticating...")
err = session.Login(cred)
c.Assert(err, IsNil)
c.Logf("Authenticated!")
}

var (
plainFlag = flag.String("plain", "", "Host to test PLAIN authentication against (depends on custom environment)")
plainUser = "einstein"
Expand Down
78 changes: 78 additions & 0 deletions session.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ package mgo

import (
"crypto/md5"
"crypto/x509"
"crypto/x509/pkix"
"encoding/asn1"
"encoding/hex"
"errors"
"fmt"
Expand Down Expand Up @@ -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.

Choose a reason for hiding this comment

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

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)

Copy link

Choose a reason for hiding this comment

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

Certificate *x509.Certificate
}

// Login authenticates with MongoDB using the provided credential. The
Expand All @@ -847,6 +853,14 @@ func (s *Session) Login(cred *Credential) error {
defer socket.Release()

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

cred.Certificate is new so this is fine

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

return fmt.Errorf("Failed to login, both certifcate and credentials are given.")

Choose a reason for hiding this comment

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

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

}

if cred.Certificate != nil {
credCopy.Username = getRFC2253NameString(cred.Certificate)
}

if cred.Source == "" {
if cred.Mechanism == "GSSAPI" {
credCopy.Source = "$external"
Expand Down Expand Up @@ -5212,3 +5226,67 @@ func hasErrMsg(d []byte) bool {
}
return false
}

// 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 {

Choose a reason for hiding this comment

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

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.

var RDNElementsASN1 = pkix.RDNSequence{}

Choose a reason for hiding this comment

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

/RDNElementsASN1/rdnSequence

The elements in there are not ASN1 anymore.

asn1.Unmarshal(certificate.RawSubject, &RDNElementsASN1)

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

There is. Something about AD field ordering.

Copy link
Author

Choose a reason for hiding this comment

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

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

var nameAndValueList = []string{}

Choose a reason for hiding this comment

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

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

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

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

Choose a reason for hiding this comment

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

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

var attributeValueString = attribute.Value.(string)

Choose a reason for hiding this comment

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

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 ?

Copy link

Choose a reason for hiding this comment

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

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.

// escape leading space or #
if strings.IndexAny(attributeValueString, " #") == 0 {

Choose a reason for hiding this comment

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

strings.HasPrefix() would be more efficient here.

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

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

attributeValueString = attributeValueString[:len(attributeValueString)-1] + "\\ "
}

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

Choose a reason for hiding this comment

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

Initialise replacer outside of the loop and reuse

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)))
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

}
}

RDNElementsString = append(RDNElementsString, strings.Join(nameAndValueList, "+"))
}

return strings.Join(RDNElementsString, ",")
}

var oidsToShortNames = []struct {
oid asn1.ObjectIdentifier
shortName string
}{
{asn1.ObjectIdentifier{2, 5, 4, 3}, "CN"},
{asn1.ObjectIdentifier{2, 5, 4, 6}, "C"},
{asn1.ObjectIdentifier{2, 5, 4, 7}, "L"},
{asn1.ObjectIdentifier{2, 5, 4, 8}, "ST"},
{asn1.ObjectIdentifier{2, 5, 4, 10}, "O"},
{asn1.ObjectIdentifier{2, 5, 4, 11}, "OU"},
{asn1.ObjectIdentifier{2, 5, 4, 9}, "STREET"},
{asn1.ObjectIdentifier{0, 9, 2342, 19200300, 100, 1, 25}, "DC"},
{asn1.ObjectIdentifier{0, 9, 2342, 19200300, 100, 1, 1}, "UID"},
}

// rdnOIDToShortName returns an short name of the given RDN OID. If the OID does not have a short
// name, the function returns an empty string
func rdnOIDToShortName(oid asn1.ObjectIdentifier) string {
for i := range oidsToShortNames {
if oidsToShortNames[i].oid.Equal(oid) {
return oidsToShortNames[i].shortName
}
}

return ""
}