Signature algorithm refactor part 2. #2324
Conversation
| * algorithms (signature scheme) extension | ||
| */ | ||
| typedef struct sigalg_lookup_st { | ||
| /* TLS 1.3 signature scheme name */ |
There was a problem hiding this comment.
is it reasonable to put the comment on same line as the field? okay if not.
| /* Index of signature algorithm */ | ||
| int sig_idx; | ||
| /* NID of signature algorithm */ | ||
| /* Combined hash and signature NID, if any */ |
There was a problem hiding this comment.
Is this second comment the definition of what the sigalg is? if not, I'm confused.
There was a problem hiding this comment.
Oops, added another comment for some reason, removed.
| const EVP_MD *peer_md; | ||
| /* Signature type: public key type or EVP_PKEY_RSA_PSS for PSS */ | ||
| int peer_sigtype; | ||
| /* Sigalg peer actualy uses */ |
| goto f_err; | ||
| /* | ||
| * Check certificate type is consistent with ciphersuite. For TLS 1.3 | ||
| * skip check as TLS 1.3 ciphersuites can be used with any certificate |
There was a problem hiding this comment.
perhaps "skip check, since" ?
| exp_idx = ssl_cipher_get_cert_index(s->s3->tmp.new_cipher); | ||
| if (exp_idx >= 0 && i != exp_idx | ||
| && (exp_idx != SSL_PKEY_GOST_EC || | ||
| (i != SSL_PKEY_GOST12_512 && i != SSL_PKEY_GOST12_256 |
There was a problem hiding this comment.
maaybe a define for ISNT_GOST makes sense? or !IS_GOST ?
| /* If Suite B only P-384+SHA384 or P-256+SHA-256 allowed */ | ||
| if (tls1_suiteb(s)) { | ||
| if (curve_id[0]) | ||
| EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); |
| return 0; | ||
| } | ||
| } else { | ||
| unsigned char curve_id[2], comp_id; |
| EC_KEY *ec = EVP_PKEY_get0_EC_KEY(pkey); | ||
| if (SSL_IS_TLS13(s)) { | ||
| /* For TLS 1.3 check curve matches signature algorithm */ | ||
| int curve = EC_GROUP_get_curve_name(EC_KEY_get0_group(ec)); |
| if (tls_sigalg_get_hash(sig) != NID_sha384) { | ||
| SSLerr(SSL_F_TLS12_CHECK_PEER_SIGALG, | ||
| SSL_R_ILLEGAL_SUITEB_DIGEST); | ||
| if (curve_id[1] == TLSEXT_curve_P_256) { |
There was a problem hiding this comment.
boy, that's a nest of if's! I can't think of a cleaner way to do it, tho.
There was a problem hiding this comment.
A switch?
switch (curve_id[1]) {
case TLSEXT_CURVE_P_256:
...
break;
case TLSEXT_CURVE_P_384:
...
break;
default:
return 0;
}There was a problem hiding this comment.
That's just the original "logic". There might be a cleaner way using the new tables, I'll have a look.
There was a problem hiding this comment.
Added a new commit that is functionally equivalent to the original but tidier.
| if (tls_sigalg_get_hash(*p) == NID_sha1 | ||
| && tls_sigalg_get_sig(*p) == rsign) | ||
| const SIGALG_LOOKUP *lu = tls1_lookup_sigalg(*p); | ||
| if (lu && lu->hash == NID_sha1 && lu->sig == rsign) |
There was a problem hiding this comment.
blank line after the decl. add explicit "!= NULL" test
Remove unnecessary lookup operations: use the indices and data in the lookup table directly.
|
Ping @richsalz I think this has addressed your comments now. Please check. |
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
Remove unnecessary lookup operations: use the indices and data in the lookup table directly. Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2324)
|
Thanks, pushed. |
Checklist
Description of change
This PR is the second part of the signature algorithms reorganisation.
The digest and public key indices are added to the SIGALG_LOOKUP table. This simplifies several pieces of code which no longer need to lookup the values in a separate table.
The peer signature algorithm used is stored instead of separate digest and signature type fields.
The test for ciphersuite and certificate type consistency has been removed for TLS 1.3 as the ciphersuites can be used with all certificate types. This means we can now interop with picotls server and ECDSA. We can't test that (yet) because server side ECDSA and TLS 1.3 is not currently supported.