allow roles to set OU value in certificates issued by the pki backend #2251

joemiller · 2017-01-10T05:36:48Z

Followup to #2220

jefferai · 2017-01-10T14:41:51Z

This looks pretty good. I had forgotten that the OU in pkix.Name is a []string although it makes sense. It got me thinking that probably multiple OUs should be allowed to be set, so that if people are filtering based on OU a particular cert can inhabit multiple (but with the list of OUs being encoded into a cert still being static per role rather than being chosen by the requestor).

Although a comma is allowed in an OU name, I think in practice that's very uncommon, so using a comma-separated string that can be split into the final list seems good (and easy to implement).

Thoughts?

joemiller · 2017-01-10T15:20:50Z

@jefferai yeah. Sounds good to me. I'll work on modifying the PR to support multiple OUs.

FWIW, since we've been using OU's in our certs for authorization purposes for several years now we have mostly chosen not to support/use them internally. While multiple OUs are supported by the specs the implementations vary. Most of our legacy codebase is still python and I recall that python does not (or maybe "did not at the time") support multiple OUs. Go's pki libs are good at supporting them though. That said, since it is technically supported I agree with supporting here as well.

jefferai · 2017-01-10T16:41:24Z

I would imagine in your use case the python code would still successfully handle it if only a single is set. It just seems like since Go's support is good, might as well fully support it since it's not too much effort.

joemiller · 2017-01-10T17:10:44Z

Well I had multiple-OUs implmented but strangely all tests with multiple OUs were failing. I'm running into what appears to be a bug or possibly intended behavior with golang's x509.CreateCertificate(). It appears that it will not populate a signed cert with multiple OU values and will only use the first value from the x509.Certificate template.

I haven't tracked down the exact spot in the stdlib where this happens yet but I did get some confirmation with a quick test using the cfssl toolset:

test-csr.json

{
    "hosts": [
      "localhost"
    ],
    "CN": "vault",
    "key": {
        "algo": "rsa",
        "size": 2048
    },
    "names": [
                        {
        "OU": "vault"
    },
                        {
        "OU": "another"
    }
                ]
}

cert gen:

$ cfssl selfsign ca multiple-ou-req.json | jq -r .cert | openssl x509 -subject -noout
subject= /OU=vault/CN=vault

Interestingly, golang will produce CSRs containing multiple OUs:

$ cfssl genkey multiple-ou-req.json| jq -r .csr | openssl req -subject -noout
subject=/OU=vault/OU=another/CN=vault

joemiller · 2017-01-10T18:09:44Z

Correct, I haven't run into any languages or libs that can't parse a multi-OU cert. They will just allow access to the first OU and hide the rest.

…

On Tue, Jan 10, 2017 at 8:41 AM Jeff Mitchell ***@***.***> wrote: I would imagine in your use case the python code would still successfully handle it if only a single is set. It just seems like since Go's support is good, might as well fully support it since it's not too much effort. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2251 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXDAwFUoXlQD8NiVLx7FyS2yM1PLWhaks5rQ7S5gaJpZM4LfDCv> .

joemiller · 2017-01-10T18:10:32Z

note: I have a few more tests to run to double-check that go is actually stripping extra OUs during CreateCertificate() calls. Will report back here.

joemiller · 2017-01-10T18:39:53Z

Tested with the sign-verbatim endpoint. It will consume a CSR with multiple-OU's but the resulting cert will only have a single OU.

I think this might be a bug - or possibly intended behavior - in the go stdlib.

joemiller · 2017-01-10T20:01:20Z

there are two commits on this PR. The first implements single-OU support, the second implements multiple-OU support even though the resulting certs signed will not have multiple OUs.

jefferai · 2017-01-11T02:27:46Z

cfssl actually rewrites a lot of the stdlib PKI code so it's not necessarily indicative of Go as a whole.

jefferai · 2017-01-11T04:07:18Z

I dug into this a bit but so far no dice. I followed things down from the CreateCertificate call through to the pkix bits into the asn1 marshaling and it seems like it should be maintaining the multiple-value slice the whole way through. I'll try to dig in more when I get a chance.

