Skip to content
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

Only set authority key identified field if the public key is available #113

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

chrisburr
Copy link
Contributor

When using voms-proxy-init with a proxy certificate and OpenSSL 3 I get an error:

$ voms-proxy-init -out $PWD/test1
$ voms-proxy-init -cert $PWD/test1 -key $PWD/test1 -out $PWD/test2
Your proxy is valid until Tue Feb 14 00:52:34 2023
Error: verification failed.
Parameters unset!

If I use a voms-proxy-init binary that was compiled with OpenSSL 1.1.1 the above example works.

Looking deeper in to the problem I found that this line was failing with OpenSSL 3:

ex11 = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, "keyid");

Inspecting the openssl error queue I found this message:

Error adding extension: error:1100007B:X509 V3 routines::unable to get issuer keyid

If I look at the certifictate that was generated with OpenSSL 1.1.1 I find the AKI is null so I presume OpenSSL 3 is being less permissive about this invalid state:

$ openssl crl2pkcs7 -nocrl -certfile test2 | openssl pkcs7 -print_certs -text -noout | grep -A 4 'Authority Key Identifier'
            X509v3 Authority Key Identifier:
                0.
            Proxy Certificate Information: critical
                Path Length Constraint: infinite
                Policy Language: Inherit all

I think the right thing to do in this case is to just not add the AKI extension.

@giacomini
Copy link
Member

Hi Chris, thanks for you PR. I'll have a look in the next days.
Please note that there is not yet an official release of VOMS for OpenSSL 3, even if builds are available.

@chrisburr
Copy link
Contributor Author

Thanks Francesco!

For some context I ran into this issue for (LHCb)DIRAC which uses the conda-forge build of VOMS (which I maintain). Conda-forge has already dropped OpenSSL 1.1.1 from the default build matrix a couple of weeks ago as the upstream end-of-life is in less than 7 months.

@giacomini
Copy link
Member

The latest official release of VOMS (2.0.16) is actually still on 1.0.2, the default on CentOS 7 :-(

@chrisburr
Copy link
Contributor Author

@chaen Did some more debugging for fts/fts-rest-flask!92 and came to the same conclusion as me (but with a citation to back it up):

The problem is due to proxies being a grey area when it comes to Authority Key Identifier (https://www.rfc-editor.org/rfc/rfc3280#section-4.2.1.1). In any case, this being a non critical extension, the sensible thing to do is probably to just skip it.

@giacomini giacomini self-assigned this Feb 22, 2023
@msalle
Copy link
Contributor

msalle commented Feb 24, 2023

Hi,
quick question, mostly out of curiosity, but also useful for testing, which end-entity certificate didn't have a Authority Key Identifier ?

@chrisburr
Copy link
Contributor Author

The issue is when using a proxy rather than a certificate. The reproducer in the PR description should allow you to demonstrate the issue.

With OpenSSL 1.x you should be able to reproduce it except instead of crashing it lets you put a null value in the extension:

            X509v3 Authority Key Identifier:
                0.

@msalle
Copy link
Contributor

msalle commented Feb 26, 2023

Ah right, hadn't read carefully enough. The crucial point is the 2nd level proxy.
By the way, you can also directly print the cert with the x509 tool of openssl:

openssl x509 -text -noout -in test2|grep -A 4 'Authority Key Identifier'

@giacomini
Copy link
Member

Would it be possible to have the end-entity certificate and the certificates of the generated proxies (only the public part), please?

@msalle
Copy link
Contributor

msalle commented Mar 15, 2023

Hi Francesco,
I don't see it crash with my local build against an older OpenSSL3.0.0, but I do see the X509v3 Authority Key Identifier: being 0. I've sent you a proxychain without private key, which includes 3 certs. The leafproxy at the top shows the weird one, it of course also includes the EEC as the 3rd cert in the chain.

@giacomini
Copy link
Member

It seems to me that the real issue is that the proxy does not contain the Subject Key Identifier, which would become the Authority Key Identifier in the next proxy. So if you have a proxy of a proxy, you get a null AKI.
Would you agree to change voms-proxy-init so that it provides the SKI in the proxy? I don't think it would break anything. I also want to check what the Java version does.

@msalle
Copy link
Contributor

msalle commented Mar 16, 2023

The java version doesn't include the X509v3 Authority Key Identifier, nor the critical CA:FALSE in the proxy certs.
I think there is no reason not to include the Subject Key Identifier so I would probably indeed add it, but either way you need to certainly include either both or none. I've also sent you a chain for the Java-based (but much slower) voms-proxy-init3.
You need to explicitly add a -noregen to the second voms-proxy-init in the above comment, or it will fail. Also made it 1k key to match the v2 version.

@chaen
Copy link

chaen commented Mar 16, 2023

Given that this value was never correct, it certainly isn't used anywhere. Removing it altogether seems faster, easier and safer, as opposed to adding a new feature to VOMS proxies (which we are all working hard to replace with tokens :-) ). How would it affect things to remove the AKI ?

@chrisburr
Copy link
Contributor Author

Hi @giacomini, @msalle, have you had a chance to think about this any further?

@msalle
Copy link
Contributor

msalle commented Apr 21, 2023

Hi @giacomini, @msalle, have you had a chance to think about this any further?

I personally have no strong opinion, either way is fine with me (and I'm not a VOMS maintainer). If it's simple and straightforward to add I would add it, otherwise probably safer to remove indeed.

@giacomini
Copy link
Member

I'll give some thought over the next days.

@giacomini
Copy link
Member

In case we go in the direction to remove the Authority Key Identifier in proxies, in practice it would mean removing the lines https://github.com/italiangrid/voms/blob/develop/src/sslutils/proxy.c#L356-L400, right?

@msalle
Copy link
Contributor

msalle commented Apr 27, 2023

There is more probably if I run a grep -ri 'auth.*key'

@giacomini
Copy link
Member

I've checked with Vincenzo: that piece of code should be executed only if we are not managing a proxy, but that check is missing. Consider that this piece of code is used also by voms-fake, which allows to generate all sorts of certificates for testing purposes. So, would you please try the following change: add if (args->proxyver == 0) before https://github.com/italiangrid/voms/blob/develop/src/sslutils/proxy.c#L358? If it works, you can resubmit this PR with just this change and I'll merge it.

@DrDaveD
Copy link
Contributor

DrDaveD commented Sep 7, 2023

I confirm that the change in this PR also fixes the problem I addressed in #120, that of making voms-proxy-init work with CILogon user certificates on EL9. It also works for that purpose with the insertion of if (args->proxyversion == 0) at line 358 of proxy.c.

@DrDaveD
Copy link
Contributor

DrDaveD commented Sep 7, 2023

And in my case the fix already in this PR is still needed because I was working with a certificate and not a proxy. So I propose merging this PR and adding another one that adds if (args->proxyversion == 0) for the proxy case.

@DrDaveD
Copy link
Contributor

DrDaveD commented Sep 7, 2023

The alternative fix for the original problem described for this PR is in #121.

@giacomini
Copy link
Member

I merge both this and #120. Thanks to all, especially for your patience. Before I tag another RC, can you give me some feedback if everything's alright? our testsuite is not yet in a good shape, so I need to rely on your field experience.

@giacomini giacomini merged commit 6a08378 into italiangrid:develop Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants