-
Notifications
You must be signed in to change notification settings - Fork 5.5k
tls: Add support for matching against OtherName SAN type #34471
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
Changes from 13 commits
1c9c914
d1f167f
7d9fa67
c95587b
3aa7618
27107f3
fa8c908
8be3f1b
5004b55
543aa7e
20a5d9f
0318aa4
dfb8a23
80e6037
bef4d58
2d9bfb7
d800848
70a5cb9
ded0296
90a980e
d3c7937
b1de297
06d4848
c773c79
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 |
|---|---|---|
|
|
@@ -226,6 +226,36 @@ std::string Utility::generalNameAsString(const GENERAL_NAME* general_name) { | |
| } | ||
| break; | ||
| } | ||
| case GEN_OTHERNAME: | ||
| ASN1_TYPE* value = general_name->d.otherName->value; | ||
| if (value == nullptr) { | ||
| break; | ||
| } | ||
| switch (value->type) { | ||
|
Member
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. Should the matcher specify the expected type to avoid any potential bypass of the checks, similar to the change made in #18628? Or is that overkill here? cc @yanavlasov @pradeepcrao .
Contributor
Author
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. @yanavlasov @pradeepcrao friendly ping for any thoughts here. There is admittedly room for bypass, but I'm not sure if the principle to create a distinction between DNS/IP/EMAIL/URI applies here between data types. Happy to make any changes if required.
Contributor
Author
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. As a follow up, I checked the RFC for this. Nothing is clearly stated about the OtherName semantics in the SAN subsection - pretty much that the OID speaks for the type and the value speaks for the name. The name constraints subsection and the above section however go into detail as to what is expected of a DNS, URI, IP, EMAIL SAN and the format and semantics it should take and when certificates should be rejected. Adding #18628 helps us conform to that. Having an DNS SAN with value "google.com" matched against a IP expectation should fail since IP SAN would be expected to have certain semantics along with the fact that those types are individually distinguished. For othername SAN, they just specify this:
Considering we have an OID matching which is probably what should define type here, it might just be enough based on my understanding of RFC. On a side note, I also noticed another RFC which tries to define semantics very specific to an usecase, but nothing generic. |
||
| case V_ASN1_NULL: | ||
| break; | ||
| case V_ASN1_BOOLEAN: | ||
| san = value->value.boolean ? "true" : "false"; | ||
| break; | ||
| case V_ASN1_ENUMERATED: | ||
| san = std::to_string(ASN1_ENUMERATED_get(value->value.enumerated)); | ||
|
arulthileeban marked this conversation as resolved.
Outdated
|
||
| break; | ||
| case V_ASN1_INTEGER: | ||
| san = std::to_string(ASN1_INTEGER_get(value->value.integer)); | ||
|
arulthileeban marked this conversation as resolved.
Outdated
|
||
| break; | ||
| case V_ASN1_OBJECT: { | ||
| char tmp_obj[256]; // OID Max length | ||
| if (OBJ_obj2txt(tmp_obj, 256, value->value.object, 1) < 0) { | ||
|
arulthileeban marked this conversation as resolved.
Outdated
|
||
| break; | ||
| } | ||
| san.assign(tmp_obj); | ||
| break; | ||
|
arulthileeban marked this conversation as resolved.
|
||
| } | ||
| default: | ||
|
arulthileeban marked this conversation as resolved.
|
||
| str = value->value.asn1_string; | ||
| san.assign(reinterpret_cast<const char*>(ASN1_STRING_data(str)), ASN1_STRING_length(str)); | ||
| break; | ||
|
arulthileeban marked this conversation as resolved.
|
||
| } | ||
| } | ||
| return san; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| [req] | ||
| distinguished_name = req_distinguished_name | ||
| req_extensions = v3_req | ||
|
|
||
| [req_distinguished_name] | ||
| countryName = US | ||
| countryName_default = US | ||
| stateOrProvinceName = California | ||
| stateOrProvinceName_default = California | ||
| localityName = San Francisco | ||
| localityName_default = San Francisco | ||
| organizationName = Lyft | ||
| organizationName_default = Lyft | ||
| organizationalUnitName = Lyft Engineering | ||
| organizationalUnitName_default = Lyft Engineering | ||
| commonName = Test Server | ||
| commonName_default = Test Server | ||
| commonName_max = 64 | ||
|
|
||
| [v3_req] | ||
| basicConstraints = CA:FALSE | ||
| keyUsage = nonRepudiation, digitalSignature, keyEncipherment | ||
| extendedKeyUsage = clientAuth, serverAuth | ||
| subjectAltName = @alt_names | ||
| subjectKeyIdentifier = hash | ||
|
|
||
| [v3_ca] | ||
| basicConstraints = critical, CA:FALSE | ||
| keyUsage = nonRepudiation, digitalSignature, keyEncipherment | ||
| extendedKeyUsage = clientAuth, serverAuth | ||
| subjectAltName = @alt_names | ||
| subjectKeyIdentifier = hash | ||
| authorityKeyIdentifier = keyid:always | ||
|
|
||
| [alt_names] | ||
| DNS.1 = server1.example.com | ||
| otherName.1 = 1.3.6.1.4.1.311.20.2.3;UTF8:server1.example.com |
Uh oh!
There was an error while loading. Please reload this page.