Hello Selva,
I'll send an updated patch wrt. some of your notes, for now just a quick reply
to some of them:
> I'm not convinced of the utility of this. It could be marginally useful in
> some
> setups where all users are tied to a domain and choosing any certificate that
> matches a template is appropriate. I don't have a setup to test this.
That's not the only use case. I'm currently deploying this patch to implement a
machine always-on VPN (machines automatically register a specific certificate
type which contains the machine name as a SAN, which can then be selected using
the TMPL: logic and on the server side the client certificate looked up in a
tls-verify script to check whether the machine account is active). This patch
allows moving the client certificate selection out of the configuration file,
as the selector based on template ID is always equal on any client (SUBJ of
course isn't) and as such can be embedded as a plain .ovpn file in the MSI
which deploys the software, without any additional configuration required on
the part of the client, and especially resilient to later renaming of the
client if that should happen (new certificate deployed with certutil /pulse,
new VPN connection found). Same of course goes for your scenario of user
auto-registered certificates.
> > +static const CRYPT_OID_INFO *
> > +find_oid(DWORD keytype, const void *key, DWORD groupid, bool
> > +fallback) {
> > + const CRYPT_OID_INFO *info = NULL;
> > +
> > + /* force resolve from local as first step */
> > + if (groupid != CRYPT_HASH_ALG_OID_GROUP_ID &&
> > + groupid != CRYPT_ENCRYPT_ALG_OID_GROUP_ID &&
> > + groupid != CRYPT_PUBKEY_ALG_OID_GROUP_ID &&
> > + groupid != CRYPT_SIGN_ALG_OID_GROUP_ID &&
> > + groupid != CRYPT_RDN_ATTR_OID_GROUP_ID &&
> > + groupid != CRYPT_EXT_OR_ATTR_OID_GROUP_ID &&
> > + groupid != CRYPT_KDF_OID_GROUP_ID)
> > + {
>
> Why not do the local lookup for all grupids? Anyway you are falling back to
> domain-wide look up if this fails.
Simply because Microsoft does it this way in their generic wrapping of
CryptFindOIDInfo for certificate selection helpers:
https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/Common/src/Interop/Windows/Crypt32/Interop.FindOidInfo.cs#L51
ff - the groups excluded here will always be looked up locally, independent of
the CRYPT_OID_DISABLE_SEARCH_FLAG, so that we can save one call for them.
> > + info = CryptFindOIDInfo(keytype, (void*)key, groupid |
> CRYPT_OID_DISABLE_SEARCH_DS_FLAG);
> > + }
> > +
> > + /* try proper resolve if not found yet, also including AD */
> > + if (!info)
> > + {
> > + info = CryptFindOIDInfo(keytype, (void*)key, groupid);
>
> How long would this take to time out -- we try to avoid calling functions that
> need network access to the DC as that's often not available before the tunnel
> comes up.
Good question which I hadn't taken into account so far; from what I've seen on
machine clients I've deployed that select their certificate using this logic,
the timeout is almost instantaneous, i.e. CryptFindOIDInfo does not find an
object for the TMPL: parameter if it can't reach the domain. Of course, your
note generally limits the functionality of actually using CryptFindOIDInfo to
resolve a printable template name from a specific domain to an OID (which is
then found in the actual certificate), but I wouldn't take this out if it
doesn't introduce a long delay (possibly, there are two tunnels in some cases,
where an independent tunnel sets up reachability of the domain).
> > + }
> > +
> > + /* fall back to all groups if not found yet and fallback requested */
> > + if (!info && fallback && groupid)
> > + {
> > + info = CryptFindOIDInfo(keytype, (void*)key, 0);
>
> That could be a second network call. Why not combine the two into one call.
The difference being that the first call only resolves the name to OID for the
specific object type (e.g., certificate template) being passed, whereas this
call resolves it for any object type. Of course, you always prefer the specific
match if that's available, and only fall back to the generic match if the
caller actually wants to try that. I've implemented this method more or less
equal to the implementation in the .NET framework, simply to facilitate using
it for additional certificate matches which might want to be implemented; there
are other use cases for CryptFindOIDInfo in other match helpers where this
logic actually makes sense. It's debatable whether the fallback to a generic
match actually makes sense in the case of certificate template OID matching
(see the use below), but at least the .NET framework implementation does this:
https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/FindPal.cs#L190
> > + /* check for V1 extension (Windows 2K and below) */
> > + ext = CertFindExtension(szOID_ENROLL_CERTTYPE_EXTENSION, info-
> >cExtension, info->rgExtension);
> > + if (ext)
> > + {
> > + pvext = decode_object(gc, X509_UNICODE_ANY_STRING, &ext->Value,
> 0, &cbext);
> > + if (pvext && cbext >= sizeof(CERT_NAME_VALUE))
> > + {
> > + const CERT_NAME_VALUE *nv = (const CERT_NAME_VALUE*)pvext;
> > + if (!_wcsicmp((const WCHAR*)nv->Value.pbData, tmpl_name))
>
> Is it guaranteed that pbData is NULL terminated? (MS docs are unclear on
> this).
> We may have to add a safety check that the data length length is >=
> wcslen(tmpl_name).
Yes, that's guaranteed, at least according to the corresponding use in the .NET
framework:
https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/FindPal.cs#L165
- of course it doesn't hurt to have an additional check anyway.
> > + const CRYPT_OID_INFO *tmpl_oid =
> find_oid(CRYPT_OID_INFO_NAME_KEY, tmpl_name,
> CRYPT_TEMPLATE_OID_GROUP_ID, true);
> > + if (tmpl_oid && !stricmp(tmpl_oid->pszOID, cte->pszObjId))
> > + {
> > + /* found OID match in extension against resolved key */
> > + return true;
> > + }
> > + else if (!tmpl_oid && !stricmp(cert_prop, cte->pszObjId))
> > + {
>
> Why not test this first so that the call to find_oid could be avoided if this
> matches?
For one, because the match in
https://github.com/dotnet/runtime/blob/01b7e73cd378145264a7cb7a09365b41ed42b240/src/libraries/System.Security.Cryptography.X509Certificates/src/Internal/Cryptography/Pal.Windows/FindPal.cs#L187
ff also does it just this way, and secondly, because this way the preference
is on matching a template name (i.e., if a string passed can be interpreted as
a template name in the current domain, it will be, even if it looks like an
OID), whereas only resolving when the OID doesn't match directly would prefer
the interpretation as an OID. It's debatable whether the former or the latter
is preferential, I'd also go to the former, just like the .NET framework does,
especially considering that the "old" X509 extension containing the certificate
template reference actually contains the name, not an OID (see the first
condition), so that possibly some people actually expect the parameter to be a
name.
For all the other notes, those are clear; I'll create a new patch with the
changes applied. Thanks for your feedback!
--- Heiko.
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
