-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Use Custom Cert Extensions as Cert Auth Constraint #3634
Changes from 2 commits
f46c010
47e9816
459860e
b97f478
ffc8487
f5e1929
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,12 @@ Must be x509 PEM encoded.`, | |
At least one must exist in either the Common Name or SANs. Supports globbing.`, | ||
}, | ||
|
||
"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.`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please note in the comment that it can be a comma-separated string or an array. |
||
}, | ||
|
||
"display_name": &framework.FieldSchema{ | ||
Type: framework.TypeString, | ||
Description: `The display name to use for clients using this | ||
|
@@ -146,6 +152,7 @@ func (b *backend) pathCertWrite( | |
displayName := d.Get("display_name").(string) | ||
policies := policyutil.ParsePolicies(d.Get("policies")) | ||
allowedNames := d.Get("allowed_names").([]string) | ||
requiredExtensions := d.Get("required_extensions").([]string) | ||
|
||
// Default the display name to the certificate name if not given | ||
if displayName == "" { | ||
|
@@ -172,11 +179,12 @@ func (b *backend) pathCertWrite( | |
} | ||
|
||
certEntry := &CertEntry{ | ||
Name: name, | ||
Certificate: certificate, | ||
DisplayName: displayName, | ||
Policies: policies, | ||
AllowedNames: allowedNames, | ||
Name: name, | ||
Certificate: certificate, | ||
DisplayName: displayName, | ||
Policies: policies, | ||
AllowedNames: allowedNames, | ||
RequiredExtensions: requiredExtensions, | ||
} | ||
|
||
// Parse the lease duration or default to backend/system default | ||
|
@@ -204,12 +212,13 @@ func (b *backend) pathCertWrite( | |
} | ||
|
||
type CertEntry struct { | ||
Name string | ||
Certificate string | ||
DisplayName string | ||
Policies []string | ||
TTL time.Duration | ||
AllowedNames []string | ||
Name string | ||
Certificate string | ||
DisplayName string | ||
Policies []string | ||
TTL time.Duration | ||
AllowedNames []string | ||
RequiredExtensions []string | ||
} | ||
|
||
const pathCertHelpSyn = ` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,28 +237,63 @@ func (b *backend) verifyCredentials(req *logical.Request, d *framework.FieldData | |
} | ||
|
||
func (b *backend) matchesConstraints(clientCert *x509.Certificate, trustedChain []*x509.Certificate, config *ParsedCert) bool { | ||
return !b.checkForChainInCRLs(trustedChain) && | ||
b.matchesNames(clientCert, config) && | ||
b.matchesCertificateExtenions(clientCert, config) | ||
} | ||
|
||
// matchesNames verifies that the certificate matches at least one configured | ||
// allowed name | ||
func (b *backend) matchesNames(clientCert *x509.Certificate, config *ParsedCert) bool { | ||
// Default behavior (no names) is to allow all names | ||
nameMatched := len(config.Entry.AllowedNames) == 0 | ||
if len(config.Entry.AllowedNames) == 0 { | ||
return true | ||
} | ||
// At least one pattern must match at least one name if any patterns are specified | ||
for _, allowedName := range config.Entry.AllowedNames { | ||
if glob.Glob(allowedName, clientCert.Subject.CommonName) { | ||
nameMatched = true | ||
return true | ||
} | ||
|
||
for _, name := range clientCert.DNSNames { | ||
if glob.Glob(allowedName, name) { | ||
nameMatched = true | ||
return true | ||
} | ||
} | ||
|
||
for _, name := range clientCert.EmailAddresses { | ||
if glob.Glob(allowedName, name) { | ||
nameMatched = true | ||
return true | ||
} | ||
} | ||
} | ||
return false | ||
} | ||
|
||
// matchesCertificateExtenions verifies that the certificate matches configured | ||
// required extensions | ||
func (b *backend) matchesCertificateExtenions(clientCert *x509.Certificate, config *ParsedCert) bool { | ||
// Build Client Extensions Map | ||
clientExtMap := map[string]string{} | ||
for _, ext := range clientCert.Extensions { | ||
// Trim prefix control characters from the Custom Extension Value for human readable configs | ||
clientExtMap[ext.Id.String()] = strings.TrimLeftFunc(string(ext.Value[:]), b.isControlRune) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you leave a comment on the purpose of this line? Why just trimming the left side? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
This is the result of generating the certificate using the 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 In general, the values within the
That expectation could just be a result of being new to working with certificates programmatically. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When viewing with openssl such a generated cert also looks strange:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
} | ||
|
||
// If any of the required extensions don't match the constraint fails | ||
for _, requiredExt := range config.Entry.RequiredExtensions { | ||
reqExt := strings.SplitN(requiredExt, ":", 2) | ||
clientExtValue, clientExtValueExists := clientExtMap[reqExt[0]] | ||
if !clientExtValueExists || !glob.Glob(reqExt[1], clientExtValue) { | ||
return false | ||
} | ||
} | ||
return true | ||
} | ||
|
||
return !b.checkForChainInCRLs(trustedChain) && nameMatched | ||
// isControlRune returns true for control charaters for trimming extension values | ||
func (b *backend) isControlRune(r rune) bool { | ||
return r <= 32 || r == 127 | ||
} | ||
|
||
// loadTrustedCerts is used to load all the trusted certificates from the backend | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
92223EAFBBEE17A3 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
[ req ] | ||
default_bits = 2048 | ||
encrypt_key = no | ||
prompt = no | ||
default_md = sha256 | ||
req_extensions = req_v3 | ||
distinguished_name = dn | ||
|
||
[ dn ] | ||
CN = example.com | ||
|
||
[ req_v3 ] | ||
subjectAltName = @alt_names | ||
2.1.1.1=ASN1:UTF8String:A UTF8String Extension | ||
2.1.1.2=ASN1:UTF8:A UTF8 Extension | ||
2.1.1.3=ASN1:IA5:An IA5 Extension | ||
2.1.1.4=ASN1:VISIBLE:A Visible Extension | ||
|
||
[ alt_names ] | ||
DNS.1 = example.com | ||
IP.1 = 127.0.0.1 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
-----BEGIN CERTIFICATE REQUEST----- | ||
MIIDAzCCAesCAQAwFjEUMBIGA1UEAwwLZXhhbXBsZS5jb20wggEiMA0GCSqGSIb3 | ||
DQEBAQUAA4IBDwAwggEKAoIBAQDM2PrLyK/wVQIcnK362ZylDrIVMjFQzps/0AxM | ||
ke+8MNPMArBlSAhnZus6qb0nN0nJrDLkHQgYqnSvK9N7VUv/xFblEcOLBlciLhyN | ||
Wkm92+q/M/xOvUVmnYkN3XgTI5QNxF7ZWDFHmwCNV27RraQZou0hG7yvyoILLMQE | ||
3MnMCNM1nZ9JIuBMcRsZLGqQ1XNaQljboRVIUjimzkcfYyTruhLosTIbwForp78J | ||
MzHHqVjtLJXPqUnRMS7KhGMj1f2mIswQzCv6F2PWEzNBbP4Gb67znKikKDs0RgyL | ||
RyfizFNFJSC58XntK8jwHK1D8W3UepFf4K8xNFnhPoKWtWfJAgMBAAGggacwgaQG | ||
CSqGSIb3DQEJDjGBljCBkzAcBgNVHREEFTATggtleGFtcGxlLmNvbYcEfwAAATAf | ||
BgNRAQEEGAwWQSBVVEY4U3RyaW5nIEV4dGVuc2lvbjAZBgNRAQIEEgwQQSBVVEY4 | ||
IEV4dGVuc2lvbjAZBgNRAQMEEhYQQW4gSUE1IEV4dGVuc2lvbjAcBgNRAQQEFRoT | ||
QSBWaXNpYmxlIEV4dGVuc2lvbjANBgkqhkiG9w0BAQsFAAOCAQEAtYjewBcqAXxk | ||
tDY0lpZid6ZvfngdDlDZX0vrs3zNppKNe5Sl+jsoDOexqTA7HQA/y1ru117sAEeB | ||
yiqMeZ7oPk8b3w+BZUpab7p2qPMhZypKl93y/jGXGscc3jRbUBnym9S91PSq6wUd | ||
f2aigSqFc9+ywFVdx5PnnZUfcrUQ2a+AweYEkGOzXX2Ga+Ige8grDMCzRgCoP5cW | ||
kM5ghwZp5wYIBGrKBU9iDcBlmnNhYaGWf+dD00JtVDPNn2bJnCsJHIO0nklZgnrS | ||
fli8VQ1nYPkONdkiRYLt6//6at1iNDoDgsVCChtlVkLpxFIKcDFUHlffZsc1kMFI | ||
HTX579k8hA== | ||
-----END CERTIFICATE REQUEST----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
-----BEGIN CERTIFICATE----- | ||
MIIDRjCCAi6gAwIBAgIJAJIiPq+77hejMA0GCSqGSIb3DQEBCwUAMBYxFDASBgNV | ||
BAMTC2V4YW1wbGUuY29tMB4XDTE3MTEyOTE5MTgwM1oXDTI3MTEyNzE5MTgwM1ow | ||
FjEUMBIGA1UEAwwLZXhhbXBsZS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAw | ||
ggEKAoIBAQDM2PrLyK/wVQIcnK362ZylDrIVMjFQzps/0AxMke+8MNPMArBlSAhn | ||
Zus6qb0nN0nJrDLkHQgYqnSvK9N7VUv/xFblEcOLBlciLhyNWkm92+q/M/xOvUVm | ||
nYkN3XgTI5QNxF7ZWDFHmwCNV27RraQZou0hG7yvyoILLMQE3MnMCNM1nZ9JIuBM | ||
cRsZLGqQ1XNaQljboRVIUjimzkcfYyTruhLosTIbwForp78JMzHHqVjtLJXPqUnR | ||
MS7KhGMj1f2mIswQzCv6F2PWEzNBbP4Gb67znKikKDs0RgyLRyfizFNFJSC58Xnt | ||
K8jwHK1D8W3UepFf4K8xNFnhPoKWtWfJAgMBAAGjgZYwgZMwHAYDVR0RBBUwE4IL | ||
ZXhhbXBsZS5jb22HBH8AAAEwHwYDUQEBBBgMFkEgVVRGOFN0cmluZyBFeHRlbnNp | ||
b24wGQYDUQECBBIMEEEgVVRGOCBFeHRlbnNpb24wGQYDUQEDBBIWEEFuIElBNSBF | ||
eHRlbnNpb24wHAYDUQEEBBUaE0EgVmlzaWJsZSBFeHRlbnNpb24wDQYJKoZIhvcN | ||
AQELBQADggEBAGU/iA6saupEaGn/veVNCknFGDL7pst5D6eX/y9atXlBOdJe7ZJJ | ||
XQRkeHJldA0khVpzH7Ryfi+/25WDuNz+XTZqmb4ppeV8g9amtqBwxziQ9UUwYrza | ||
eDBqdXBaYp/iHUEHoceX4F44xuo80BIqwF0lD9TFNUFoILnF26ajhKX0xkGaiKTH | ||
6SbjBfHoQVMzOHokVRWregmgNycV+MAI9Ne9XkIZvdOYeNlcS9drZeJI3szkiaxB | ||
WWaWaAr5UU2Z0yUCZnAIDMRcIiUbSEjIDz504sSuCzTctMOxWZu0r/0UrXRzwZZi | ||
HAaKm3MUmBh733ChP4rTB58nr5DEr5rJ9P8= | ||
-----END CERTIFICATE----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
-----BEGIN PRIVATE KEY----- | ||
MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDM2PrLyK/wVQIc | ||
nK362ZylDrIVMjFQzps/0AxMke+8MNPMArBlSAhnZus6qb0nN0nJrDLkHQgYqnSv | ||
K9N7VUv/xFblEcOLBlciLhyNWkm92+q/M/xOvUVmnYkN3XgTI5QNxF7ZWDFHmwCN | ||
V27RraQZou0hG7yvyoILLMQE3MnMCNM1nZ9JIuBMcRsZLGqQ1XNaQljboRVIUjim | ||
zkcfYyTruhLosTIbwForp78JMzHHqVjtLJXPqUnRMS7KhGMj1f2mIswQzCv6F2PW | ||
EzNBbP4Gb67znKikKDs0RgyLRyfizFNFJSC58XntK8jwHK1D8W3UepFf4K8xNFnh | ||
PoKWtWfJAgMBAAECggEAW7hLkzMok9N8PpNo0wjcuor58cOnkSbxHIFrAF3XmcvD | ||
CXWqxa6bFLFgYcPejdCTmVkg8EKPfXvVAxn8dxyaCss+nRJ3G6ibGxLKdgAXRItT | ||
cIk2T4svp+KhmzOur+MeR4vFbEuwxP8CIEclt3yoHVJ2Gnzw30UtNRO2MPcq48/C | ||
ZODGeBqUif1EGjDAvlqu5kl/pcDBJ3ctIZdVUMYYW4R9JtzKsmwhX7CRCBm8k5hG | ||
2uzn8AKwpuVtfWcnX59UUmHGJ8mjETuNLARRAwWBWhl8f7wckmi+PKERJGEM2QE5 | ||
/Voy0p22zmQ3waS8LgiI7YHCAEFqjVWNziVGdR36gQKBgQDxkpfkEsfa5PieIaaF | ||
iQOO0rrjEJ9MBOQqmTDeclmDPNkM9qvCF/dqpJfOtliYFxd7JJ3OR2wKrBb5vGHt | ||
qIB51Rnm9aDTM4OUEhnhvbPlERD0W+yWYXWRvqyHz0GYwEFGQ83h95GC/qfTosqy | ||
LEzYLDafiPeNP+DG/HYRljAxUwKBgQDZFOWHEcZkSFPLNZiksHqs90OR2zIFxZcx | ||
SrbkjqXjRjehWEAwgpvQ/quSBxrE2E8xXgVm90G1JpWzxjUfKKQRM6solQeEpnwY | ||
kCy2Ozij/TtbLNRlU65UQ+nMto8KTSIyJbxxdOZxYdtJAJQp1FJO1a1WC11z4+zh | ||
lnLV1O5S8wKBgQCDf/QU4DBQtNGtas315Oa96XJ4RkUgoYz+r1NN09tsOERC7UgE | ||
KP2y3JQSn2pMqE1M6FrKvlBO4uzC10xLja0aJOmrssvwDBu1D8FtA9IYgJjFHAEG | ||
v1i7lJrgdu7TUtx1flVli1l3gF4lM3m5UaonBrJZV7rB9iLKzwUKf8IOJwKBgFt/ | ||
QktPA6brEV56Za8sr1hOFA3bLNdf9B0Tl8j4ExWbWAFKeCu6MUDCxsAS/IZxgdeW | ||
AILovqpC7CBM78EFWTni5EaDohqYLYAQ7LeWeIYuSyFf4Nogjj74LQha/iliX4Jx | ||
g17y3dp2W34Gn2yOEG8oAxpcSfR54jMnPZnBWP5fAoGBAMNAd3oa/xq9A5v719ik | ||
naD7PdrjBdhnPk4egzMDv54y6pCFlvFbEiBduBWTmiVa7dSzhYtmEbri2WrgARlu | ||
vkfTnVH9E8Hnm4HTbNn+ebxrofq1AOAvdApSoslsOP1NT9J6zB89RzChJyzjbIQR | ||
Gevrutb4uO9qpB1jDVoMmGde | ||
-----END PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment here -- comma separated string or an array. |
||
- `policies` `(string: "")` - A comma-separated list of policies to set on tokens | ||
issued when authenticating against this CA certificate. | ||
- `display_name` `(string: "")` - The `display_name` to set on tokens issued | ||
|
@@ -93,6 +95,7 @@ $ curl \ | |
"display_name": "test", | ||
"policies": "", | ||
"allowed_names": "", | ||
"required_extensions": "", | ||
"ttl": 2764800 | ||
}, | ||
"warnings": null, | ||
|
@@ -327,4 +330,4 @@ $ curl \ | |
"renewable": true, | ||
} | ||
} | ||
``` | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: signle/single