Alternately we could merge as-is and put a warning in the docs, but if we do that I'd ask that you open a bug report against golang/go so that this doesn't get totally dropped on the floor.

vishalnayak · 2017-01-11T07:09:52Z

builtin/logical/pki/cert_util.go

+	// Set OU (organizationalUnit) values if specified in the role
+	ou := []string{}
+	{
+		if role.OU != "" {


Can we make the OU field of roleEntry a []string instead of a string? Also, we can use the inbuilt helper function strutil.ParseDedupAndSortStrings() to sanitize the input before storing in the role. That way, the logic here translates to ou := role.OU.

The reason for suggesting this change is to avoid having to parse the role's fields in all the places they are retrieved.

I was thinking about that too; I realized that elsewhere in PKI we are doing this parsing every time, but that's because much of it was written around the 0.2.1 era.

When the role is read back via a GET it should come back with an array, so storing it as one is probably a good idea as you suggest.

updated to use strutil.ParseDedupAndSortStrings

joemiller · 2017-01-11T15:06:04Z

ok, my next steps:

keep multi-OU support in this PR
write a POC with the stdlib pki demonstrating the issue
submit bug against golang/go
update the web docs in this PR with note about the go bug
update with @vishalnayak 's change req

jefferai · 2017-01-11T15:23:05Z

Neato. 💯

joemiller · 2017-01-13T20:36:49Z

Working a bit more on this today. It appears the bug is actually in the x509.ParseCertificate() not x509.CreateCertificate(). Create properly creates/signs certs with multiple-OUs. But when those same certs are parsed only the first OU is returned.

Vault's pki backend does a Create() followed by a Parse() which is why we are seeing the certs signed/issued by Vault dropping the additional OUs:

	certBytes, err = x509.CreateCertificate(rand.Reader, certTemplate, caCert, csr.PublicKey, creationInfo.SigningBundle.PrivateKey)

	if err != nil {
		return nil, errutil.InternalError{Err: fmt.Sprintf("unable to create certificate: %s", err)}
	}

	result.CertificateBytes = certBytes
	result.Certificate, err = x509.ParseCertificate(certBytes)

https://github.com/hashicorp/vault/blob/master/builtin/logical/pki/cert_util.go#L1028

POC code demonstrating the behavior in this gist: https://gist.github.com/joemiller/c97a52d46cae0a4b38df841db8307fc4

jefferai · 2017-01-13T20:54:58Z

Neat find! Since what gets returned from Vault are the PEM encoded bytes it seems like it should be fine then if what you pass that PEM block to isn't also Go code.

Can you raise this with golang/go?

joemiller · 2017-01-13T21:58:53Z

golang/go issue golang/go#18654

joemiller · 2017-01-14T18:18:48Z

Not quite sure the root of the issue yet, but I am able to get the unit tests in this PR to pass when using go 1.8rc1. The issue, whatever it is, is not quite as consistent as I thought (see the comments in the open issue on golang/go). In any case I'm having better results with 1.8rc1 which is nice.

joemiller · 2017-01-14T18:42:28Z

Another update ... =)

The issue on golang/go was closed due to the bug having already been resolved in 1.8.

jefferai · 2017-01-14T18:50:59Z

Yep, I was watching it!

jefferai · 2017-01-23T02:26:14Z

@joemiller What's your thinking on this PR at this point? Everything seem good for a final review/merge?

joemiller · 2017-01-23T17:21:19Z

@jefferai Tests should be passing with go1.8. I believe the code is ready to go. Now it is a matter of how to release, I think.

The multi-OU unit test could be commented out so that the builds pass on go < 1.8. Possibly a note in the pki doc about multi-OU signing not working unless built with go1.8+. Or, hold this PR until go1.8 is GA (should be Jan 31st). I am not sure what the best path forward for release is since I'm not familiar with the details of Hashi's release and build policies for Vault. I am happy to help in any way I can though.

joemiller · 2017-01-23T17:22:20Z

@jefferai Also @vishalnayak's review on using strutil.ParseDedupAndSortStrings appears to be still open and will need to be resolved before merge. I made the requested change and it is ready for review.

jefferai · 2017-01-23T17:39:08Z

@joemiller 0.7 will require Go 1.8, so if you're happy using a local build until then, we could defer this to 0.7 (mid March, probably). But, we were thinking mid next week for 0.6.5 which should allow us to depend on 1.8 for 0.6.5 too, ideally. It also wouldn't be the first time we used a Go RC for a minor release to solve specific bugs...one of the benefits of building statically :-)

