| Message ID | 1424565478-6551-1-git-send-email-gerow@google.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sat, Feb 21, 2015 at 04:37:58PM -0800, Mike Gerow wrote: > This can get returned by modules that simply aren't ready yet, so just throwing > away the pkcs11 pin doesn't really make sense. > > This also addresses a problem where an error condition isn't communicated to the > caller appropriately in this specific case. Please read the top level CONTRIBUTIONS file and provide a Signed-off-by: line at the end of the commit message. I won't be able to apply changes to hostap.git without that. As far as the change itself is concerned, I could agree with this if the error was indeed caused by the module not being ready. However, I'm concerned about the original reason why this functionality and comment was added. > diff --git a/src/eap_peer/eap_tls_common.c b/src/eap_peer/eap_tls_common.c > /* > - * At this point with the pkcs11 engine the PIN might be wrong. > - * We reset the PIN in the configuration to be sure to not use > - * it again and the calling function must request a new one. > + * This generally means whatever we're talking to through > + * PKCS#11 simply isn't ready and should be treated as a > + * transient failure. > */ > - os_free(config->pin); > - config->pin = NULL; So what if this error was indeed caused by PIN being incorrect? Wouldn't that result in that same incorrect PIN being tried again every time the connection is attempt? I'd assume that could result in the PIN getting locked which is quite undesirable. It would look useful to split the TLS_SET_PARAMS_ENGINE_PRV_INIT_FAILED return value to at least two different values where one of them would indicate the possibility of the PIN being wrong and that one getting this existing behavior. > + sm->ignore = TRUE; > + return -1; While this new behavior would be used for the case where the PIN has not been used and the failure was due to PKCS#11 not being ready.
diff --git a/src/eap_peer/eap_tls_common.c b/src/eap_peer/eap_tls_common.c index 8710781..78f33c2 100644 --- a/src/eap_peer/eap_tls_common.c +++ b/src/eap_peer/eap_tls_common.c @@ -198,12 +198,12 @@ static int eap_tls_init_connection(struct eap_sm *sm, res = tls_connection_set_params(data->ssl_ctx, data->conn, params); if (res == TLS_SET_PARAMS_ENGINE_PRV_INIT_FAILED) { /* - * At this point with the pkcs11 engine the PIN might be wrong. - * We reset the PIN in the configuration to be sure to not use - * it again and the calling function must request a new one. + * This generally means whatever we're talking to through + * PKCS#11 simply isn't ready and should be treated as a + * transient failure. */ - os_free(config->pin); - config->pin = NULL; + sm->ignore = TRUE; + return -1; } else if (res == TLS_SET_PARAMS_ENGINE_PRV_VERIFY_FAILED) { wpa_printf(MSG_INFO, "TLS: Failed to load private key"); /*