Re: [6/6] secur32: Add support for SECPKG_ATTR_KEY_INFO (GnuTLS only).

[Jacek Caban]

Hi Akihiro,

Thanks for working on that. I have a few comments to the patch.

On 17.04.2017 16:26, Akihiro Sagawa wrote:
> diff --git a/dlls/secur32/schannel.c b/dlls/secur32/schannel.c
> index 6914837..317686f 100644
> --- a/dlls/secur32/schannel.c
> +++ b/dlls/secur32/schannel.c
> @@ -54,11 +54,14 @@ struct schan_handle
>       enum schan_handle_type type;
>   };
>
> +#define SCHAN_KEY_NAME_MAX 8
>   struct schan_context
>   {
>       schan_imp_session session;
>       ULONG req_ctx_attr;
>       const CERT_CONTEXT *cert;
> +    SEC_WCHAR key_signature_name[SCHAN_KEY_NAME_MAX];
> +    SEC_WCHAR key_encrypt_name[SCHAN_KEY_NAME_MAX];

Those should not be needed. You could for example store both WCHAR and 
char strings inside alg_name_map.
> };
>
> static struct schan_handle *schan_handle_table;
> @@ -962,6 +965,53 @@ static SECURITY_STATUS SEC_ENTRY schan_InitializeSecurityContextA(
>       return ret;
>   }
>
> +struct schan_alg_id_name {
> +    ALG_ID alg_id;
> +    const char* name;
> +};
> +
> +static int comp_alg_id(const void *a, const void *b)
> +{
> +    const struct schan_alg_id_name *lhs = a;
> +    const struct schan_alg_id_name *rhs = b;
> +    return (int)lhs->alg_id - (int)rhs->alg_id;
> +}
> +
> +static void fill_alg_name(ALG_ID id, void *buffer, BOOL wide)

If you change the patch as I suggested above, this could be something like:

