Skip to content
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

fix descriptions of credentials and verification app options #11273

Closed
wants to merge 2 commits into from

Conversation

@DDvO
Copy link
Contributor

@DDvO DDvO commented Mar 6, 2020

Looking for a way to provide extra untrusted certs for chain building during TLS server cert verification
I found that for both s_client and s_server the documentation of the certificates given with the -cert_chain option attributes them as 'trusted' while in fact they are just untrusted additional certs to use for chain building.

On the other hand the documentation of most of the -chainCAfile, -chainCApath, -chainCAstore, -verifyCAfile, -verifyCApath, and -verifyCAstore options failed to make clear that all the certificates addressed by them are trusted.

When correcting this I found and fixed some mistakes in the file format options and their descriptions.

I've also slightly improved the description of the crl_download and nameopt options.

Moreover, the order of the description of these and related options was somewhat inconsistent, which is fixed as well in this PR.

@DDvO DDvO changed the title fix doc of s_client/server credentials and verifiation options fix doc of s_client/server credentials and verification options Mar 6, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 6, 2020

BTW, how can one provide untrusted certs to help verifying peer certs, to use in case a peer does not send (all of) its cert chain?

@DDvO DDvO force-pushed the siemens:fix_credential_option_doc branch Mar 7, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 7, 2020

It turns out that also the descriptions of verification options for the s_time, x509, crl, req, ts, and verify apps deserve some consistency improvements. Done as well.

@DDvO DDvO changed the title fix doc of s_client/server credentials and verification options fix descriptions of credentials and verification app options Mar 7, 2020
@beldmit
beldmit approved these changes Mar 7, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 7, 2020

Thanks @beldmit for your very swift approval!

Do you know if/how one can provide untrusted certs to help verifying peer TLS certs, to use in case a peer does not send (all of) its cert chain?
I found this possibility only for the verify app and direct use of X509_STORE_CTX_init().

@beldmit
Copy link
Member

@beldmit beldmit commented Mar 7, 2020

I don't know either :(
Not sure if it's a reasonable option because in real life (e. g. browsers) we don't have such a store so servers must include a full chain to avoid errors

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 7, 2020

I agree that for web scenarios pre-known untrusted peer certs make little sense,
but in managed (e.g., industrial) scenarios it can make much sense,
in particular with limited bandwidth where it would be nice to avoid sending the same untrusted certs over and over.

@richsalz
Copy link
Contributor

@richsalz richsalz commented Mar 7, 2020

This is going to be an RFC soon, it's just waiting for text editing: https://datatracker.ietf.org/doc/draft-ietf-tls-certificate-compression/?include_text=1

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 7, 2020

Nice, but this just compresses the certs being transmitted, but does not stop sending them.
And transmission size is not the only potential reason why a TLS party may want/need to augment the cert chain it receives from a peer.

@DDvO DDvO force-pushed the siemens:fix_credential_option_doc branch Mar 9, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 9, 2020

Rebased and partly squashed.
Given that the approval was done two days before, can I merge this PR right away?

@t8m
Copy link
Member

@t8m t8m commented Mar 9, 2020

It is missing OTC member approval.

@kaduk
Copy link
Contributor

@kaduk kaduk commented Mar 10, 2020

Using extra certs for chain building can come into play when the server is sending a potentially valid chain but you want to use a different one, e.g., chaining up to a different or cross-signed root.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 18, 2020

Ping for OTC approval

DDvO added 2 commits Mar 6, 2020
…apps

fix doc of s_client and s_server credentials and verification options
fix doc of verification options also for s_time, x509, crl, req, ts, and verify
correcting and extending texts regarding untrusted and trusted certs,
making the order of options in the docs and help texts more consistent,
etc.
@DDvO DDvO force-pushed the siemens:fix_credential_option_doc branch to 5303a7d Mar 23, 2020
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 23, 2020

Rebased this PR. It still needs an OTC member approval.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Mar 26, 2020

The review/approval by @jaym05700 does not count here.

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Apr 6, 2020

Pinging again on a OTC member review+approval.

@openssl-machine
Copy link

@openssl-machine openssl-machine commented Apr 7, 2020

This pull request is ready to merge

@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Apr 15, 2020

Merged - thanks!

@DDvO DDvO closed this Apr 15, 2020
openssl-machine pushed a commit that referenced this pull request Apr 20, 2020
…apps

fix doc of s_client and s_server credentials and verification options
fix doc of verification options also for s_time, x509, crl, req, ts, and verify
correcting and extending texts regarding untrusted and trusted certs,
making the order of options in the docs and help texts more consistent,
etc.

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11273)
openssl-machine pushed a commit that referenced this pull request Apr 20, 2020
…since #10667

Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com>
Reviewed-by: Matt Caswell <matt@openssl.org>
(Merged from #11273)
@DDvO
Copy link
Contributor Author

@DDvO DDvO commented Apr 20, 2020

I just found that the merge did not really happen, likely due to some misspelling or the like.
Now it's actually merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants