Use consistently SHA-256 for signatures (possible fix for issue #1992) #1999
Conversation
src/XrdCrypto/XrdCryptosslgsiAux.cc
Outdated
| @@ -455,7 +455,7 @@ int XrdCryptosslX509CreateProxy(const char *fnc, const char *fnk, | |||
| } | |||
| // | |||
| // Sign the request | |||
| if (!(X509_REQ_sign(preq, ekPX, EVP_sha1()))) { | |||
| if (!(X509_REQ_sign(preq, ekPX, EVP_sha256()))) { | |||
There was a problem hiding this comment.
I wonder if this could/should be made configurable by the user.
There was a problem hiding this comment.
At run time or build time?
There was a problem hiding this comment.
I think run time would be better, but I'd like to hear what @abh thinks about it too. If SHA-256 works everywhere and is relatively future-proof, we could just switch the default and still leave it hard-coded.
There was a problem hiding this comment.
But also, why not SHA3 now? Old OpenSSL won't support it?
There was a problem hiding this comment.
Have a look at the revised patch
There was a problem hiding this comment.
(Yes, SHA3 is not supported by the OpenSSL's around - introduced in 1.1.1 - so to force it requires some additional checks ... SHA-256 is present in all the versions still around).
Default is SHA-256. The SHA funtion is controlled by name by the env var XrdCryptoGSISHA .
|
I'd say there is no reason in the context of this use. The world is moving
away from sha1 and isn't quite ready for sha3. So, the only thing left is
sha256 which works in all contexts. So, there seems nothing to configure
here.
…On Mon, 17 Apr 2023, Guilherme Amadio wrote:
@amadio commented on this pull request.
> @@ -455,7 +455,7 @@ int XrdCryptosslX509CreateProxy(const char *fnc, const char *fnk,
}
//
// Sign the request
- if (!(X509_REQ_sign(preq, ekPX, EVP_sha1()))) {
+ if (!(X509_REQ_sign(preq, ekPX, EVP_sha256()))) {
I wonder if this could/should be made configurable by the user.
--
Reply to this email directly or view it on GitHub:
#1999 (review)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
|
| static const EVP_MD *sslSHAFun = 0; | ||
| if (!sslSHAFun) { | ||
| const char *_md = getenv("XrdCryptoGSISHA") ? getenv("XrdCryptoGSISHA") : "sha256"; | ||
| sslSHAFun = EVP_get_digestbyname(_md); |
There was a problem hiding this comment.
If the user passes a bad name, this will assign nullptr to sslSHAFun, which will probably cause trouble. We need to check that we get a non-null pointer from EVP_get_digestbyname and fallback to the default otherwise.
I was also thinking that if the only place where this is needed is in xrdgsiproxy, we could add an option on the command line, like -sig sha256 which would use a different signature than the default, but that can be done later.
There was a problem hiding this comment.
Ok, so to make it cleaner I close this PR and make another one only with the initial changes, not to pollute the history.
|
Ok, let's go for the initial change to just replace sha1 with sha256 then. I still like the idea of providing a |
|
Closing to rebase and make it cleaner |
On systems honouring, by default, the deprecation of SHA1, such as AlmaLinux 9, xrdgsiproxy fails (see issue #1992).
This patch replaces use of SHA1 by SHA-256 for signatures. SHA-256 is supported by all OpenSSL versions still around,
which makes it easier from point of view of portability. Move to SHA3 should be considered for the future.