RFC7512 URI support #6860
RFC7512 URI support #6860
Conversation
This comment has been minimized.
This comment has been minimized.
|
You can close and reopen the PR to retrigger all builds. |
This comment has been minimized.
This comment has been minimized.
|
Thanks the close/open did help. Please, how to move forward ?
I will need to find a bit of time to review it :)
… |
|
I think it would be better to add this to |
I do not get the point: currently, |
I think I got your ideas @bukka , I'll provide a v2 of this serie later tonight. |
|
I did a complete rebase thanks to your comments @bukka. Moreover, I tried to get it fully generic. Your feedbacks are welcomed. |
There was a problem hiding this comment.
It looks mostly good, just some minor things.
Also the new functionality needs to be covered by tests. There should be few tests added or extended that will show that local_cert and local_pk works with cert and pk objects and possibly with string PEM and file:// prefix.
In addition it would be good to cover the pkcs11 functionality but in this case we can't really run it in the pipeline because it requires some working device obviously. I have been thinking what would be the best way to test and we could just add a test for cert and pk that will run only if for example env OPENSSL_TEST_PKCS11_URI is set - it means adding skip if it's not set. Something like:
if (!getenv("OPENSSL_TEST_PKCS11_URI")) die("skip No pkcs11 URI")
If that env is set, then it would be possible to test the functionality. I think this will be especially useful for debugging issues, manual testing and as some kind of usage example.
|
@vjardin Ah I just noticed this extension https://github.com/gamringer/php-pkcs11/ and see that you have been contributing into it so I guess you might have some experience with SoftHSM which looks maybe as even better idea for the test. Not sure if we could make it work in the pipeline but even if not, it would be still useful to have some skip and add notes to the test how to set it up. |
|
@bukka : regarding some tests with SoftHSM2, I think it could be doable for checking:
and they would be enough for such integration tests of the URI of the RFC7512. What do you think ? For sure, in order to succeed, I will need your deep help for such tests. It will be a long process since it will be a first of SoftHSM2 with the mainline php-src/phpt. What's about having two steps:
Limitations of the tests:
UPDATE: I think I found a quick/easy design for such tests, but the CI environment will require the following tools:
I'll post an update soon. |
|
Hi @bukka , the test (738180a) passes on Ubuntu 20.04. I am not going to check all the combination cases, I do not have the proper disk/memory/skill ressources in order to check all the other cases. The skip section should disable the tests when the tools are not properly installed. I tried to follow the other OpenSSL's PHPT SKIP section. |
This comment has been minimized.
This comment has been minimized.
|
@vjardin The pkcs11 test looks pretty cool. I will try it later but if that works seems definitely enough in terms of pkcs11 functionality. Could you then just add tests for supporting different key and cert formats / objects (addition of php_openssl_x509_from_str and php_openssl_pkey_from_zval) in TLS stream (just normal certs and keys - not pkcs11) to cover this new functionality? See for example tls_wrapper.phpt how such tests are done (there might be even better examples containing the local key and cert). |
|
btw if you want to re-trigger build it's usually just easier to do |
Thanks. Adding more tests won't add much benefits since under the hood OpenSSL does the job. Good tests should remain simple and easy to maintain. More tests using SSL are not required since there are already many client/servers SSL tests performed by openssl/tests. The cornerstone is to load a key (file, memory buffer/string, URI) which is currently tested, then any other tests with some SSL transport layers are going to be just some 'legacy' OpenSSL tests. |
|
Hi @bukka : do you have some comments about this latest update ? |
|
I have been a bit busy with some other stuff but will give it a try once I have got more time. From just reading the code I think the logic looks good. There just some coding style issue so please fix those. Also please squash it to a single commit and push force it. |
Add RFC7512 URIs with openssl_pkey_get_public(), openssl_pkey_get_private() and openssl_x509_read(), support When a URI based on the RFC7512 is used, the private key should be loaded from the engine instead of using a local file. Reviewed-by: Jakub Zelenka <bukka@php.net>
|
Thanks @vjardin . Code looks good. I didn't have too much time today but did some initial testing at least but getting failure when loading PKCS11 engine even though I configure it in openssl.cnf and installed engine. It's probably because I have got a bit custom setup (not using system OpenSSL but compiling multiple different versions to different directories). I need to debug it to see what's going on. Unfortunately I'm quite busy so it might take me few weeks. There's no rush really as this is just for master only anyway. |
|
Thanks @bukka for trying. I suggest that you get a fresh Ubuntu 20.04 VM then you'll get it quickly. I'd like to avoid having such topic on hold for a long time: I'll be quite busy by end of next week, so I'll have less time to update if needed (hard to align our 2 agendas :( ). If needed contact me over DM and I can provide you a fresh VM with Ubuntu and a sudo-er account today. |
|
Hi @bukka : I guess we have time till the next release but I do not feel comfortable waiting weeks since it won't be fresh anymore in my mind if I still need to comment/update. How to smooth the integration of this patch ? As stated, I can provide you with a VM if needed. |
|
I don't think I will need your input. If I find any issue I will resolve it myself. My time is very limited but have got this quite high on my priority list so I should be able to get to it in the coming months. However I need to prioritise issues related support for OpenSSL 3.0. after seeing quite a few failing tests. That's much more important but as soon as I'm done with that, I should be able to find some time for this. |
|
Good luck regarding OpenSSL 3.0; its churns are creating lot of work. Are you focusing on this topic #7002 ? I guess it will impact this serie too. |
|
Yes that's the one. Engines will be deprecated but should still work. I think there's no stable provider for pkcs11 and we still need to support older versions so I think adding pkcs11 engine support to PHP still makes sense but will definitely require some testing of this PR as well. Looks like pkey tests mostly works (except one DH / DSA test) so hopefully this will work too if the libp11 engine works too but hard to say without trying :) |
|
Do you mean that, as a bottom line, this patch should be merged after OpenSSL 3.0 becomes available with PHP ? |
|
It's more that 3.0 is a bit higher on my priority list but it's not prerequisite for this PR. |
|
I do respect that you look at OpenSSL 3.0 but Opensource contributions require a smooth review/integration which is not the case here. If you are too busy because of OpenSSL 3.0, please either assign someone else on reviewing this pull request or please do respect that we cannot keep waiting. Frankly, the more we wait with this issue the less my brain is on it if I have to provide an update. Please ? |
|
All of the previous maintainers of this extension are inactive so I'm the only active maintainer and probably all other core developers don't either have time or knowledge to handle bigger PR's in this extension. You should realise that most people working on PHP are volunteers doing that in their free time (except 2 or 3 exceptions) and we have to prioritise things that are in our eyes more important and impact more users. That said I will do my best to find some time for this and get it to 8.1. As I said before all of your work is done and I won't need your help for any updates so don't worry about that part. :) |
|
Me too I am volunteer on it too over night and WE. So, I feel, in order to remain motivated, that it should be smooth. |
|
I understand that there is not enough volunteer supporting OpenSSL/PHP, but in order to sustain the growth of a community, some progresses are required. Without any progress, as new volunteer, we give up even before entering into the game. If you do not have time to apply this pull-request, please, can you delegate to someone else ? |
|
Sorry haven't had much time recently but any of the core developers can pick this up if they want and can. I'm not blocking anything... Few weeks ago I actually tried to put together a test for loading keys from pkey but this doesn't work for me for some reason - need to do some debugging. It was a quick attempt so I might have missed something: See bukka@a6d842d |
|
FWIW the OpenSSL 3.0 compatibility is now implemented. I'm not familiar with PKCS11. A problem here is going to be that the APIs this uses are deprecated in OpenSSL 3, because engines are being replaced with providers. But I assume that no provider-based implementation of p11 is available yet? |
|
@nikic I do not understand, what do you mean ? |
@bukka , please what's about this serie and 8.1 now ? |
|
Since May, the comment was:
then, nothing. It is not a nice way to support contributions. Please @bukka , can you proceed with this serie ? It is getting older and older now. |
| char *verbose = NULL; | ||
| ENGINE *engine; | ||
|
|
||
| engine = ENGINE_by_id("pkcs11"); |
There was a problem hiding this comment.
As already pointed in #6860 (comment) see https://www.openssl.org/docs/man3.0/man7/migration_guide.html#Providers-are-a-replacement-for-engines-and-low-level-method-overrides
Engines are replaced by providers, I'm not sure it makes sense to add compatibility with openssl1.1 ATM
There was a problem hiding this comment.
Please, can someone propose an update of this serie that use the providers instead of "engine" ? As stated, I am not working on this topic anymore. I was focused on it few months ago, but I cannot spend months in waiting.
|
Sorry for delay but I have got more important things to do. After a bit of re-considering I think we at least need to wait for PKCS11 provider to be available as it should be a primary thing that we will need to support going forward. This could be possibly a compatibility layer for older version but the primary implementation should be for the future provider. So basically this needs to wait longer. |
|
So, we should close this merge request even if it is not integrated. I am busy on some other topics. |
|
I wouldn't mind to have this open as a reminder of the work that needs to be done once some PKCS#11 provider is available. It's a nice work and we could potentially merge it with additional provider changes (e.g. I could just update this PR if you are busy). The delay is mainly to assure that we won't merge something that we would have to soon deprecate... |
Thanks for taking over this work, you'll be welcomed. |
When a URI based on the RFC7512 is used, the private key should be loaded
from the engine instead of using a local file.