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

ssl: Set engine implicitly when a PKCS#11 URI is provided #2333

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ansasaki
Contributor

ansasaki commented Feb 23, 2018

This allows the use of PKCS#11 URI (RFC 7512) for certificates and keys without setting the corresponding type as "ENG" explicitly. If a PKCS#11 URI is provided for certificate, key, proxy_certificate, or proxy_key, the corresponding type is set as "ENG" when not provided explicitly.

If OpenSSL is used as the cryptographic library and a PKCS#11 URI is provided, the engine is set to "pkcs11" when not provided explicitly. This uses the engine_pkcs11 engine (https://github.com/OpenSC/libp11).

A test script can be found here:
https://github.com/ansasaki/toolbox/blob/master/curl-with-p11.sh

The test uses GnuTLS as the test server and SoftHSM as the PKCS#11 device.

This is related with #974

ansasaki added some commits Feb 19, 2018

Set engine implicitly when a PKCS#11 URI is provided
This allows the use of PKCS#11 URI for certificates and keys without
setting the corresponding type as "ENG" and the engine as "pkcs11"
explicitly. If a PKCS#11 URI is provided for certificate, key,
proxy_certificate or proxy_key, the corresponding type is set as
"ENG" if not provided and the engine is set to "pkcs11" if not
provided.
Add description for PKCS#11 URI usage in man
Add descriptions in man pages of the usage and the behavior changes
when using PKCS#11 URI's (RFC 7512) for certificates and keys.
@bagder

This comment has been minimized.

Member

bagder commented Mar 4, 2018

I don't like how this code adds 6(!) (separate) string comparisons for "pkcs11:". It adds multiple code chunks that look almost identical but they are not. I'm not sure what the fix for this is, but can we think of a nicer way to do this?

Also, should the string check really require the string to be lowercase or should it be done case insensitively?

@ansasaki

This comment has been minimized.

Contributor

ansasaki commented Mar 5, 2018

I don't like how this code adds 6(!) (separate) string comparisons for "pkcs11:". It adds multiple code chunks that look almost identical but they are not. I'm not sure what the fix for this is, but can we think of a nicer way to do this?

Sure, we can try to find the best way. It is possible to write a function to check if a given string is a PKCS#11 URI and at least make a single base code. But it will do the string comparison in the same way. And since different parameters can be independently provided as PKCS#11 URI, I believe that there is no other way than doing it many times in the almost identical way. That would result in substituting the lines:

if(!strncmp(<some_string>, "pkcs11:", 7)) {

for

if(is_pkcs11_uri(<some_string>)) {

where is_pkcs11_uri() is defined as:

static bool is_pkcs11_uri(const char * string) {
  return !strncmp(string, "pkcs11:", 7);
}

But the other lines will remain the same. This would (potentially) add a function call overhead in exchange for the unified location for the check. Do you have any suggestion to improve this?

Also, should the string check really require the string to be lowercase or should it be done case insensitively?

According to RFC 7512, yes, it must be lowercase.

Add function to check if a string is a PKCS#11 URI
The function checks if the string begins with "pkcs11:".
@bagder

This comment has been minimized.

Member

bagder commented Mar 15, 2018

According to RFC 7512, yes, it must be lowercase

Where does it say that? It refers to the string being a URI and "pkcs11" being a URI scheme. URIs are defined by RFC 3986 which says:

Although schemes are case-insensitive, the canonical form is lowercase and documents that specify schemes must do so with lowercase letters.

RFC 4395 in turn defines how to register URI schemes and it says:

Note that although scheme names are case insensitive, scheme names MUST be registered using lowercase letters.

@ansasaki

This comment has been minimized.

Contributor

ansasaki commented Mar 16, 2018

You are right, I was not aware about it. Thank you, I will be more careful with my affirmations.
This same issue is present in OpenSC/libp11 (they consider it case sensitive in the same way I did). I will open an issue for them.

It seems it have to be fixed in tool_getparam.c, line 340, too.

I will use the functions provided in strcase.h instead. Is it OK?

And I do not like the way I did, duplicating the is_pkcs11_uri() function in two places (in lib/vtls/openssl.c and src/tool_operate.c). Is it possible to do it in one place shared between libcurl and curl? In which file should this function be?

@jay

This comment has been minimized.

Member

jay commented Apr 9, 2018

ansasaki added some commits Apr 6, 2018

Fix PKCS#11 URI detection to be case insensitive
The RFC 7512 defines the URI scheme "pkcs11". As defined in RFC 3986,
URI schemes are case-insensitive, although the canonical form must be
written in lowercase. The RFC 4395 requires URI schemes names to be
registered using lowecase letters.
Add PKCS#11 URI unit test for parse_cert_parameter()
Extend the existing unit test to cover the PKCS#11 URI case

@ansasaki ansasaki force-pushed the ansasaki:master branch from 3ca3e1c to 5de4302 Apr 17, 2018

@nmav

This comment has been minimized.

Contributor

nmav commented Aug 6, 2018

That patch looks like a good way forward in two aspects

  1. unifying curl with the two back-ends that support PKCS#11 URIs (openssl and gnutls)
  2. (most important) Allowing PKCS#11 URIs to be used in the place of files to specify certificates and private keys; that's a goal we have in Fedora

Bottom line, it would be great if that would be merged!

@bagder

This comment has been minimized.

Member

bagder commented Aug 8, 2018

Thanks!

@bagder bagder closed this in 298d256 Aug 8, 2018

xquery added a commit to xquery/curl that referenced this pull request Aug 9, 2018

ssl: set engine implicitly when a PKCS#11 URI is provided
This allows the use of PKCS#11 URI for certificates and keys without
setting the corresponding type as "ENG" and the engine as "pkcs11"
explicitly. If a PKCS#11 URI is provided for certificate, key,
proxy_certificate or proxy_key, the corresponding type is set as "ENG"
if not provided and the engine is set to "pkcs11" if not provided.

Acked-by: Nikos Mavrogiannopoulos
Closes curl#2333

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

ssl: set engine implicitly when a PKCS#11 URI is provided
This allows the use of PKCS#11 URI for certificates and keys without
setting the corresponding type as "ENG" and the engine as "pkcs11"
explicitly. If a PKCS#11 URI is provided for certificate, key,
proxy_certificate or proxy_key, the corresponding type is set as "ENG"
if not provided and the engine is set to "pkcs11" if not provided.

Acked-by: Nikos Mavrogiannopoulos
Closes curl#2333

@lock lock bot locked as resolved and limited conversation to collaborators Nov 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.