-
Notifications
You must be signed in to change notification settings - Fork 177
ssl: add SSLSocket#sigalg, #peer_sigalg, #group #908
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
45025e0
to
b13cd03
Compare
Honestly, I'm not so sure if there are actual demand for these methods, though I have no objection to adding them if they turn out to be useful.
Re [BTW, the peer/local split in the getter functions is a pity, since the methods (the corresponding OpenSSL API) for setting the allowed signature algorithms use the server/client split instead.] As for |
I talked with the team working for OpenSSL in my company. While I cannot share the details due to our internal information, there are many actual demands from organizations, that they want to migrate to post-quantum cryptography (PQC). If they want to migrate to PQC, they want to audit the TLS servers especially. While many IT departments don't know that they need the PQC yet, the team is predicting they will have the requirements soon.
In this case of the unit tests with RSA case, the value of these method's value are similar. The
However, there is a case printing the following result with RSA PSS by
while I see the following result with RSA PSS on my local.
Also the following case to verify the PQC connection.
So, I still want the method to get the "Peer signing digest" too.
That can be good idea. Let me check the usage of the local-side counterpart (
We may be able to implement the peer/local split in one getter method. But do you like that Ruby binding method should be 1:1 for the OpenSSL C API?
Sure. I will rename the |
There is another reference for demanding PQC. The following document is European Commission's movement to migrate to PQC. The PDF document downloaded on the page says below.
And about the following thing:
I confirmed the team the above output happened by the |
The signature algorithm is closely related to the certificate's public key, so if the goal is just to check whether if ML-DSA is used or not, that's already possible by looking at the certificate ( If there's a demand for getting the IANA name of the signature algorithm, I'm fine with adding it, especially when the binding implementation is simple. I just wasn't sure there was. |
It seems that |
I think this is about the current I confirmed the values of the |
Thanks for investigating this! I believe the commit openssl/openssl@681528c changed the behavior of the "Peer signature type". The change was merged to the openssl master and openssl-3.5 branch according the PR openssl/openssl#27128. I checked the openssl-3.4 branch, and I confirmed the change was not applied. As the It seems that the purpose of this change is written on the issue ticket openssl/openssl#27115. Before the change, the If the
|
I was mostly wondering because you linked #894, as just writing unit tests that use PQC algorithms wouldn't strictly require these methods. If there is a need to get the name of the chosen signature algorithm from Ruby programs, or if you think it would be useful, I have no objection.
I suspect it may have simply been overlooked. |
Okay. I understand my intention was not clear on the ticket. I talked with my colleagues working for the OpenSSL. Here is the result. There is a need to verify the used signature algorithms. Because we cannot determine a used signature algorithm just by checking a used key. For example, when a ML-DSA key is used, the possible signature algorithms are Pure ML-DSA (FIPS 204) and Pre-Hash ML-DSA (FIPS 204 - 5.4 Pre-Hash ML-DSA). Though Pre-Hash ML-DSA is not used in the TLS. And when a RSA key is used, the possible signature algorithms are RSASSA-PKCS1-v1_5 (SHA512, SHA384, RSA256, or etc), or RSASSA-PSS (SHA512, SHA384, RSA256, or etc). So, I think a method such as current
Okay. That makes sense. I still want a method such as |
Let's add it. I think it should come with the local counterpart, too. Also, we should use consistent method names with I'm slightly leaning towards using the unabbreviated "signature_algorithm" in all methods, though "sigalg" would also be fine. "signature_name" feels a bit off for the setter methods since they can take more than just a list of literal names.
Can we postpone adding this until someone demonstrates an actual use case? OpenSSL >= 3.0 is trying to reduce exposure of NID in the public API, so I have a suspicion that the existing functions based on NID may be deprecated in a not so distant future. |
Yes, I also talked about the local counterpart with my colleagues. And I see the method is also useful to test PQC caess. Let's add it. I will rename the methods to the
Yes, let's postpone adding the |
b13cd03
to
d83f3cd
Compare
I rebased this PR fixing the things for the review. Could you review the PR again? Thanks. |
d1aa05a
to
af543ed
Compare
These methods are useful to test post-quantum cryptography (PQC) cases.
af543ed
to
434ef74
Compare
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.
I fixed all the items that you mentioned in the review. So, what do you think now? Does the PR look good to you?
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.
Looks good to me, thanks!
@rhenium This PR is related to #894 - Add features to get the following data from a TLS client.
Note I added the
#include <openssl/obj_mac.h>
forNID_undef
and#include <openssl/objects.h>
for theOBJ_nid2sn()
.I am not sure about the best location where I should put these new logic, the functions and unit tests in the files. I would appreciate if you tell me the best location.
You can also just cherry-pick this PR or refer to this PR or create your new PR.
Could you review the PR? Thank you!