So I think we assume we'll use Go 1.8 in some way shape or form for 0.6.5 and just merge it.

jefferai · 2017-01-23T17:44:39Z

Merging! Thanks!

vishalnayak requested changes Jan 11, 2017

View reviewed changes

joemiller mentioned this pull request Jan 13, 2017

crypto/x509: ParseCertificate ignores all but first value from Subject's []string{} fields golang/go#18654

Closed

allow roles to set OU value in certificates issued by the pki backend

81851c6

jefferai mentioned this pull request Jan 23, 2017

support setting certificate subject OU value in pki backend #2240

Closed

1 task

jefferai merged commit 90e3251 into hashicorp:master Jan 23, 2017

jefferai added this to the 0.6.5 milestone Feb 7, 2017

joemiller mentioned this pull request Sep 3, 2018

add allowed_organizational_units parameter to cert credential backend #5252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow roles to set OU value in certificates issued by the pki backend #2251

allow roles to set OU value in certificates issued by the pki backend #2251

joemiller commented Jan 10, 2017

jefferai commented Jan 10, 2017

joemiller commented Jan 10, 2017

jefferai commented Jan 10, 2017

joemiller commented Jan 10, 2017 •

edited

Loading

joemiller commented Jan 10, 2017 via email

joemiller commented Jan 10, 2017

joemiller commented Jan 10, 2017

joemiller commented Jan 10, 2017

jefferai commented Jan 11, 2017

jefferai commented Jan 11, 2017

vishalnayak Jan 11, 2017

jefferai Jan 11, 2017

joemiller Jan 13, 2017

joemiller commented Jan 11, 2017 •

edited

Loading

jefferai commented Jan 11, 2017

joemiller commented Jan 13, 2017

jefferai commented Jan 13, 2017

joemiller commented Jan 13, 2017

joemiller commented Jan 14, 2017

joemiller commented Jan 14, 2017

jefferai commented Jan 14, 2017

jefferai commented Jan 23, 2017

joemiller commented Jan 23, 2017

joemiller commented Jan 23, 2017

jefferai commented Jan 23, 2017

jefferai commented Jan 23, 2017

allow roles to set OU value in certificates issued by the pki backend #2251

allow roles to set OU value in certificates issued by the pki backend #2251

Conversation

joemiller commented Jan 10, 2017

jefferai commented Jan 10, 2017

joemiller commented Jan 10, 2017

jefferai commented Jan 10, 2017

joemiller commented Jan 10, 2017 • edited Loading

joemiller commented Jan 10, 2017 via email

joemiller commented Jan 10, 2017

joemiller commented Jan 10, 2017

joemiller commented Jan 10, 2017

jefferai commented Jan 11, 2017

jefferai commented Jan 11, 2017

vishalnayak Jan 11, 2017

Choose a reason for hiding this comment

jefferai Jan 11, 2017

Choose a reason for hiding this comment

joemiller Jan 13, 2017

Choose a reason for hiding this comment

joemiller commented Jan 11, 2017 • edited Loading

jefferai commented Jan 11, 2017

joemiller commented Jan 13, 2017

jefferai commented Jan 13, 2017

joemiller commented Jan 13, 2017

joemiller commented Jan 14, 2017

joemiller commented Jan 14, 2017

jefferai commented Jan 14, 2017

jefferai commented Jan 23, 2017

joemiller commented Jan 23, 2017

joemiller commented Jan 23, 2017

jefferai commented Jan 23, 2017

jefferai commented Jan 23, 2017

joemiller commented Jan 10, 2017 •

edited

Loading

joemiller commented Jan 11, 2017 •

edited

Loading