[prev in list] [next in list] [prev in thread] [next in thread] 

List:       tomcat-dev
Subject:    Re: (tomcat-native) branch main updated: BZ 67818: SSL#setVerify()/SSLContext#setVerify() silently s
From:       Michael Osipov <michaelo () apache ! org>
Date:       2023-10-31 8:36:14
Message-ID: pony-559d3d5b-79df-4a4c-85da-923ff6bf98fa-dev () tomcat ! apache ! org
[Download RAW message or body]

On 2023/10/30 15:47:20 Christopher Schultz wrote:
> Michael,
> 
> On 10/30/23 08:40, Michael Osipov wrote:
> > On 2023/10/30 11:50:55 Mark Thomas wrote:
> > > 30 Oct 2023 10:25:07 michaelo@apache.org:
> > > 
> > > > This is an automated email from the ASF dual-hosted git repository.
> > > > 
> > > > michaelo pushed a commit to branch main
> > > > in repository https://gitbox.apache.org/repos/asf/tomcat-native.git
> > > > 
> > > > 
> > > > The following commit(s) were added to refs/heads/main by this push:
> > > > new ccc6bfe99 BZ 67818: SSL#setVerify()/SSLContext#setVerify()
> > > > silently set undocumented default verify paths
> > > > ccc6bfe99 is described below
> > > > 
> > > > commit ccc6bfe99d1981aabde6a3175866f99d38207f03
> > > > Author: Michael Osipov <michaelo@apache.org>
> > > > AuthorDate: Wed Oct 18 22:22:06 2023 +0200
> > > > 
> > > > BZ 67818: SSL#setVerify()/SSLContext#setVerify() silently set
> > > > undocumented default verify paths
> > > > ---
> > > > native/src/ssl.c                                   | 11 ++---------
> > > > native/src/sslcontext.c                     | 12 +++---------
> > > > xdocs/miscellaneous/changelog.xml |   4 ++++
> > > > 3 files changed, 9 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/native/src/ssl.c b/native/src/ssl.c
> > > > index e0b0461a9..7f4ca7e78 100644
> > > > --- a/native/src/ssl.c
> > > > +++ b/native/src/ssl.c
> > > > @@ -1177,15 +1177,8 @@ TCN_IMPLEMENT_CALL(void, SSL,
> > > > setVerify)(TCN_STDARGS, jlong ssl,
> > > > if ((c->verify_mode == SSL_CVERIFY_OPTIONAL) ||
> > > > (c->verify_mode == SSL_CVERIFY_OPTIONAL_NO_CA))
> > > > verify |= SSL_VERIFY_PEER;
> > > > -       if (!c->store) {
> > > > -               if (SSL_CTX_set_default_verify_paths(c->ctx)) {
> > > > -                       c->store = SSL_CTX_get_cert_store(c->ctx);
> > > > -                       X509_STORE_set_flags(c->store, 0);
> > > > -               }
> > > > -               else {
> > > > -                       /* XXX: See if this is fatal */
> > > > -               }
> > > > -       }
> > > > +       if (!c->store)
> > > > +               c->store = SSL_CTX_get_cert_store(c->ctx);
> > > > 
> > > > SSL_set_verify(ssl_, verify, SSL_callback_SSL_verify);
> > > > }
> > > > diff --git a/native/src/sslcontext.c b/native/src/sslcontext.c
> > > > index 34669ff70..f5b2b9831 100644
> > > > --- a/native/src/sslcontext.c
> > > > +++ b/native/src/sslcontext.c
> > > > @@ -35,6 +35,7 @@ static apr_status_t ssl_context_cleanup(void *data)
> > > > if (c) {
> > > > int i;
> > > > c->crl = NULL;
> > > > +               c->store = NULL;
> > > > if (c->ctx)
> > > > SSL_CTX_free(c->ctx);
> > > > c->ctx = NULL;
> > > > @@ -861,15 +862,8 @@ TCN_IMPLEMENT_CALL(void, SSLContext,
> > > > setVerify)(TCN_STDARGS, jlong ctx,
> > > > if ((c->verify_mode == SSL_CVERIFY_OPTIONAL) ||
> > > > (c->verify_mode == SSL_CVERIFY_OPTIONAL_NO_CA))
> > > > verify |= SSL_VERIFY_PEER;
> > > > -       if (!c->store) {
> > > > -               if (SSL_CTX_set_default_verify_paths(c->ctx)) {
> > > > -                       c->store = SSL_CTX_get_cert_store(c->ctx);
> > > > -                       X509_STORE_set_flags(c->store, 0);
> > > > -               }
> > > > -               else {
> > > > -                       /* XXX: See if this is fatal */
> > > > -               }
> > > > -       }
> > > > +       if (!c->store)
> > > > +               c->store = SSL_CTX_get_cert_store(c->ctx);
> > > > 
> > > > SSL_CTX_set_verify(c->ctx, verify, SSL_callback_SSL_verify);
> > > > }
> > > > diff --git a/xdocs/miscellaneous/changelog.xml
> > > > b/xdocs/miscellaneous/changelog.xml
> > > > index ffd0e10f5..0aedd8212 100644
> > > > --- a/xdocs/miscellaneous/changelog.xml
> > > > +++ b/xdocs/miscellaneous/changelog.xml
> > > > @@ -59,6 +59,10 @@
> > > > <update>
> > > > Remove an unreachable if condition around CRLs in sslcontext.c.
> > > > (michaelo)
> > > > </update>
> > > > +       <fix>
> > > > +           <bug>67818</bug>:
> > > > <code>SSL.setVerify()</code>/<code>SSLContext.setVerify()</code>
> > > > +           silently set undocumented default verify paths. (michaelo)
> > > > +       </fix>
> > > 
> > > I think this needs a better change log entry. It isn't clear if the paths
> > > were set and now are not set or vice versa.
> > 
> > I see. Can you propose something which is worded better? I wasn't able to come up \
> > with anything better. At most: SSL#setVerify()/SSLContext#setVerify() \
> > unconditionally silently set undocumented default verify paths
> 
> I think if you try to figure out how to get the words "now" and/or 
> "when" into the change-entry, it'll be more clear what's happening.

What about?

When <code>SSL.setVerify()</code>/<code>SSLContext.setVerify()</code> are invoked \
they silently set undocumented default verify paths. Now, one needs to properly \
configure those paths according to documentation.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


[prev in list] [next in list] [prev in thread] [next in thread]