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
Draft 19 certificate request format support. #2918
Conversation
Also TODO: trace support. |
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.
some nits.
ssl/statem/statem_clnt.c
Outdated
goto err; | ||
} | ||
int al = SSL_AD_DECODE_ERROR; | ||
unsigned int i; |
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.
blank line after decl. and should i be size_t?
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.
There's no reason i
should be unsigned, even... it's only used as an index as far as I can see (?)
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.
It makes sense to me for it to be size_t since it is bounded by the size of something (i.e. SSL_PKEY_NUM) and can't be -ve.
ssl/statem/statem_lib.c
Outdated
} | ||
|
||
int parse_ca_names(SSL *s, PACKET *pkt, int *al) | ||
{ |
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.
undent
ssl/statem/statem_lib.c
Outdated
} | ||
|
||
namestart = namebytes; | ||
|
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.
delete this blank line, the one at 1913, and maybe add one before 1918?
ssl/statem/statem_lib.c
Outdated
|
||
return 1; | ||
|
||
decerr: |
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.
undent the label
ssl/statem/statem_lib.c
Outdated
|
||
decerr: | ||
*al = SSL_AD_DECODE_ERROR; | ||
err: |
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.
undent
ssl/statem/statem_srvr.c
Outdated
int i; | ||
STACK_OF(X509_NAME) *sk = NULL; | ||
|
||
int al = SSL_AD_INTERNAL_ERROR; |
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.
blank line after decl
ca1d3ca
to
505380d
Compare
Update: rebased and comments addressed. Tests are in #2969, the additional APIs etc. can be part of a separate PR. Taken out of WIP. |
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.
one question, but assuming you address it (however appropriate): +1
} | ||
|
||
sk_X509_NAME_pop_free(s->s3->tmp.ca_names, X509_NAME_free); | ||
s->s3->tmp.ca_names = ca_sk; |
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.
free the old value? suppose peer sends this extension twice, for example?
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.
The new extension code rejects duplicate supported extensions.
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2918)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2918)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2918)
Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2918)
Checklist
Description of change
Add support for the TLS 1.3 draft-19 certificate request format.
It passes 'make test' but no other interop testing has been done: there aren't many implementations about yet ;-)
This adds support for the certificate_authorities extension which can be used by client and server.
The signature algorithms extension can now be generated/parsed by both client and server.
Still todo:
Check new extension code is correct. Maybe rename/move the signature algorithms functions: e.g. tls_construct_ctos_sig_algs can now also construct signature algorithms from server to client too.
Add functions to allow setting of CA list client side and retrieval server side and appropriate options to s_client, s_server and SSL_CONF.
Document new functions.
Add tests: the certificate request distinguished names is currently untested too: DONE in #2969