Validate Subject DN entries during Certificate based authentication with Vault

[palsaurabh2005]

Why?

Vault should be able to verify individual Subject DN entries of a presented client certificate.
A subject DN is particularly important when a PKI is backed by LDAP and its almost necessary at times
to match DN fields before access could be granted.
Further, its not always possible to tweak the PKI on request to get DN entries in certificate extensions.

Furthermore, as of today, we provide options to check for allowed_common_names and allowed_organizational_units(0.11.2).
Instead of adding a check for each of the Subject DN fields, there should be a unified way of checking any of the Subject DN entries.
Similar to how the "required_extensions" check works.

How:

With this PR I am pushing in changes that will allow Vault to verify Subject DN entries in a client certificate.
These entries will be matched against a config property "required_subject_oids".
The format for "required_subject_oids" is a comma separated array or string of form oid:value and globbing of value is supported.
For example:
A config entry for required_subject_oids as: "0.9.2342.19200300.100.1.1:TheUID,2.5.4.3:example.com,2.5.4.6:US,2.5.4.8:CA,2.5.4.7:Sunnyvale,2.5.4.10:ExampleOrg,2.5.4.11:ExampleDivision1,2.5.4.11:Example*2,2.5.4.11:ExampleDivision3,0.9.2342.19200300.100.1.25:ExampleDC,2.5.4.4:ExampleSN"
will require the Client certificate to contain a Subject DN with entries as:

UID = TheUID
CN = example.com
C = US
ST = CA
L = Sunnyvale
O = ExampleOrg
0.OU = ExampleDivision1
1.OU = ExampleDivision2
2.OU = ExampleDivision3
DC = ExampleDC
SN = ExampleSN

Note that all these fields should match for the the certificate verification to pass.

The test cases cover most of the scenarios:
./builtin/credential/cert/backend_test.go#TestBackend_subjectoids_singleCert()

To execute the test cases:
make test TEST=./builtin/credential/cert

Ref: https://tools.ietf.org/html/rfc5280#section-4.1.2.4
https://www.cryptosys.net/pki/manpki/pki_distnames.html

Related conversation on this topic @jefferai @briankassouf :
https://groups.google.com/forum/#!topic/vault-tool/3XzNAaKWi9U

[palsaurabh2005]

Hello Admin(s),
your thoughts..does this approach makes sense for Validating Subject fields or the guideline is to write a parameter check for each of the Subject entries as it is done today for common_name and OU ?
If so I can drop the PR keeping the implementation local.
As stated in the PR description, this design allows for validating multiple Subject entry fields including arbitrary relative DN's.

Thanks.
@jefferai @briankassouf @chrishoffman

[palsaurabh2005]

@bluecmd @michaelansel @vishalnayak @joemiller @armon @traviscosgrave
Requesting feedback/review.

Thanks!

[bluecmd]

Nice! To me this makes a lot of sense -- I just scrolled through the code, I'm more reviewing the idea.
I'm a bit concerned about the reviewability of the resulting policies. Is there any way so that:

1. Well-known UIDs can be entered using name instead (like how they are in openssl.cnf)?
2. The call is a map instead of a (k, v) tuple list?

I'm thinking something like:

	r := &vault.Whatever{
		RequiredExtensions: map[string]string{
			"uid": "TheUID",
			"dc": "ExampleDC",
			"0.OU" "ExampleDivision1", // Case in-sensitive
			"2.5.4.3": "example.com", // Supports explicit extensions as well
		},
	}

I think those API calls would be 1) much more user friendly, 2) easier to audit, and 3) harder to screw up.
I'm not sure what to return in the case of a read of the policy however.

[palsaurabh2005]

@bluecmd Thanks for the feedback. My comments inline.

Nice! To me this makes a lot of sense -- I just scrolled through the code, I'm more reviewing the idea.
I'm a bit concerned about the reviewability of the resulting policies. Is there any way so that:

The resulting policy would still be determined by a defined role at the cert endpoint. Nothing changes there.

1. Well-known UIDs can be entered using name instead (like how they are in openssl.cnf)?
2. The call is a map instead of a (k, v) tuple list?

I'm thinking something like:

	r := &vault.Whatever{
		RequiredExtensions: map[string]string{
			"uid": "TheUID",
			"dc": "ExampleDC",
			"0.OU" "ExampleDivision1", // Case in-sensitive
			"2.5.4.3": "example.com", // Supports explicit extensions as well
		},
	}

I think those API calls would be 1) much more user friendly, 2) easier to audit, and 3) harder to screw up.

- On the user friendliness, I do agree that readable names would be easier for operators, but considering that this will also support arbitrary relative DN's, it might be a good idea to just keep it consistent in OID format; similar to the required_extensions check in cert auth. But if the consensus on this is to keep it readable, it will change the interface.
- On the Audit side of things, the log would contain the same data as today:

"metadata": {
        "authority_key_id": "...",
        "cert_name": "<<cert role name>>",
        "common_name": "...",
        "serial_number": "...",
        "subject_key_id": "..."
      }

- "harder to screw up" : Core configs would be done by operators and not general users and this will be a relatively low recurring change. But yes, mistakes will be easier to identify if the names are more readable.

I'm not sure what to return in the case of a read of the policy however.

The policy configured for the cert auth role.

[hashicorp-cla]

All committers have signed the CLA.

[jefferai]

I'm adding this to the next release milestone for visibility; note that I won't be making the final decision on when it might land.

Also, nobody from HC can review this until you accept the CLA, so that's a hard prerequisite before we can proceed.

[palsaurabh2005]

Thank you @jefferai .
The CLA is signed and the branch is up to date with master. Some changes were needed to my initial commit since the branch was out of sync with master; now the build and tests look good.

[desperados1999]

Hi @jefferai , when can we expect this to be merged?

[jefferai]

@desperados1999 No idea. It hasn't been discussed by the Vault team yet, so far as I know; there are concerns about its implementation raised in the comments above.

[aphorise]

hey @palsaurabh2005 any chance you can resolve the merge conflict here - I appreciate a lot has changed and I'm not entirely certain if it will be merged - but then having said that I cant think of reasons why not since all these properties are user configurable. Thank you for your efforts and suggestion here.

[aphorise]

@cipherboy any inputs you may have on this would be welcome too.

[palsaurabh2005]

Hey @aphorise , sorry about the delay in getting back to you.
A lot has certainly changed over 4 years; I will try and work on resolving the merge conflicts in the coming weeks. Not sure I will need to add more test cases to cover any new scenarios, but I will try and get it up to date.