static void *get_alg_name(ALF_ID id, BOOL wide)
> +{
> +    static const struct schan_alg_id_name alg_name_map[] = {
> +        { CALG_ECDSA,      "ECDSA" },
> +        { CALG_RSA_SIGN,   "RSA" },
> +        { CALG_DES,        "DES" },
> +        { CALG_RC2,        "RC2" },
> +        { CALG_3DES,       "3DES" },
> +        { CALG_AES_128,    "AES" },
> +        { CALG_AES_192,    "AES" },
> +        { CALG_AES_256,    "AES" },
> +        { CALG_RC4,        "RC4" },
> +    };
> +    const struct schan_alg_id_name *res;
> +    struct schan_alg_id_name key;
> +    const char *name;
> +
> +    key.alg_id = id;
> +    res = bsearch(&key, alg_name_map,
> +                  sizeof(alg_name_map)/sizeof(alg_name_map[0]), sizeof(alg_name_map[0]),
> +                  comp_alg_id);
> +    if (res)
> +        name = res->name;
> +    else
> +    {
> +        TRACE("Unknown ALG_ID %04x\n", id);
> +        name = "???";
> +    }

A FIXME would would be nice here. Also, I think you could just return 
NULL in this case.
> +
> +    if (wide)
> +        MultiByteToWideChar(CP_ACP, 0, name, -1, (SEC_WCHAR*)buffer, SCHAN_KEY_NAME_MAX);
> +    else
> +        strcpy((SEC_CHAR*)buffer, name);
> +}
> +
>   static SECURITY_STATUS ensure_remote_cert(struct schan_context *ctx)
>   {
>       HCERTSTORE cert_store;
> @@ -989,6 +1039,7 @@ static SECURITY_STATUS SEC_ENTRY schan_QueryContextAttributesW(
>
> if (!context_handle) return SEC_E_INVALID_HANDLE;
>       ctx = schan_get_object(context_handle->dwLower, SCHAN_HANDLE_CTX);
> +    if (!ctx) return SEC_E_INVALID_HANDLE;

This has an effect outside of scope of this patch. This should be be a 
separated patch, with a test case.
> switch(attribute)
>       {
> @@ -1016,6 +1067,19 @@ static SECURITY_STATUS SEC_ENTRY schan_QueryContextAttributesW(
>
> return status;
>           }
> +        case SECPKG_ATTR_KEY_INFO:
> +        {
> +            SecPkgContext_KeyInfoW *info = buffer;
> +            SECURITY_STATUS status = schan_imp_get_key_info(ctx->session, info);
> +            if (status == SEC_E_OK)
> +            {
> +                info->sSignatureAlgorithmName = ctx->key_signature_name;
> +                info->sEncryptAlgorithmName = ctx->key_encrypt_name;
> +                fill_alg_name(info->SignatureAlgorithm, info->sSignatureAlgorithmName, TRUE);
> +                fill_alg_name(info->EncryptAlgorithm, info->sEncryptAlgorithmName, TRUE);
> +            }
> +            return status;
> +        }
>           case SECPKG_ATTR_REMOTE_CERT_CONTEXT:
>           {
>               PCCERT_CONTEXT *cert = buffer;
> @@ -1091,6 +1155,24 @@ static SECURITY_STATUS SEC_ENTRY schan_QueryContextAttributesA(
>       {
>           case SECPKG_ATTR_STREAM_SIZES:
>               return schan_QueryContextAttributesW(context_handle, attribute, buffer);
> +        case SECPKG_ATTR_KEY_INFO:
> +        {
> +            SecPkgContext_KeyInfoA *info = buffer;
> +            struct schan_context *ctx;
> +            SECURITY_STATUS status;
> +            if (!context_handle) return SEC_E_INVALID_HANDLE;
> +            ctx = schan_get_object(context_handle->dwLower, SCHAN_HANDLE_CTX);
> +            if (!ctx) return SEC_E_INVALID_HANDLE;
> +            status = schan_imp_get_key_info(ctx->session, (SecPkgContext_KeyInfoW*)info);
> +            if (status == SEC_E_OK)
> +            {
> +                info->sSignatureAlgorithmName = (SEC_CHAR *)ctx->key_signature_name;
> +                info->sEncryptAlgorithmName = (SEC_CHAR *)ctx->key_encrypt_name;
> +                fill_alg_name(info->SignatureAlgorithm, info->sSignatureAlgorithmName, FALSE);
> +                fill_alg_name(info->EncryptAlgorithm, info->sEncryptAlgorithmName, FALSE);
> +            }
> +            return status;
> +        }
>           case SECPKG_ATTR_REMOTE_CERT_CONTEXT:
>               return schan_QueryContextAttributesW(context_handle, attribute, buffer);
>           case SECPKG_ATTR_CONNECTION_INFO:
> diff --git a/dlls/secur32/schannel_gnutls.c b/dlls/secur32/schannel_gnutls.c
> index 30640d3..ddf33a8 100644
> --- a/dlls/secur32/schannel_gnutls.c
> +++ b/dlls/secur32/schannel_gnutls.c
> @@ -363,6 +363,22 @@ static ALG_ID schannel_get_kx_algid(int kx)
>       }
>   }
>
> +static ALG_ID schannel_get_kx_signature_algid(gnutls_kx_algorithm_t kx)
> +{
> +    switch (kx)
> +    {
> +    case GNUTLS_KX_UNKNOWN: return 0;
> +    case GNUTLS_KX_RSA:
> +    case GNUTLS_KX_RSA_EXPORT:
> +    case GNUTLS_KX_DHE_RSA:
> +    case GNUTLS_KX_ECDHE_RSA: return CALG_RSA_SIGN;
> +    case GNUTLS_KX_ECDHE_ECDSA: return CALG_ECDSA;
> +    default:
> +        FIXME("unknown algorithm %d\n", kx);
> +        return 0;
> +    }
> +}
> +
>   unsigned int schan_imp_get_session_cipher_block_size(schan_imp_session session)
>   {
>       gnutls_session_t s = (gnutls_session_t)session;
> @@ -394,6 +410,23 @@ SECURITY_STATUS schan_imp_get_connection_info(schan_imp_session session,
>       return SEC_E_OK;
>   }
>
> +SECURITY_STATUS schan_imp_get_key_info(schan_imp_session session, SecPkgContext_KeyInfoW *info)
> +{
> +    gnutls_session_t s = (gnutls_session_t)session;
> +    gnutls_cipher_algorithm_t alg = pgnutls_cipher_get(s);
> +    gnutls_kx_algorithm_t kx = pgnutls_kx_get(s);
> +
> +    TRACE("(%p,%p)\n", session, info);
> +
> +    info->sSignatureAlgorithmName = NULL; /* filled later */
> +    info->sEncryptAlgorithmName = NULL; /* filled later */
> +    info->KeySize = pgnutls_cipher_get_key_size(alg) * 8;
> +    info->SignatureAlgorithm = schannel_get_kx_signature_algid(kx);
> +    info->EncryptAlgorithm = schannel_get_cipher_algid(alg);
> +
> +    return SEC_E_OK;
> +}

I don't think such helper that fills output partially is the right 
solution here. You could use schan_imp_get_connection_info to get all 
info you need except signature algorithm. For signature algorithm, you 
may introduce a new, simpler, function.

Thanks,
Jacek