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 8 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
91 changes: 91 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,13 @@ 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.

// If providing a certificate:
// The Username field is populated from the cert and should not be set

Choose a reason for hiding this comment

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

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

// The Mechanism field should be MONGODB-X509 or not set.
// The Source field should be $external or not set.
Certificate *x509.Certificate
}

// Login authenticates with MongoDB using the provided credential. The
Expand All @@ -847,6 +857,17 @@ 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 errors.New("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.

certificate is spelt wrong - sorry!

}

if cred.Certificate != nil {
credCopy.Username, err = getRFC2253NameStringFromCert(cred.Certificate)
if err != nil {
return err
}
}

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

// getRFC2253NameStringFromCert converts from an ASN.1 structured representation of the certificate
// to a UTF-8 string representation(RDN) and returns it.
func getRFC2253NameStringFromCert(certificate *x509.Certificate) (string, error) {
var RDNElements = pkix.RDNSequence{}
_, err := asn1.Unmarshal(certificate.RawSubject, &RDNElements)
return getRFC2253NameString(&RDNElements), err
}

// getRFC2253NameString converts from an ASN.1 structured representation of the RDNSequence
// 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(",", "\\,", "=", "\\=", "+", "\\+", "<", "\\<", ">", "\\>", "#", "\\#", ";", "\\;")
Copy link

@szank szank Oct 31, 2017

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

//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]))
for j, attribute := range (*RDNElements)[i] {
var shortAttributeName = rdnOIDToShortName(attribute.Type)
if len(shortAttributeName) <= 0 {
nameAndValueList[j] = fmt.Sprintf("%s=%X", attribute.Type.String(), attribute.Value.([]byte))
continue
}
var attributeValueString = attribute.Value.(string)
// escape leading space
if strings.HasPrefix(attributeValueString, " ") {

Choose a reason for hiding this comment

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

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.

Copy link
Author

@csucu csucu Nov 1, 2017

Choose a reason for hiding this comment

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

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?

Copy link

@tadukurow tadukurow Nov 2, 2017

Choose a reason for hiding this comment

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

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.

attributeValueString = "\\" + attributeValueString
}
// escape trailing space, unless it's already escaped
if strings.HasSuffix(attributeValueString, " ") && !strings.HasSuffix(attributeValueString, "\\ ") {
attributeValueString = attributeValueString[:len(attributeValueString)-1] + "\\ "
}

// escape , = + < > # ;
attributeValueString = replacer.Replace(attributeValueString)
nameAndValueList[j] = fmt.Sprintf("%s=%s", shortAttributeName, attributeValueString)
}

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 ""
}
44 changes: 42 additions & 2 deletions session_internal_test.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,17 @@
package mgo

import (
"testing"

"crypto/x509/pkix"
"encoding/asn1"
"github.com/globalsign/mgo/bson"
. "gopkg.in/check.v1"
"testing"
)

type S struct{}

var _ = Suite(&S{})

// This file is for testing functions that are not exported outside the mgo
// package - avoid doing so if at all possible.

Expand All @@ -22,3 +28,37 @@ func TestIndexedInt64FieldsBug(t *testing.T) {

_ = simpleIndexKey(input)
}

func (s *S) TestGetRFC2253NameStringSingleValued(c *C) {
var RDNElements = pkix.RDNSequence{
{{Type: asn1.ObjectIdentifier{2, 5, 4, 6}, Value: "GO"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 8}, Value: "MGO"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 7}, Value: "MGO"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 10}, Value: "MGO"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 11}, Value: "Client"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 3}, Value: "localhost"}},
}

c.Assert(getRFC2253NameString(&RDNElements), Equals, "CN=localhost,OU=Client,O=MGO,L=MGO,ST=MGO,C=GO")
}

func (s *S) TestGetRFC2253NameStringEscapeChars(c *C) {
var RDNElements = pkix.RDNSequence{
{{Type: asn1.ObjectIdentifier{2, 5, 4, 6}, Value: "GB"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 8}, Value: "MGO "}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 10}, Value: "Sue, Grabbit and Runn < > ;"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 3}, Value: "L. Eagle"}},
}

c.Assert(getRFC2253NameString(&RDNElements), Equals, "CN=L. Eagle,O=Sue\\, Grabbit and Runn \\< \\> \\;,ST=MGO\\ ,C=GB")
}

func (s *S) TestGetRFC2253NameStringMultiValued(c *C) {
var RDNElements = pkix.RDNSequence{
{{Type: asn1.ObjectIdentifier{2, 5, 4, 6}, Value: "US"}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 10}, Value: "Widget Inc."}},
{{Type: asn1.ObjectIdentifier{2, 5, 4, 11}, Value: "Sales"}, {Type: asn1.ObjectIdentifier{2, 5, 4, 3}, Value: "J. Smith"}},
}

c.Assert(getRFC2253NameString(&RDNElements), Equals, "OU=Sales+CN=J. Smith,O=Widget Inc.,C=US")
}