Use Custom Cert Extensions as Cert Auth Constraint #3634

traviscosgrave · 2017-11-30T20:38:30Z

This change allows the use of custom certificate extensions as a configurable constraint for certificate authentication. Opening the ability to use CAs like Puppet or Vault itself to inject more
detailed information into the certificate as a means of more narrowly defining access to specific profiles.

This change allows the use of custom certificate extensions as a configurable constraint for certificate authentication. Opening the ability to use CAs like Puppet or Vault it self to inject more detailed information into the certificate as a means of more narrowly defining access to specific profiles.

jefferai · 2017-12-04T17:20:23Z

builtin/credential/cert/path_login.go

 }

+// isControlRune returns true for control charaters for trimming extension values
+func (b *backend) isControlRune(r rune) bool { return r <= 32 || r == 127 }


Please don't put the function definition inline -- it's not our style.

jefferai · 2017-12-04T17:22:15Z

builtin/credential/cert/path_login.go

+	// Build Client Extensions Map
+	clientExtMap := map[string]string{}
+	for _, ext := range clientCert.Extensions {
+		clientExtMap[ext.Id.String()] = strings.TrimLeftFunc(string(ext.Value[:]), b.isControlRune)


Can you leave a comment on the purpose of this line? Why just trimming the left side?

When testing the conditions with certificates with various custom extensions, I noticed that there were control characters prefixing the value of the extension. For example using fmt.Printf("%q",string(ext.Value[:]) yields:

2.1.1.1: "\f\x16A UTF8String Extension" 2.1.1.2: "\f\x10A UTF8 Extension" 2.1.1.3: "\x16\x10An IA5 Extension" 2.1.1.4: "\x1a\x13A Visible Extension"

This is the result of generating the certificate using the .cnf below in the following manner:
openssl req -new -out rootcawext.csr -config rootcawext.cnf -keyout rootcawextkey.pem -utf8
openssl x509 -req -in rootcawext.csr -CA rootcacert.pem -CAkey rootcakey.pem -out rootcawextcert.pem -sha256 -extensions req_v3 -extfile rootcawext.cnf

I tried some of the other custom extension formats, and they also resulted in prefix values not immediately obvious from the cnf file nor could I find any specific documentation regarding the manner in which these were added (probably looking in the wrong place). I did not see any examples with suffixes being added to the value.

Trimming the left hand side removed the control characters when present, but did not solve the problem entirely as some types used more than just control characters in the prefix. I had a hard time testing with certificates generated by Puppet due to the way the tests utilized the certificates and the inability to force the Puppet CA to generate the expected values, but I can check again to see if a cert with custom extensions created by Puppet or Vault also produce the (encoding?) prefixes.

In the end, I was trying to avoid writing custom extension constraints with were always bounded by wildcards ie *myvalue*.

In general, the values within the .Extensions of the certificate are not as clean as I expected. eg another cert used in the tests.

2.5.29.37: "0\x14\x06\b+\x06\x01\x05\x05\a\x03\x01\x06\b+\x06\x01\x05\x05\a\x03\x02" 2.5.29.14: "\x04\x14\x9b\xef\x9e\x1e\x9c\x8cޞ\xf4\xf1\xb8\x19&\xe4X\x11\xd5\xf5\xa3\xe5" 2.5.29.35: "0\x16\x80\x14\x9dĳO\xfe\x871\xecr\xba%=\xff\xb1 \x1e﨓\x9b" 1.3.6.1.5.5.7.1.1: "0-0+\x06\b+\x06\x01\x05\x05\a0\x02\x86\x1fhttp://127.0.0.1:8200/v1/pki/ca" 2.5.29.17: "0\x18\x82\x10cert.example.com\x87\x04\u007f\x00\x00\x01" 2.5.29.31: "0(0&\xa0$\xa0\"\x86 http://127.0.0.1:8200/v1/pki/crl"

That expectation could just be a result of being new to working with certificates programmatically.

When viewing with openssl such a generated cert also looks strange:

2.1.1.1: ..A UTF8String Extension 2.1.1.2: ..A UTF8 Extension 2.1.1.3: ..An IA5 Extension 2.1.1.4: ..A Visible Extension

How did you come upon that syntax for specifying custom OIDs? The man page (https://www.openssl.org/docs/man1.1.0/apps/config.html) suggests that it's simple key-value.

I would argue however that even if what you're doing is correct, because the control characters are a part of that custom OID, eliding them is actually incorrect behavior. Instead, you should do a direct comparison on the byte slices. This does prevent a problem for ingressing such data for comparison -- we could require all such input to be base64 encoded, which isn't nice for command-line users; we could try to heuristically detect whether it's base64-encoded, which is fragile; or we could assume string unless it's defined to be base64 in some way, such as with a b64: prefix, and hope that that never shows up in the real world. Or, we can for the moment just assume that both the values in the cert and supplied will be UTF-8 strings and punt anything further down the road if people actually need it.

The syntax in question comes from: https://www.openssl.org/docs/manmaster/man5/x509v3_config.html

After a bunch of reading on ASN.1, and the x509 code, it appears that Custom Extensions are prefixed by an implicit bitstring tag 03 LENGTH. What follows is the tag for the type written in the config eg 12 LENGTH. As such, it would make sense that we drop the first two bytes of the value and assume string. (see latest commit)

jefferai · 2017-12-04T17:23:05Z

builtin/credential/cert/path_login.go

+	// If we match all of the expected extensions, the requirement is satisfied
+	matchedExts := 0
+	for _, requiredExt := range config.Entry.RequiredExtensions {
+		reqExt := strings.Split(requiredExt, ":")


Since the value can be a UTF8 string it's probably safer to use SplitN here to ensure you only split the actual OID.

jefferai · 2017-12-04T17:25:06Z

builtin/credential/cert/path_login.go

+	for _, requiredExt := range config.Entry.RequiredExtensions {
+		reqExt := strings.Split(requiredExt, ":")
+		clientExtValue, clientExtValueExists := clientExtMap[reqExt[0]]
+		if glob.Glob(reqExt[1], clientExtValue) && clientExtValueExists {


I realize doing it this way helps keep timing constant but it's a bit more complicated that it needs to be, as I don't think timing is really a concern here (after all the certificate is public and the Vault role definition is authn/authz'd). You could simply shortcut and return false if !clientExtValueExists or the glob fails.

Use SplitN over Split for custom extension configuration to avoid missing values containing :. Simply return false on the first required custom extension faliure. Format oneline function correctly.

vishalnayak · 2017-12-07T04:12:19Z

builtin/credential/cert/backend_test.go

+}
+
+// Test a self-signed client with custom extensions (root CA) that is trusted
+func TestBackend_extensions_signleCert(t *testing.T) {


Typo: signle/single

jefferai

The changes look good but I still don't like the eliding. I'd be interested in knowing where you found that syntax for the control file.

…st two bytes and assume the value is a string.

traviscosgrave · 2017-12-10T15:03:19Z

The syntax in question comes from: https://www.openssl.org/docs/manmaster/man5/x509v3_config.html

The other syntax appears either out of date or I don't understand how it's supposed to work. I'm leaning towards the syntax has a different purpose in informing Openssl of custom extensions vs adding them to a CSR. It started to make sense when I read about puppet's use of custom extensions more closely here: https://puppet.com/docs/puppet/5.3/ssl_attributes_extensions.html#manually-checking-for-extensions-in-csrs-and-certificates

Note that every extension is preceded by any combination of two characters (.$ and .. in the above example) that contain ASN.1 encoding information. Since OpenSSL is unaware of Puppet’s custom extensions OIDs, it’s unable to properly display the values.

When I don't include the ANS1: encoding information in the csr config I receive an error about the first number in the OID being too large. The rules for OIDs state that the first number must be 0, 1 or 2 so using the Custom OID Name in the configuration doesn't match that.

Based on docs for ASN.1 and some of the Openssl source, and the golang x509 code, it appears that Custom Extensions are prefixed by a tag 03 LENGTH. What follows is the tag for the type written in the config eg 12 LENGTH. This is why the UTF8 and IA5 values don't get written to string interfaces. As such, it would make sense that we drop the first two bytes of the value, the tag, and assume string until such other features are required.

jefferai · 2017-12-11T17:41:23Z

The Puppet doc shows the .$ and .. values for CSRs, but not for final certs. Note in the example that when the CSR is printed it shows these extra control characters, but in the cert below, in the field that maps to (for instance) the UUID, those control characters display properly. This is different from the certs you're generating where the certs themselves are still showing control characters.

Possibly this is because Puppet's CA strips these values off, but it does seem like they're unexpected in an issued certificate context.

traviscosgrave · 2017-12-12T15:33:14Z

I've tried to dig down into the (puppet code)[https://github.com/puppetlabs/puppet] where it handles this. The example call they give for the CSR comes from openssl but the certificate is via puppet cert. The relevant line seems to be https://github.com/puppetlabs/puppet/blob/master/lib/puppet/ssl/certificate.rb#L84 . Where the last.value is returned. I don't have a puppet development environment set up at the moment, so I am unable to trace the process and verify if the leading 03 tag is dropped in this, or if it is dropped by the to_der call found in the Openssl package for ruby. (The ruby source is available here rb_str_drop_bytes).

When asking openssl to parse a certificate generated by puppet I get the following:

X509v3 extensions:
            Netscape Comment:
                Puppet Server Internal Certificate
            X509v3 Authority Key Identifier:
                keyid:1E:13:A2:74:61:2C:6E:D7:24:AD:6E:33:FB:78:85:EF:9F:3C:E4:D2

            X509v3 Subject Key Identifier:
                0F:E7:63:10:C2:C1:45:C0:B1:B3:E4:C5:50:54:52:70:F1:77:09:CF
            X509v3 Subject Alternative Name:
                DNS:puppet, DNS:REDACTED
            1.3.6.1.4.1.34380.1.1.1:
                .$ED803750-E3C7-44F5-BB08-41A04433FE2E
            1.3.6.1.4.1.34380.1.1.3:
                ..my_ami_image
            1.3.6.1.4.1.34380.1.1.4:
                .$342thbjkt82094y0uthhor289jnqthpc2290
            X509v3 Basic Constraints: critical
                CA:FALSE
            X509v3 Extended Key Usage: critical
                TLS Web Server Authentication, TLS Web Client Authentication
            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment

So puppet isn't removing them on certificate generation, but rather during the parsing/output step.

jefferai · 2017-12-14T20:55:41Z

Huh, so when puppet signs a CSR it drops the bytes, and when it generates its own cert it keeps them? That seems...not consistent :-)

Here's my concern though: those bytes are encoding information about the string. By not verifying that those bytes properly match, technically, the value could be something different. I also have a concern around users: if they see that value from your parsed certificate, will they put in my_ami_image as the value to match or ..my_ami_image, neither of which are technically correct?

Which brings us to a third possibility of a way forward, which is to drop the tag values from the string that comes from the parsing code, and then also strip out .. or .$ if they appear at the beginning of a user-submitted string. This probably most closely matches what people want, but means we're now modifying two values before comparing them. The user-focused person in me says we should do this, the security-focused person feels icky.

traviscosgrave · 2017-12-14T23:22:18Z

Puppet isn't dropping the bytes when it signs the CSR. The puppet cert wrapper is making them regular strings as noted in this test: https://github.com/puppetlabs/puppet/blob/496cf2f9b60d2c6224e2e8358f4284f5afa0a00f/spec/unit/ssl/certificate_factory_spec.rb#L158

So, I've continued to go further and I have found that the function which openssl uses to print ASN1 Strings can be found here: https://github.com/openssl/openssl/blob/26a7d938c9bf932a55cb5e4e02abb48fe395c5cd/crypto/asn1/a_print.c#L68

The reason we see .. for most fields (I can generate more certs if needed) is because of this code:

if ((p[i] > '~') || ((p[i] < ' ') &&
                     (p[i] != '\n') && (p[i] != '\r')))
    buf[n] = '.';
else
    buf[n] = p[i];

Whereby, if the character at the position is < ' ' (ie our leading bytes of 12 etc), just print a ., don't do anything else.

There is a command line option which will parse unknown extensions -certopt ext_parse which yeilds output on a puppet certificate like this:

 X509v3 extensions:
            Netscape Comment:
                Puppet Server Internal Certificate
            X509v3 Authority Key Identifier:
                keyid:1E:13:A2:74:61:2C:6E:D7:24:AD:6E:33:FB:78:85:EF:9F:3C:E4:D2

            X509v3 Subject Key Identifier:
                0F:E7:63:10:C2:C1:45:C0:B1:B3:E4:C5:50:54:52:70:F1:77:09:CF
            X509v3 Subject Alternative Name:
                DNS:puppet, DNS:gcpvault-1.c.wf-sre-dev.internal
            1.3.6.1.4.1.34380.1.1.1:
    0:d=0  hl=2 l=  36 prim: UTF8STRING        :ED803750-E3C7-44F5-BB08-41A04433FE2E

            1.3.6.1.4.1.34380.1.1.3:
    0:d=0  hl=2 l=  12 prim: UTF8STRING        :my_ami_image

            1.3.6.1.4.1.34380.1.1.4:
    0:d=0  hl=2 l=  36 prim: UTF8STRING        :342thbjkt82094y0uthhor289jnqthpc2290

This discussion is on going see openssl/openssl#4288 and goes back to openssl/openssl#1 .

As a result, it appears to be safe to use the encode/asn1 package to Unmarshal the value to a string and move on. Any bad values will just be zeroed out. Thanks for continuing to push on this. I never thought I would end up reading the ASN1 library in 3 different languages. Happy to share more of what I've learned if it helps.

jefferai · 2017-12-15T14:15:26Z

Wowza. Thanks for digging in so deeply!

In hindsight, this is the obvious correct way to proceed, but you know what they say about hindsight...

I'll re-review and hopefully we can get this in for 0.9.1!

jefferai · 2017-12-15T14:44:34Z

builtin/credential/cert/path_certs.go

+			"required_extensions": &framework.FieldSchema{
+				Type: framework.TypeCommaStringSlice,
+				Description: `A comma-separated list of extensions
+formatted as "$oid:value". All values much match. Supports globbing on $value.`,


Please note in the comment that it can be a comma-separated string or an array.

jefferai · 2017-12-15T14:48:30Z

website/source/api/auth/cert/index.html.md

@@ -33,6 +33,8 @@ Sets a CA cert and associated parameters in a role name.
  the client certificate with a [globbed pattern]
  (https://github.com/ryanuber/go-glob/blob/master/README.md#example). Value is 
  a comma-separated list of patterns.  Authentication requires at least one Name matching at least one pattern.  If not set, defaults to allowing all names.
+- `required_extensions` `(string: "")` - Require specific Custom Extension OIDs to exist and match the pattern. 
+   Value is a comma separated list of `oid:glob,oid:glob`. All conditions _must_ be met.


Same comment here -- comma separated string or an array.

jefferai

Looks great! Just some comment fixing.

traviscosgrave · 2017-12-15T21:56:33Z

I think I messed up getting up to date :(

chrishoffman

Looks good!

jefferai · 2017-12-18T17:53:36Z

Thanks a bunch...merging!

jefferai added this to the 0.9.1 milestone Dec 4, 2017

jefferai reviewed Dec 4, 2017

View reviewed changes

PR Comment Changes

47e9816

Use SplitN over Split for custom extension configuration to avoid missing values containing :. Simply return false on the first required custom extension faliure. Format oneline function correctly.

vishalnayak reviewed Dec 7, 2017

View reviewed changes

jefferai requested changes Dec 8, 2017

View reviewed changes

traviscosgrave added 2 commits December 8, 2017 22:14

Trim ASN.1 Tag from Custom Extensions value bytes by dropping the fir…

459860e

…st two bytes and assume the value is a string.

Fixes typo in test name

b97f478

Use ASN1 to Unmarshal extensions into string values

ffc8487

jefferai reviewed Dec 15, 2017

View reviewed changes

jefferai requested a review from briankassouf December 15, 2017 14:48

updating docs based on review feedback

f5e1929

traviscosgrave force-pushed the check_cert_extensions branch from e2999b6 to f5e1929 Compare December 15, 2017 22:07

jefferai approved these changes Dec 18, 2017

View reviewed changes

chrishoffman approved these changes Dec 18, 2017

View reviewed changes

jefferai merged commit 95328e2 into hashicorp:master Dec 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Custom Cert Extensions as Cert Auth Constraint #3634

Use Custom Cert Extensions as Cert Auth Constraint #3634

traviscosgrave commented Nov 30, 2017

jefferai Dec 4, 2017

jefferai Dec 4, 2017

traviscosgrave Dec 4, 2017

jefferai Dec 6, 2017

traviscosgrave Dec 10, 2017

jefferai Dec 4, 2017

jefferai Dec 4, 2017

vishalnayak Dec 7, 2017

jefferai left a comment

traviscosgrave commented Dec 10, 2017

jefferai commented Dec 11, 2017

traviscosgrave commented Dec 12, 2017

jefferai commented Dec 14, 2017

traviscosgrave commented Dec 14, 2017

jefferai commented Dec 15, 2017

jefferai Dec 15, 2017

jefferai Dec 15, 2017

jefferai left a comment

traviscosgrave commented Dec 15, 2017

chrishoffman left a comment

jefferai commented Dec 18, 2017

Use Custom Cert Extensions as Cert Auth Constraint #3634

Use Custom Cert Extensions as Cert Auth Constraint #3634

Conversation

traviscosgrave commented Nov 30, 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

jefferai left a comment

Choose a reason for hiding this comment

traviscosgrave commented Dec 10, 2017

jefferai commented Dec 11, 2017

traviscosgrave commented Dec 12, 2017

jefferai commented Dec 14, 2017

traviscosgrave commented Dec 14, 2017

jefferai commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

traviscosgrave commented Dec 15, 2017

chrishoffman left a comment

Choose a reason for hiding this comment

jefferai commented Dec 18, 2017