Skip to content

Conversation

@lindig
Copy link
Contributor

@lindig lindig commented May 14, 2025

A user can install server certificates to secure the connection between xapi and their API clients. So far we demanded SHA256 certificates. Accept SHA512 in addition.

The patch renames the predicate to no longer imply that we only accept SHA256.

Tested this:

openssl req -x509 -sha512 -nodes -days 365 -newkey rsa:2048 -keyout mycert.key -out mycert.crt -subj "/CN=localhost"
xe host-server-certificate-install certificate=mycert.crt private-key=mycert.key

A user can install server certificates to secure the connection between
xapi and their API clients. So far we demanded SHA256 certificates.
Accept SHA512 in addition.

The patch renames the predicate to no longer imply that we only accept
SHA256.

Signed-off-by: Christian Lindig <[email protected]>
@lindig lindig requested review from edwintorok and psafont May 14, 2025 13:59
Copy link
Member

@psafont psafont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The string for server_certificate_signature_not_supported in ocaml/idl/datamodel_errors.ml needs to be changed as well to mention SHA512.

A unit test needs to be added to ocaml/gencert/test_lib.mlto ensure that SHA512 signatures work.

Do we want to update the default generated certs to use SHA512 signatures as well?

@edwintorok
Copy link
Contributor

Do we want to update the default generated certs to use SHA512 signatures as well?

I don't think we want to change the defaults at this time. AFAICT there is nothing wrong with SHA-256.

@stormi
Copy link
Contributor

stormi commented May 14, 2025

We've had two separate customers with certs issued by Sectigo who could not import them in XAPI because they were SHA-384. I'm sure they would appreciate if that were supported too.

@andyhhp
Copy link
Collaborator

andyhhp commented May 14, 2025

because they were SHA-384

SHA(any)-384 is one of the idiotic digest sizes; You're doing all the work with the larger internal state size, then truncating the result.

But, it is a standard, and people do use it, so it should be accepted.

@edwintorok
Copy link
Contributor

edwintorok commented May 15, 2025

SHA(any)-384 is one of the idiotic digest sizes; You're doing all the work with the larger internal state size, then truncating the result.

But, it is a standard, and people do use it, so it should be accepted.

Wikipedia says "length extension attacks on SHA-224 and SHA-384 succeed with probability 2^−(256−224) = 2^−32 > 2^−224 and 2^−(512−384) = 2^−128 > 2^−384 respectively.", so in some contexts SHA-384 only has 128 bits of security or less. I don't know whether in the context of TLS cert signatures those attacks would be applicable or not.
But we definitely shouldn't support SHA-224, that seems too risky (luckily noone proposes to support that).

Also we do currently use SHA-384 in the TLS cipher, our 2 ciphers are ECDHE-RSA-AES256-GCM-SHA384 and ECDHE-RSA-AES128-GCM-SHA256, so it is hard to say why SHA-384 is acceptable there but not as a certificate signature.

I'm not entirely convinced that those asking for SHA-384 know it is from the SHA-2 family, as you say it is very easy to confuse it with SHA-3 (which is an entirely different algorithm).

Christian Lindig added 2 commits May 15, 2025 10:35
Mention the supported algorithms, re-arrange the wording to usa list.

Signed-off-by: Christian Lindig <[email protected]>
Signed-off-by: Christian Lindig <[email protected]>
@lindig lindig force-pushed the private/christianlin/CP-307865 branch from b34074f to 44031b5 Compare May 15, 2025 09:35
@psafont psafont added this pull request to the merge queue May 15, 2025
Merged via the queue into xapi-project:master with commit be8e7b3 May 15, 2025
17 checks passed
@stormi
Copy link
Contributor

stormi commented May 16, 2025

I'm not entirely convinced that those asking for SHA-384 know it is from the SHA-2 family

I don't think they do. They just bought certs from a provider that did not provide SHA-256 and were stuck because of that.

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.

6 participants