-
Notifications
You must be signed in to change notification settings - Fork 7.5k
SSL: support loading keys via OSSL_STORE. #436
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
Conversation
c755803
to
6701e23
Compare
Changes:
The last change may need more context than the commit message contains:
There are 3 provider-specific solutions:
but nothing that works in general and ensures that the worker doesn't get blocked by a sudden unnoticed pin prompt. Tangentially related, but |
I have been actually working quite heavily with nginx and pkcs11-provider in recent months and have got some fixes for the PIN issues (you can find more info in some of the PR's that I have open but I have got more things coming) . In my case it's even more complicated because I have pkcs11-proxy in between. But mostly it's more a problem that the key refresh uri (saved in provider object) is not always passed (that's where you need to specify pin-value or pin-source if the key is in different slot than the default). What is specified in pkcs11-module-token-pin is used as the default pin (currently it is the first slot in the list but added a PR to select it) if no pin is provided with a key. This is also important when client cert is used because it will use it for importing the public key (which is happening due to how provider works in OpenSSL). So basically you need to always set pkcs11-module-token-pin to the first slot PIN (or later selected ones once my PR is merged) and then specify PIN's for all key in the PKCS11 URI. All of the above are just issue in pkcs11-provider and I didn't find any problem in nginx so far. The loading keys in this way is not really necessary because the current way is to create PEM file containing the URI. That said it might be nicer to be able to specify it directly so it is a nice improvement. The UI method change might be also nicer way to fail but not a big deal either. The UI method actually immediately fails anyway if running in background so it might be just a slightly nicer error. Currently one can actually specify the password if running nginx in foreground but that's not really useful in real use. |
For the record. Using pkcs11-provider won't obviously work in OpenSSL 1.1.1, although the OSSL_STORE interface is supported there |
I considered making the change about providers and hiding everything under OpenSSL >= 3.0.0 check, but decided against that. Nothing in the code is specific to providers, and it is technically possible to register a scheme handler in 1.1.1 via The Although I still consider |
I don't see how default Interestingly enough, passing Also, in the above case, when using
So, this needs to be addressed in some way.
This will break loading password protected private keys from disk via "store:" without any diagnostics.
Apparently, Setting To sum up:
Patch on top (default to diff --git a/src/event/ngx_event_openssl_cache.c b/src/event/ngx_event_openssl_cache.c
index d0b81e469..74131ce79 100644
--- a/src/event/ngx_event_openssl_cache.c
+++ b/src/event/ngx_event_openssl_cache.c
@@ -727,6 +727,21 @@ ngx_ssl_cache_pkey_create(ngx_ssl_cache_key_t *id, char **err, void *data)
#endif
}
+ cb_data.encrypted = 0;
+
+ if (*passwords) {
+ cb_data.pwd = (*passwords)->elts;
+ tries = (*passwords)->nelts;
+ pwd = &cb_data;
+ cb = ngx_ssl_cache_pkey_password_callback;
+
+ } else {
+ cb_data.pwd = NULL;
+ tries = 1;
+ pwd = NULL;
+ cb = NULL;
+ }
+
if (id->type == NGX_SSL_CACHE_STORE) {
#ifdef ERR_R_OSSL_STORE_LIB
@@ -737,7 +752,9 @@ ngx_ssl_cache_pkey_create(ngx_ssl_cache_key_t *id, char **err, void *data)
uri = id->data + sizeof("store:") - 1;
- store = OSSL_STORE_open((char *) uri, NULL, NULL, NULL, NULL);
+ store = OSSL_STORE_open((char *) uri,
+ UI_UTIL_wrap_read_pem_callback(cb, 0), pwd,
+ NULL, NULL);
if (store == NULL) {
*err = "OSSL_STORE_open() failed";
@@ -762,7 +779,12 @@ ngx_ssl_cache_pkey_create(ngx_ssl_cache_key_t *id, char **err, void *data)
OSSL_STORE_close(store);
- return pkey;
+ if (pkey == NULL) {
+ *err = "OSSL_STORE_load() failed";
+ return NULL;
+ }
+
+ goto done;
#else
@@ -777,21 +799,6 @@ ngx_ssl_cache_pkey_create(ngx_ssl_cache_key_t *id, char **err, void *data)
return NULL;
}
- cb_data.encrypted = 0;
-
- if (*passwords) {
- cb_data.pwd = (*passwords)->elts;
- tries = (*passwords)->nelts;
- pwd = &cb_data;
- cb = ngx_ssl_cache_pkey_password_callback;
-
- } else {
- cb_data.pwd = NULL;
- tries = 1;
- pwd = NULL;
- cb = NULL;
- }
-
for ( ;; ) {
pkey = PEM_read_bio_PrivateKey(bio, NULL, cb, pwd);
@@ -811,12 +818,15 @@ ngx_ssl_cache_pkey_create(ngx_ssl_cache_key_t *id, char **err, void *data)
return NULL;
}
+
+ BIO_free(bio);
+
+done:
+
if (cb_data.encrypted) {
*passwords = NGX_SSL_CACHE_DISABLED;
}
- BIO_free(bio);
-
return pkey;
}
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Posted my review, please see above.
[Unfortunately, deficient github interface force me to write something here in order to press the button.]
6701e23
to
fec4dbb
Compare
A new "store:..." prefix for the "ssl_certificate_key" directive allows loading keys via the OSSL_STORE API. The change is required to support hardware backed keys in OpenSSL 3.x using the new "provider(7ossl)" modules, such as "pkcs11-provider". While the engine API is present in 3.x, some operating systems (notably, RHEL10) have already disabled it in their builds of OpenSSL. Related: https://trac.nginx.org/nginx/ticket/2449
Certain providers may attempt to reload the key on the first use after a fork. Such attempt would require re-prompting the pin, and this time we are not able to pass the password callback. While it is addressable with configuration for a specific provider, it would be prudent to ensure that no such prompts could block worker processes by setting the default UI method. UI_null() first appeared in 1.1.1 along with the OSSL_STORE, so it is safe to assume the same set of guards.
5ddb503
to
9dffdaa
Compare
For the record: the following error and related issues with
As After an internal discussion, we agreed that we are not going to support this configuration. This is the least of the problems possible with intermixed use of code from two different builds (and versions) of libcrypto. There's a known issue with pkcs11-provider, a pin passed through |
Hello @bavshin-f5, As of NGINX versions prior to the change in this PR, is it possible to configure OpenSSL providers, via an OpenSSL configuration file, in such a way that the provider's key and certificate can be used by NGINX to establish TLS connections? From what I understand, this pull request enables selecting specific keys. However, is it also possible for the provider to expose a default key that NGINX could use without explicitly selecting it? |
This depends on the provider. In case of pkcs11-provider, it was already possible to use keys through the special PEM key (and possibly cert as well) that has specific ASN.1 structure. The pkcs11-provider has it's own decoded for such format so such key can already be loaded. I developed a small tool that sets everythin up for testing. The key format part is as follows: https://github.com/bukka/nginx-pkcs11-provider/blob/main/nginx_pkcs11_provider/generate_keys.py#L98-L106 . |
@bukka What would the NGINX configuration file look like I'm this case? Is it possible to omit the |
It's the same as normal nginx configuration. There is still As I said this is provider specific because the provider loads the key so it can have it's own decoding logic like in the case of pkcs11-provider. This is not just nginx thing - you can do the same in curl or whatever other OpenSSL users. |
A new "store:..." prefix for the "ssl_certificate_key" directive allows loading keys via the OSSL_STORE API.
The change is required to support hardware backed keys in OpenSSL 3.x using the new "provider(7ossl)" modules, such as "pkcs11-provider". While the engine API is present in 3.x, some operating systems (notably, RHEL10) have already disabled it in their builds of OpenSSL.
Related: https://trac.nginx.org/nginx/ticket/2449
Successfully tested with:
It is unclear if we want to handle property query to resolve ambiguities, e.g.
-provider tpm2 -propquery '?provider=tpm2' -key file:$name.key
.