Re: Using UPN from subjectAltName with SSLUserName
On Sun, Aug 02, 2015 at 09:33:48AM +0200, Kaspar Brand wrote: > On 19.07.2015 17:24, Kaspar Brand wrote: > > But, to be on the safe side, I think we could a) move the OBJ_create() > > call to ssl_hook_pre_config and b) omit OBJ_cleanup(). Do you concur? > > For the record: I have now committed this to trunk with r1693792. Thank you! -- Jan Pazdziora Senior Principal Software Engineer, Identity Management Engineering, Red Hat
Re: Using UPN from subjectAltName with SSLUserName
On Sun, Aug 02, 2015 at 09:33:48AM +0200, Kaspar Brand wrote: > On 19.07.2015 17:24, Kaspar Brand wrote: > > But, to be on the safe side, I think we could a) move the OBJ_create() > > call to ssl_hook_pre_config and b) omit OBJ_cleanup(). Do you concur? > > For the record: I have now committed this to trunk with r1693792. Sorry - yeah, looks good to me like that. Thanks Kaspar! Regards, Joe
Re: Using UPN from subjectAltName with SSLUserName
On 19.07.2015 17:24, Kaspar Brand wrote: > But, to be on the safe side, I think we could a) move the OBJ_create() > call to ssl_hook_pre_config and b) omit OBJ_cleanup(). Do you concur? For the record: I have now committed this to trunk with r1693792. Kaspar
Re: Using UPN from subjectAltName with SSLUserName
On 13.07.2015 16:03, Joe Orton wrote: > On Sat, Jul 11, 2015 at 04:40:20PM +0200, Kaspar Brand wrote: >> @@ -1902,5 +1907,7 @@ apr_status_t ssl_init_ModuleKill(void *data) >> >> free_dh_params(); >> >> +OBJ_cleanup(); >> + >> return APR_SUCCESS; > > From being burnt previously three or four times, I get scared by OpenSSL > process global stuff. > > Have you worked out that it's safe to do that call there? It looks odd > to do that there rather than alongside other global cleanups in > ssl_cleanup_pre_config, so it's at least worth a comment if you really > want this here. Thanks for the heads up. After further digging, I believe it's not really comparable to the cases you're probably referring to (only found these two): 1) ERR_load_crypto_strings() / ERR_free_strings() Originally addressed in https://svn.apache.org/r101624, it was subsequently fixed in OpenSSL (0.9.8e and later, see [1]), and in late 2014 also adjusted for more recent OpenSSL versions (PR 53435 / https://bz.apache.org/bugzilla/show_bug.cgi?id=53435). 2) CRYPTO_new_ex_data() / CRYPTO_cleanup_all_ex_data() Addressed in https://svn.apache.org/r654119 (by dropping the CRYPTO_cleanup_all_ex_data call), see also PR 44975 / https://bz.apache.org/bugzilla/show_bug.cgi?id=44975). OBJ_create() / OBJ_cleanup() doesn't seem to suffer from these problems, but on the other hand, I'm also fine with dropping the OBJ_cleanup() call, since we only call OBJ_create() if the table entry doesn't exist yet (i.e. even repeated calls in ssl_init_Module / reloads do not cause any "leak"). > There is some complicated interaction between EVP_cleanup() and > OBJ_cleanup() which I haven't tried to decipher, but it looks like > EVP_cleanup() will actually do the cleanup call? This was added with 1.0.0, apparently [2]. The code would apply if OBJ_cleanup were called *before* EVP_cleanup (in all other cases, EVP_cleanup does not call OBJ_cleanup). But, to be on the safe side, I think we could a) move the OBJ_create() call to ssl_hook_pre_config and b) omit OBJ_cleanup(). Do you concur? Kaspar [1] https://git.openssl.org/?p=openssl.git;a=commitdiff;h=35e59297fca389b68bcad29876927666300ce971 [2] https://git.openssl.org/?p=openssl.git;a=commitdiff;h=246e09319c1d2a8140ffe1e5aeb1be26015696f0
Re: Using UPN from subjectAltName with SSLUserName
On Sat, Jul 11, 2015 at 04:40:20PM +0200, Kaspar Brand wrote: > @@ -1902,5 +1907,7 @@ apr_status_t ssl_init_ModuleKill(void *data) > > free_dh_params(); > > +OBJ_cleanup(); > + > return APR_SUCCESS; >From being burnt previously three or four times, I get scared by OpenSSL process global stuff. Have you worked out that it's safe to do that call there? It looks odd to do that there rather than alongside other global cleanups in ssl_cleanup_pre_config, so it's at least worth a comment if you really want this here. There is some complicated interaction between EVP_cleanup() and OBJ_cleanup() which I haven't tried to decipher, but it looks like EVP_cleanup() will actually do the cleanup call? Regards, Joe
Re: Using UPN from subjectAltName with SSLUserName
On Sat, Jul 11, 2015 at 04:40:20PM +0200, Kaspar Brand wrote: > On 29.06.2015 15:14, Jan Pazdziora wrote: > > How about just passing char * and doing all the mapping logic > > including possible OBJ_create in parse_otherName_value? My goal here > > is to have all the "hard" work of determining the semantics isolated > > in one place. > > > > Please see patch attached. > > You're right, an ASN1_OBJECT * as an argument for modssl_X509_getSAN > makes handling of otherName entries relatively awkward. In the attached > patch, I have switched to a string for specifying the requested > otherName form (similar to what you did in your patch). > > OBJ_create adds new entries to a process-wide table, so instead of > checking for the presence of a specific entry at each request (in > parse_otherName_value), I consider it more appropriate and efficient to > do this only once, in ssl_init_Module. > > Barring feedback against this approach (or the observation of bugs in > the implementation), I intend to commit it to trunk in the next few days > (including mod_ssl.xml changes and a CHANGES item). I've tried your patch and it works find for me. So I'm happy with your plan of committing it to trunk. ;-) Thank you! -- Jan Pazdziora Senior Principal Software Engineer, Identity Management Engineering, Red Hat
Re: Using UPN from subjectAltName with SSLUserName
On 29.06.2015 15:14, Jan Pazdziora wrote: > How about just passing char * and doing all the mapping logic > including possible OBJ_create in parse_otherName_value? My goal here > is to have all the "hard" work of determining the semantics isolated > in one place. > > Please see patch attached. You're right, an ASN1_OBJECT * as an argument for modssl_X509_getSAN makes handling of otherName entries relatively awkward. In the attached patch, I have switched to a string for specifying the requested otherName form (similar to what you did in your patch). OBJ_create adds new entries to a process-wide table, so instead of checking for the presence of a specific entry at each request (in parse_otherName_value), I consider it more appropriate and efficient to do this only once, in ssl_init_Module. Barring feedback against this approach (or the observation of bugs in the implementation), I intend to commit it to trunk in the next few days (including mod_ssl.xml changes and a CHANGES item). Kaspar Index: modules/ssl/ssl_engine_init.c === --- modules/ssl/ssl_engine_init.c (revision 1690371) +++ modules/ssl/ssl_engine_init.c (working copy) @@ -352,6 +352,11 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po init_dh_params(); +if (OBJ_txt2nid("id-on-dnsSRV") == NID_undef) { +(void)OBJ_create("1.3.6.1.5.5.7.8.7", "id-on-dnsSRV", + "SRVName otherName form"); +} + return OK; } @@ -1902,5 +1907,7 @@ apr_status_t ssl_init_ModuleKill(void *data) free_dh_params(); +OBJ_cleanup(); + return APR_SUCCESS; } Index: modules/ssl/ssl_util_ssl.c === --- modules/ssl/ssl_util_ssl.c (revision 1690371) +++ modules/ssl/ssl_util_ssl.c (working copy) @@ -252,19 +252,48 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5 return result; } +static void parse_otherName_value(apr_pool_t *p, ASN1_TYPE *value, + const char *onf, apr_array_header_t **entries) +{ +const char *str; +int nid = onf ? OBJ_txt2nid(onf) : NID_undef; + +if (!value || (nid == NID_undef) || !*entries) + return; + +/* + * Currently supported otherName forms (values for "onf"): + * "msUPN" (1.3.6.1.4.1.311.20.2.3): Microsoft User Principal Name + * "id-on-dnsSRV" (1.3.6.1.5.5.7.8.7): SRVName, as specified in RFC 4985 + */ +if ((nid == NID_ms_upn) && (value->type == V_ASN1_UTF8STRING) && +(str = asn1_string_to_utf8(p, value->value.utf8string))) { +APR_ARRAY_PUSH(*entries, const char *) = str; +} else if (strEQ(onf, "id-on-dnsSRV") && + (value->type == V_ASN1_IA5STRING) && + (str = asn1_string_to_utf8(p, value->value.ia5string))) { +APR_ARRAY_PUSH(*entries, const char *) = str; +} +} + /* * Return an array of subjectAltName entries of type "type". If idx is -1, * return all entries of the given type, otherwise return an array consisting * of the n-th occurrence of that type only. Currently supported types: * GEN_EMAIL (rfc822Name) * GEN_DNS (dNSName) + * GEN_OTHERNAME (requires the otherName form ["onf"] argument to be supplied, + *see parse_otherName_value for the currently supported forms) */ -BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, int idx, -apr_array_header_t **entries) +BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, const char *onf, +int idx, apr_array_header_t **entries) { STACK_OF(GENERAL_NAME) *names; +int nid = onf ? OBJ_txt2nid(onf) : NID_undef; -if (!x509 || (type < GEN_OTHERNAME) || (type > GEN_RID) || (idx < -1) || +if (!x509 || (type < GEN_OTHERNAME) || +((type == GEN_OTHERNAME) && (nid == NID_undef)) || +(type > GEN_RID) || (idx < -1) || !(*entries = apr_array_make(p, 0, sizeof(char * { *entries = NULL; return FALSE; @@ -277,33 +306,43 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5 for (i = 0; i < sk_GENERAL_NAME_num(names); i++) { name = sk_GENERAL_NAME_value(names, i); -if (name->type == type) { -if ((idx == -1) || (n == idx)) { -switch (type) { -case GEN_EMAIL: -case GEN_DNS: -utf8str = asn1_string_to_utf8(p, name->d.ia5); -if (utf8str) { -APR_ARRAY_PUSH(*entries, const char *) = utf8str; -} -break; -default: -/* - * Not implemented right now: - * GEN_OTHERNAME (otherName) - * GEN_X400 (x400Address) - * GEN_DIRNAME (directoryName) -
Re: Using UPN from subjectAltName with SSLUserName
On 06/29/2015 03:14 PM, Jan Pazdziora wrote: On Mon, Jun 29, 2015 at 01:47:45PM +0200, Jan Pazdziora wrote: On Sun, Jun 28, 2015 at 05:11:57PM +0200, Kaspar Brand wrote: On 22.06.2015 10:37, Jan Pazdziora wrote: Please find a new patch attached which I hope covers all the parts you've outlined, for SSL_CLIENT_SAN_OTHER_msUPN_*. Thanks. Your implementation assumes that only a single otherName form (msUPN) needs to be supported, but I would prefer to code it in a somewhat more extensible way. Does the attached patch work for you? As a practical way of Yes it does. My only question (and comments bellow) is about passing the oid rather than nid through the functions. I can see the string "id-on-dnsSRV" used twice in the patch in call OBJ_txt2nid and twice in call OBJ_txt2obj as well when ideally all which should be needed one OBJ_txt2nid("id-on-dnsSRV") and one OBJ_create if not found -- the rest could be done by comparing integers (nid). Unless I'm missing something about the oid/nid interaction. Ah, now I see it -- you look at the semantics of oid to compare value->type so nid would not be enough. How about just passing char * and doing all the mapping logic including possible OBJ_create in parse_otherName_value? My goal here is to have all the "hard" work of determining the semantics isolated in one place. Please see patch attached. Hi Kaspar, please could you find some time to review this patch? I can say that both proposed patches (your and Jan's) are equivalent when it comes to implementation functionality. Unfortunately, I don't have the OpenSSL knowledge to comment the differences on technical level, but I would also like to see this functionality in the trunk :). Regards, Jan Kaluza
Re: Using UPN from subjectAltName with SSLUserName
On Mon, Jun 29, 2015 at 01:47:45PM +0200, Jan Pazdziora wrote: > On Sun, Jun 28, 2015 at 05:11:57PM +0200, Kaspar Brand wrote: > > On 22.06.2015 10:37, Jan Pazdziora wrote: > > > Please find a new patch attached which I hope covers all the > > > parts you've outlined, for SSL_CLIENT_SAN_OTHER_msUPN_*. > > > > Thanks. Your implementation assumes that only a single otherName form > > (msUPN) needs to be supported, but I would prefer to code it in a > > somewhat more extensible way. > > > > Does the attached patch work for you? As a practical way of > > Yes it does. > > My only question (and comments bellow) is about passing the oid rather > than nid through the functions. I can see the string "id-on-dnsSRV" > used twice in the patch in call OBJ_txt2nid and twice in call OBJ_txt2obj > as well when ideally all which should be needed one > OBJ_txt2nid("id-on-dnsSRV") and one OBJ_create if not found -- the rest > could be done by comparing integers (nid). Unless I'm missing something > about the oid/nid interaction. Ah, now I see it -- you look at the semantics of oid to compare value->type so nid would not be enough. How about just passing char * and doing all the mapping logic including possible OBJ_create in parse_otherName_value? My goal here is to have all the "hard" work of determining the semantics isolated in one place. Please see patch attached. -- Jan Pazdziora Senior Principal Software Engineer, Identity Management Engineering, Red Hat Index: modules/ssl/ssl_engine_init.c === --- modules/ssl/ssl_engine_init.c (revision 1688186) +++ modules/ssl/ssl_engine_init.c (working copy) @@ -1902,5 +1902,7 @@ free_dh_params(); +OBJ_cleanup(); + return APR_SUCCESS; } Index: modules/ssl/ssl_engine_vars.c === --- modules/ssl/ssl_engine_vars.c (revision 1688186) +++ modules/ssl/ssl_engine_vars.c (working copy) @@ -665,6 +665,7 @@ { int type, numlen; apr_array_header_t *entries; +const char *id = NULL; if (strcEQn(var, "Email_", 6)) { type = GEN_EMAIL; @@ -674,6 +675,20 @@ type = GEN_DNS; var += 4; } +else if (strcEQn(var, "OTHER_", 6)) { +type = GEN_OTHERNAME; +var += 6; +if (strEQn(var, "msUPN_", 6)) { +var += 6; +id = "msUPN"; +} +else if (strEQn(var, "dnsSRV_", 7)) { +var += 7; +id = "id-on-dnsSRV"; +} +else + return NULL; +} else return NULL; @@ -682,7 +697,7 @@ if ((numlen < 1) || (numlen > 4) || (numlen != strlen(var))) return NULL; -if (modssl_X509_getSAN(p, xs, type, atoi(var), &entries)) +if (modssl_X509_getSAN(p, xs, type, id, atoi(var), &entries)) /* return the first entry from this 1-element array */ return APR_ARRAY_IDX(entries, 0, char *); else @@ -1032,12 +1047,15 @@ /* subjectAltName entries of the server certificate */ xs = SSL_get_certificate(ssl); if (xs) { -if (modssl_X509_getSAN(p, xs, GEN_EMAIL, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_EMAIL, NULL, -1, &entries)) { extract_san_array(t, "SSL_SERVER_SAN_Email", entries, p); } -if (modssl_X509_getSAN(p, xs, GEN_DNS, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_DNS, NULL, -1, &entries)) { extract_san_array(t, "SSL_SERVER_SAN_DNS", entries, p); } +if (modssl_X509_getSAN(p, xs, GEN_DNS, "id-on-dnsSRV", -1, &entries)) { +extract_san_array(t, "SSL_SERVER_SAN_OTHER_dnsSRV", entries, p); +} /* no need to free xs (refcount does not increase) */ } @@ -1044,12 +1062,15 @@ /* subjectAltName entries of the client certificate */ xs = SSL_get_peer_certificate(ssl); if (xs) { -if (modssl_X509_getSAN(p, xs, GEN_EMAIL, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_EMAIL, NULL, -1, &entries)) { extract_san_array(t, "SSL_CLIENT_SAN_Email", entries, p); } -if (modssl_X509_getSAN(p, xs, GEN_DNS, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_DNS, NULL, -1, &entries)) { extract_san_array(t, "SSL_CLIENT_SAN_DNS", entries, p); } +if (modssl_X509_getSAN(p, xs, GEN_OTHERNAME, "msUPN", -1, &entries)) { +extract_san_array(t, "SSL_CLIENT_SAN_OTHER_msUPN", entries, p); +} X509_free(xs); } } Index: modules/ssl/ssl_util_ssl.c === --- modules/ssl/ssl_util_ssl.c (revision 1688186) +++ modules/ssl/ssl_util_ssl.c (working copy) @@ -252,6 +252,46 @@ return result; } +static BOOL parse_otherName_value(apr_pool_t *p, OTHERNAME *othername, + const char *id, +
Re: Using UPN from subjectAltName with SSLUserName
On Sun, Jun 28, 2015 at 05:11:57PM +0200, Kaspar Brand wrote: > On 22.06.2015 10:37, Jan Pazdziora wrote: > > Please find a new patch attached which I hope covers all the > > parts you've outlined, for SSL_CLIENT_SAN_OTHER_msUPN_*. > > Thanks. Your implementation assumes that only a single otherName form > (msUPN) needs to be supported, but I would prefer to code it in a > somewhat more extensible way. > > Does the attached patch work for you? As a practical way of Yes it does. My only question (and comments bellow) is about passing the oid rather than nid through the functions. I can see the string "id-on-dnsSRV" used twice in the patch in call OBJ_txt2nid and twice in call OBJ_txt2obj as well when ideally all which should be needed one OBJ_txt2nid("id-on-dnsSRV") and one OBJ_create if not found -- the rest could be done by comparing integers (nid). Unless I'm missing something about the oid/nid interaction. > demonstrating generic support of otherName forms, I have > added the case of the SRVName otherName form (RFC 4985, for things like > _carddavs._tcp.example.com, exposed via SSL_SERVER_SAN_OTHER_dnsSRV_n). > > Kaspar > Index: modules/ssl/ssl_engine_init.c > === > --- modules/ssl/ssl_engine_init.c (revision 1687983) > +++ modules/ssl/ssl_engine_init.c (working copy) > @@ -352,6 +352,11 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po > > init_dh_params(); > > +if (OBJ_txt2nid("id-on-dnsSRV") == NID_undef) { > +(void)OBJ_create("1.3.6.1.5.5.7.8.7", "id-on-dnsSRV", > + "SRVName otherName form"); > +} Why do the OBJ_create call here and not in ssl_var_lookup_ssl_cert_san, once it it clear it will be needed at all? > Index: modules/ssl/ssl_util_ssl.c > === > --- modules/ssl/ssl_util_ssl.c(revision 1687983) > +++ modules/ssl/ssl_util_ssl.c(working copy) > @@ -252,19 +252,46 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5 > return result; > } > > +static void parse_otherName_value(apr_pool_t *p, ASN1_TYPE *value, > + ASN1_OBJECT *oid, > + apr_array_header_t **entries) > +{ > +const char *str; > + > +if (!value || !oid || !*entries) > + return; > + > +/* > + * Currently supported otherName forms: > + * - "msUPN" (1.3.6.1.4.1.311.20.2.3): Microsoft User Principal Name > + * - "dnsSRV" (1.3.6.1.5.5.7.8.7): SRVName, as specified in RFC 4985 > + */ > +if ((OBJ_obj2nid(oid) == NID_ms_upn) && Since the OBJ_obj2nid is made always before the comparison, could it be simpler to just convert to nid in ssl_var_lookup_ssl_cert_san and pass int, instead passingof ASN1_OBJECT * to modssl_X509_getSAN and parse_otherName_value? > +(value->type == V_ASN1_UTF8STRING) && > +(str = asn1_string_to_utf8(p, value->value.utf8string))) { > +APR_ARRAY_PUSH(*entries, const char *) = str; > +} else if ((OBJ_obj2nid(oid) == OBJ_txt2nid("id-on-dnsSRV")) && > + (value->type == V_ASN1_IA5STRING) && > + (str = asn1_string_to_utf8(p, value->value.ia5string))) { > +APR_ARRAY_PUSH(*entries, const char *) = str; > +} > +} > + > /* > * Return an array of subjectAltName entries of type "type". If idx is -1, > * return all entries of the given type, otherwise return an array consisting > * of the n-th occurrence of that type only. Currently supported types: > * GEN_EMAIL (rfc822Name) > * GEN_DNS (dNSName) > + * GEN_OTHERNAME (see parse_otherName_value for currently supported forms) > */ > -BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, int idx, > -apr_array_header_t **entries) > +BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, ASN1_OBJECT > *oid, > +int idx, apr_array_header_t **entries) > { > STACK_OF(GENERAL_NAME) *names; > > -if (!x509 || (type < GEN_OTHERNAME) || (type > GEN_RID) || (idx < -1) || > +if (!x509 || (type < GEN_OTHERNAME) || (type == GEN_OTHERNAME && !oid) || > +(type > GEN_RID) || (idx < -1) || > !(*entries = apr_array_make(p, 0, sizeof(char * { > *entries = NULL; > return FALSE; > @@ -277,33 +304,43 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5 > > for (i = 0; i < sk_GENERAL_NAME_num(names); i++) { > name = sk_GENERAL_NAME_value(names, i); > -if (name->type == type) { > -if ((idx == -1) || (n == idx)) { > -switch (type) { > -case GEN_EMAIL: > -case GEN_DNS: > -utf8str = asn1_string_to_utf8(p, name->d.ia5); > -if (utf8str) { > -APR_ARRAY_PUSH(*entries, const char *) = utf8str; > -
Re: Using UPN from subjectAltName with SSLUserName
On 22.06.2015 10:37, Jan Pazdziora wrote: > Please find a new patch attached which I hope covers all the > parts you've outlined, for SSL_CLIENT_SAN_OTHER_msUPN_*. Thanks. Your implementation assumes that only a single otherName form (msUPN) needs to be supported, but I would prefer to code it in a somewhat more extensible way. Does the attached patch work for you? As a practical way of demonstrating generic support of otherName forms, I have added the case of the SRVName otherName form (RFC 4985, for things like _carddavs._tcp.example.com, exposed via SSL_SERVER_SAN_OTHER_dnsSRV_n). Kaspar Index: modules/ssl/ssl_engine_init.c === --- modules/ssl/ssl_engine_init.c (revision 1687983) +++ modules/ssl/ssl_engine_init.c (working copy) @@ -352,6 +352,11 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_po init_dh_params(); +if (OBJ_txt2nid("id-on-dnsSRV") == NID_undef) { +(void)OBJ_create("1.3.6.1.5.5.7.8.7", "id-on-dnsSRV", + "SRVName otherName form"); +} + return OK; } @@ -1902,5 +1907,7 @@ apr_status_t ssl_init_ModuleKill(void *data) free_dh_params(); +OBJ_cleanup(); + return APR_SUCCESS; } Index: modules/ssl/ssl_util_ssl.c === --- modules/ssl/ssl_util_ssl.c (revision 1687983) +++ modules/ssl/ssl_util_ssl.c (working copy) @@ -252,19 +252,46 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5 return result; } +static void parse_otherName_value(apr_pool_t *p, ASN1_TYPE *value, + ASN1_OBJECT *oid, + apr_array_header_t **entries) +{ +const char *str; + +if (!value || !oid || !*entries) + return; + +/* + * Currently supported otherName forms: + * - "msUPN" (1.3.6.1.4.1.311.20.2.3): Microsoft User Principal Name + * - "dnsSRV" (1.3.6.1.5.5.7.8.7): SRVName, as specified in RFC 4985 + */ +if ((OBJ_obj2nid(oid) == NID_ms_upn) && +(value->type == V_ASN1_UTF8STRING) && +(str = asn1_string_to_utf8(p, value->value.utf8string))) { +APR_ARRAY_PUSH(*entries, const char *) = str; +} else if ((OBJ_obj2nid(oid) == OBJ_txt2nid("id-on-dnsSRV")) && + (value->type == V_ASN1_IA5STRING) && + (str = asn1_string_to_utf8(p, value->value.ia5string))) { +APR_ARRAY_PUSH(*entries, const char *) = str; +} +} + /* * Return an array of subjectAltName entries of type "type". If idx is -1, * return all entries of the given type, otherwise return an array consisting * of the n-th occurrence of that type only. Currently supported types: * GEN_EMAIL (rfc822Name) * GEN_DNS (dNSName) + * GEN_OTHERNAME (see parse_otherName_value for currently supported forms) */ -BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, int idx, -apr_array_header_t **entries) +BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, ASN1_OBJECT *oid, +int idx, apr_array_header_t **entries) { STACK_OF(GENERAL_NAME) *names; -if (!x509 || (type < GEN_OTHERNAME) || (type > GEN_RID) || (idx < -1) || +if (!x509 || (type < GEN_OTHERNAME) || (type == GEN_OTHERNAME && !oid) || +(type > GEN_RID) || (idx < -1) || !(*entries = apr_array_make(p, 0, sizeof(char * { *entries = NULL; return FALSE; @@ -277,33 +304,43 @@ char *modssl_X509_NAME_to_string(apr_pool_t *p, X5 for (i = 0; i < sk_GENERAL_NAME_num(names); i++) { name = sk_GENERAL_NAME_value(names, i); -if (name->type == type) { -if ((idx == -1) || (n == idx)) { -switch (type) { -case GEN_EMAIL: -case GEN_DNS: -utf8str = asn1_string_to_utf8(p, name->d.ia5); -if (utf8str) { -APR_ARRAY_PUSH(*entries, const char *) = utf8str; -} -break; -default: -/* - * Not implemented right now: - * GEN_OTHERNAME (otherName) - * GEN_X400 (x400Address) - * GEN_DIRNAME (directoryName) - * GEN_EDIPARTY (ediPartyName) - * GEN_URI (uniformResourceIdentifier) - * GEN_IPADD (iPAddress) - * GEN_RID (registeredID) - */ -break; + +if (name->type != type) +continue; + +switch (type) { +case GEN_EMAIL: +case GEN_DNS: +if (((idx == -1) || (n == idx)) && +(utf8str = asn1_string_to_utf8(p, name->d.ia5))) { +APR_A
Re: Using UPN from subjectAltName with SSLUserName
On Sun, Jun 21, 2015 at 08:55:02AM +0200, Kaspar Brand wrote: > > The point with the otherName SAN type is that it's yet another bag of > potentially arbitrary ASN.1 stuff, actually (not just simple strings, as > is the case for rfc822Name or dNSName): > >GeneralName ::= CHOICE { > otherName [0] OtherName, > rfc822Name [1] IA5String, > dNSName [2] IA5String, > x400Address [3] ORAddress, > directoryName [4] Name, > ediPartyName[5] EDIPartyName, > uniformResourceIdentifier [6] IA5String, > iPAddress [7] OCTET STRING, > registeredID[8] OBJECT IDENTIFIER } > >OtherName ::= SEQUENCE { > type-idOBJECT IDENTIFIER, > value [0] EXPLICIT ANY DEFINED BY type-id } > ^^ > > (See RFC 7299 section 3.14 for the otherName forms defined by the PKIX > WG in the past.) > > While Microsoft's UPN happens to be a very simple case (the value is > just a bare UTF8String here), this need not be true for other forms > of otherName. > > Adding support for msUPN seems useful, but it's really something which > needs special-case coding in ssl_util_ssl.c:modssl_X509_getSAN(). I > suggest using SSL_{CLIENT,SERVER}_SAN_OTHER_msUPN_* for the variable > name(s), to make it clear that it's a subjectAltName entry of type > otherName. Then, in the code for GEN_OTHERNAME, specifically look for > this otherName form via "NID_ms_upn" - as only in this case, you > can be sure to expect a simple UTF8String in otherName->value > (strongSwan's openssl_x509.c might be a source of inspiration for coding > this). > > Note that for exposing the msUPN variables with StdEnvVars, you also > need to adapt ssl_engine_vars.c:modssl_var_extract_san_entries(). Thank you for the analysis. Please find a new patch attached which I hope covers all the parts you've outlined, for SSL_CLIENT_SAN_OTHER_msUPN_*. Yours, -- Jan Pazdziora Senior Principal Software Engineer, Identity Management Engineering, Red Hat Index: docs/manual/mod/mod_ssl.xml === --- docs/manual/mod/mod_ssl.xml (revision 1686805) +++ docs/manual/mod/mod_ssl.xml (working copy) @@ -77,6 +77,7 @@ SSL_CLIENT_S_DN_x509 string Component of client's Subject DN SSL_CLIENT_SAN_Email_n string Client certificate's subjectAltName extension entries of type rfc822Name SSL_CLIENT_SAN_DNS_n string Client certificate's subjectAltName extension entries of type dNSName +SSL_CLIENT_SAN_OTHER_msUPN_n stringClient certificate's subjectAltName extension entries of type otherName with Microsoft Universal Principal Name (OID 1.3.6.1.4.1.311.20.2.3) SSL_CLIENT_I_DN string Issuer DN of client's certificate SSL_CLIENT_I_DN_x509 string Component of client's Issuer DN SSL_CLIENT_V_STARTstring Validity of client's certificate (start time) Index: modules/ssl/ssl_engine_vars.c === --- modules/ssl/ssl_engine_vars.c (revision 1686805) +++ modules/ssl/ssl_engine_vars.c (working copy) @@ -674,6 +674,10 @@ type = GEN_DNS; var += 4; } +else if (strcEQn(var, "OTHER_msUPN_", 12)) { +type = GEN_OTHERNAME; +var += 12; +} else return NULL; @@ -1050,6 +1054,9 @@ if (modssl_X509_getSAN(p, xs, GEN_DNS, -1, &entries)) { extract_san_array(t, "SSL_CLIENT_SAN_DNS", entries, p); } +if (modssl_X509_getSAN(p, xs, GEN_OTHERNAME, -1, &entries)) { +extract_san_array(t, "SSL_CLIENT_SAN_OTHER_msUPN", entries, p); +} X509_free(xs); } } Index: modules/ssl/ssl_util_ssl.c === --- modules/ssl/ssl_util_ssl.c (revision 1686805) +++ modules/ssl/ssl_util_ssl.c (working copy) @@ -258,6 +258,7 @@ * of the n-th occurrence of that type only. Currently supported types: * GEN_EMAIL (rfc822Name) * GEN_DNS (dNSName) + * GEN_OTHERNAME (otherName for UPN) */ BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, int idx, apr_array_header_t **entries) @@ -287,10 +288,18 @@ APR_ARRAY_PUSH(*entries, const char *) = utf8str; } break; +case GEN_OTHERNAME: +if (name->d.otherName && name->d.otherName->value +&& OBJ_obj2nid(name->d.otherName->type_id) == NID_ms_upn) { +utf8str = asn1_string_to_utf8(p, name->d.otherName->value->value.asn1_string); +if (utf8str) { +
Re: Using UPN from subjectAltName with SSLUserName
On 19.06.2015 16:51, Jan Pazdziora wrote: > On Thu, Jun 18, 2015 at 12:22:21PM +0200, Yann Ylavic wrote: >> I think a more generic way would to have something like >> SSL_CLIENT_OID__n, so that we wouldn't have to add a new field >> each time. >> In this case, that would be: SSL_CLIENT_OID_1.3.6.1.4.1.311.20.2.3_n. The point with the otherName SAN type is that it's yet another bag of potentially arbitrary ASN.1 stuff, actually (not just simple strings, as is the case for rfc822Name or dNSName): GeneralName ::= CHOICE { otherName [0] OtherName, rfc822Name [1] IA5String, dNSName [2] IA5String, x400Address [3] ORAddress, directoryName [4] Name, ediPartyName[5] EDIPartyName, uniformResourceIdentifier [6] IA5String, iPAddress [7] OCTET STRING, registeredID[8] OBJECT IDENTIFIER } OtherName ::= SEQUENCE { type-idOBJECT IDENTIFIER, value [0] EXPLICIT ANY DEFINED BY type-id } ^^ (See RFC 7299 section 3.14 for the otherName forms defined by the PKIX WG in the past.) While Microsoft's UPN happens to be a very simple case (the value is just a bare UTF8String here), this need not be true for other forms of otherName. Adding support for msUPN seems useful, but it's really something which needs special-case coding in ssl_util_ssl.c:modssl_X509_getSAN(). I suggest using SSL_{CLIENT,SERVER}_SAN_OTHER_msUPN_* for the variable name(s), to make it clear that it's a subjectAltName entry of type otherName. Then, in the code for GEN_OTHERNAME, specifically look for this otherName form via "NID_ms_upn" - as only in this case, you can be sure to expect a simple UTF8String in otherName->value (strongSwan's openssl_x509.c might be a source of inspiration for coding this). Note that for exposing the msUPN variables with StdEnvVars, you also need to adapt ssl_engine_vars.c:modssl_var_extract_san_entries(). Kaspar
Re: Using UPN from subjectAltName with SSLUserName
On Thu, Jun 18, 2015 at 12:22:21PM +0200, Yann Ylavic wrote: > On Thu, Jun 18, 2015 at 11:49 AM, Jan Pazdziora wrote: > > > > I'd appreciate any comments about suitability of such change, as well > > as the implementation. Specifically, I'm not sure if people will > > prefer the generic and currently proposed > > > > SSL_CLIENT_SAN_otherName_n > > > > which gets any value of otherName type, or perhaps going with > > > > SSL_CLIENT_SAN_UPN_n > > > > and checking the OID just for the UPNs. Based on that decision I plan > > to then respin the patch with documentation changes included. > > I think a more generic way would to have something like > SSL_CLIENT_OID__n, so that we wouldn't have to add a new field > each time. > In this case, that would be: SSL_CLIENT_OID_1.3.6.1.4.1.311.20.2.3_n. Please find attached a patch which makes it possible to use SSL_CLIENT_SAN_otherName_1.3.6.1.4.1.311.20.2.3_0 or SSL_CLIENT_SAN_otherName_0 In the first case we use OBJ_create to create the object and then compare its nid to OBJ_obj2nid result. In the second form we ignore the OID. I went with the SAN_otherName rather than OID to make it clear where we get the value from. Of course, of you think that SSL_CLIENT_OID_1.3.6.1.4.1.311.20.2.3_n better and the correct place to look for it is always in name->d.otherName, it should be easy to change the name. -- Jan Pazdziora Senior Principal Software Engineer, Identity Management Engineering, Red Hat Index: modules/ssl/ssl_engine_vars.c === --- modules/ssl/ssl_engine_vars.c (revision 1686420) +++ modules/ssl/ssl_engine_vars.c (working copy) @@ -663,8 +663,9 @@ static char *ssl_var_lookup_ssl_cert_san(apr_pool_t *p, X509 *xs, char *var) { -int type, numlen; +int type, numlen, nid = NID_undef; apr_array_header_t *entries; +char *ptr; if (strcEQn(var, "Email_", 6)) { type = GEN_EMAIL; @@ -674,6 +675,17 @@ type = GEN_DNS; var += 4; } +else if (strcEQn(var, "otherName_", 10)) { +type = GEN_OTHERNAME; +var += 10; +ptr = strchr(var, '_'); +if (ptr) { +char *oid_str; +oid_str = apr_pstrndup(p, var, (ptr - var)); +nid = OBJ_create(oid_str, NULL, NULL); +var = ptr + 1; +} +} else return NULL; @@ -682,7 +694,7 @@ if ((numlen < 1) || (numlen > 4) || (numlen != strlen(var))) return NULL; -if (modssl_X509_getSAN(p, xs, type, atoi(var), &entries)) +if (modssl_X509_getSAN(p, xs, type, nid, atoi(var), &entries)) /* return the first entry from this 1-element array */ return APR_ARRAY_IDX(entries, 0, char *); else @@ -1032,10 +1044,10 @@ /* subjectAltName entries of the server certificate */ xs = SSL_get_certificate(ssl); if (xs) { -if (modssl_X509_getSAN(p, xs, GEN_EMAIL, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_EMAIL, NID_undef, -1, &entries)) { extract_san_array(t, "SSL_SERVER_SAN_Email", entries, p); } -if (modssl_X509_getSAN(p, xs, GEN_DNS, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_DNS, NID_undef, -1, &entries)) { extract_san_array(t, "SSL_SERVER_SAN_DNS", entries, p); } /* no need to free xs (refcount does not increase) */ @@ -1044,10 +1056,10 @@ /* subjectAltName entries of the client certificate */ xs = SSL_get_peer_certificate(ssl); if (xs) { -if (modssl_X509_getSAN(p, xs, GEN_EMAIL, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_EMAIL, NID_undef, -1, &entries)) { extract_san_array(t, "SSL_CLIENT_SAN_Email", entries, p); } -if (modssl_X509_getSAN(p, xs, GEN_DNS, -1, &entries)) { +if (modssl_X509_getSAN(p, xs, GEN_DNS, NID_undef, -1, &entries)) { extract_san_array(t, "SSL_CLIENT_SAN_DNS", entries, p); } X509_free(xs); Index: modules/ssl/ssl_util_ssl.c === --- modules/ssl/ssl_util_ssl.c (revision 1686420) +++ modules/ssl/ssl_util_ssl.c (working copy) @@ -258,8 +258,9 @@ * of the n-th occurrence of that type only. Currently supported types: * GEN_EMAIL (rfc822Name) * GEN_DNS (dNSName) + * GEN_OTHERNAME (otherName) */ -BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, int idx, +BOOL modssl_X509_getSAN(apr_pool_t *p, X509 *x509, int type, int nid, int idx, apr_array_header_t **entries) { STACK_OF(GENERAL_NAME) *names; @@ -287,10 +288,19 @@ APR_ARRAY_PUSH(*entries, const char *) = utf8str; } break; +case GEN_OTHERNAME: +if (name->d.otherName && name->d.otherName->value +&& (nid == NI
Re: Using UPN from subjectAltName with SSLUserName
On Fri, Jun 19, 2015 at 11:56 AM, Yann Ylavic wrote: > > Instead of SSL_CLIENT_OID_, we could also have something like > SSL_CLIENTn since the underlying mod_ssl > code handles both (IIRC). > I don't know if SAN_otherName/UPN have a short/long name though, but many > have. Nope, SAN as an oid/long/short name, but no fancy name for its inner fields. But since openssl.cnf (man x509v3_config) accepts things like "subjectAltName=email:my@other.address,RID:1.2.3.4,otherName:my.other.name", maybe we can do something...
Re: Using UPN from subjectAltName with SSLUserName
On Fri, Jun 19, 2015 at 11:32 AM, Jan Kaluža wrote: > On 06/18/2015 12:22 PM, Yann Ylavic wrote: >> >> On Thu, Jun 18, 2015 at 11:49 AM, Jan Pazdziora >> wrote: >>> >>> >>> I'd appreciate any comments about suitability of such change, as well >>> as the implementation. Specifically, I'm not sure if people will >>> prefer the generic and currently proposed >>> >>> SSL_CLIENT_SAN_otherName_n >>> >>> which gets any value of otherName type, or perhaps going with >>> >>> SSL_CLIENT_SAN_UPN_n >>> >>> and checking the OID just for the UPNs. Based on that decision I plan >>> to then respin the patch with documentation changes included. >> >> >> I think a more generic way would to have something like >> SSL_CLIENT_OID__n, so that we wouldn't have to add a new field >> each time. >> In this case, that would be: SSL_CLIENT_OID_1.3.6.1.4.1.311.20.2.3_n. > > > I think that's nice idea. I can probably work on that. The only question is > if we would like to have this generic way as additional feature, or we > really want to use it instead of the SSL_CLIENT_SAN_otherName_n as proposed > by Jan. > > I think that the common cases should have non-generic variable. The question > is if otherName is the common case. Depends on whether one needs it or not I guess :) > > Ideas? Instead of SSL_CLIENT_OID_, we could also have something like SSL_CLIENTn since the underlying mod_ssl code handles both (IIRC). I don't know if SAN_otherName/UPN have a short/long name though, but many have.
Re: Using UPN from subjectAltName with SSLUserName
On 06/18/2015 12:22 PM, Yann Ylavic wrote: On Thu, Jun 18, 2015 at 11:49 AM, Jan Pazdziora wrote: I'd appreciate any comments about suitability of such change, as well as the implementation. Specifically, I'm not sure if people will prefer the generic and currently proposed SSL_CLIENT_SAN_otherName_n which gets any value of otherName type, or perhaps going with SSL_CLIENT_SAN_UPN_n and checking the OID just for the UPNs. Based on that decision I plan to then respin the patch with documentation changes included. I think a more generic way would to have something like SSL_CLIENT_OID__n, so that we wouldn't have to add a new field each time. In this case, that would be: SSL_CLIENT_OID_1.3.6.1.4.1.311.20.2.3_n. I think that's nice idea. I can probably work on that. The only question is if we would like to have this generic way as additional feature, or we really want to use it instead of the SSL_CLIENT_SAN_otherName_n as proposed by Jan. I think that the common cases should have non-generic variable. The question is if otherName is the common case. Ideas? Regards, Yann. Regards, Jan Kaluza
Re: Using UPN from subjectAltName with SSLUserName
On Thu, Jun 18, 2015 at 11:49 AM, Jan Pazdziora wrote: > > I'd appreciate any comments about suitability of such change, as well > as the implementation. Specifically, I'm not sure if people will > prefer the generic and currently proposed > > SSL_CLIENT_SAN_otherName_n > > which gets any value of otherName type, or perhaps going with > > SSL_CLIENT_SAN_UPN_n > > and checking the OID just for the UPNs. Based on that decision I plan > to then respin the patch with documentation changes included. I think a more generic way would to have something like SSL_CLIENT_OID__n, so that we wouldn't have to add a new field each time. In this case, that would be: SSL_CLIENT_OID_1.3.6.1.4.1.311.20.2.3_n. Regards, Yann.