You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by biilmann <gi...@git.apache.org> on 2016/01/01 01:18:34 UTC
[GitHub] trafficserver pull request: Add TSSslContextCreate method
GitHub user biilmann opened a pull request:
https://github.com/apache/trafficserver/pull/402
Add TSSslContextCreate method
TSSslContextCreate returns a new SSL Context that's configured according to
the settings in records.config
This is useful if an extension wants to use the TS_SSL_CERT_HOOK to
control loading of SNI certificates, and still want to respect the
cipher suite and related SSL settings.
The current experimental plugin for loading SSL certificates just use [SSL_CTX_new(SSLv23_client_method())](https://github.com/apache/trafficserver/blob/f54fbd5bf426c8ff42c16422e9b0708f79c3745e/plugins/experimental/ssl_cert_loader/ssl-cert-loader.cc#L156) to create a new context, and doesn't respect the cipher suite settings in records.config.
The meat of this implementation is in SSLCreateServerContext and could be cleaned up a lot, since there's a lot of repetition between this method and ssl_store_ssl_context. Ideally I would refactor that method to separate context initialization and configuration from inserting the context into the SSLCertLookup. Just wanted to check first to see if there is support for adding the new TSSslContextCreate API method...
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/biilmann/trafficserver ssl-ctx
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/402.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #402
----
commit cdc051d2478da8dc6c1076a6c01ee5d9c7de7db1
Author: Mathias Biilmann Christensen <in...@mathias-biilmann.net>
Date: 2015-12-29T06:45:03Z
Add TSSslContextCreate method
This method returns a new SSL Context that's configured according to
the settings in records.config
This is useful if an extension wants to use the TS_SSL_CERT_HOOK to
control loading of SNI certificates, and still want to respect the
cipher suite and related SSL settings.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-199870599
@biilmann Can you email something to the dev@ mailing list with the API proposal, for a review? See the link above for the process.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by biilmann <gi...@git.apache.org>.
Github user biilmann commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-169107617
> I assume that the bulk of the diff'ed lines in SSLUtils are due to just moving functions ahead of use and the functions themselves are unchanged (e.g., asn1_strdup).
Yes, the actual changes in SSLUtils is moving some setup related code from `ssl_store_ssl_context` into `SSLInitServerContext`, adding a new `SSLCreateServerContext` wrapper method and then moving `SSLInitServerContext` down below the functions it now depends on.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-187818521
To @jpeach to shepherd.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-214042675
Closing in favor of #594.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by biilmann <gi...@git.apache.org>.
Github user biilmann commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-168449041
I've done a bit of cleanup to get anything not related to storing the ssl context moved from `ssl_store_ssl_context` and into `SSLInitServerContext`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-168268438
> Ideally I would refactor that method to separate context initialization and configuration from
> inserting the context into the SSLCertLookup
Yeh that is the way it is supposed to work, but the separation has faded over time and it is a bit of a jumble now :(
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by biilmann <gi...@git.apache.org>.
Github user biilmann commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-171775715
@shinrich I'll send out an API review notice. Before that, what's your thoughts on adding a `TSSslContextDestroy` method to clarify that it's up to the plugin to eventually free the context?
Another issue is what @jpeach mentioned around session ticket initialization. I've been digging into this a bit, and I think it would be best to expose some separate methods around session ticket handling. It's actually something it would be really valuable for plugins to hook into. An important use case would be to coordinate key creation and rotation amongst several ATS servers in a CDN PoP, etc..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach closed the pull request at:
https://github.com/apache/trafficserver/pull/402
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-168898043
This looks pretty reasonable to me. @shinrich, what do you think?
In ``ts/ts.h``, ``TSSslContextCreate()`` needs to be ``TSSslContextCreate(void)``.
Once a plugin calls ``TSSslContextCreate()``, does it make sense for it to have to call ``SSL_CTX_free()``? I'm probably OK with that as long as we document.
Does it make sense to also hoist the session ticket initialization up into ``SSLInitServerContext()``?
Since this is a change to the public API we need to do an [API review](https://cwiki.apache.org/confluence/display/TS/API+Review+Process) on the dev@ list.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-197684472
@biilmann The next thing we need here is an [API review](https://cwiki.apache.org/confluence/display/TS/API+Review+Process) posted to the dev@ list. It would also are very helpful to add a simple API reference page to the documentation. There are a number of examples [here](https://github.com/apache/trafficserver/tree/master/doc/developer-guide/api/functions).
Once we have the API review done, I can get this integrated.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-171990184
Yes, I agree that having a matching TSSslContextDestroy is a good idea even if it is just a wrapper for the openssl call at this point.
I also like the idea of adding hook points to aid in coordination of session ticket handling. I had proposed something similar for coordinating ssl session state between servers (TS-3527). I had initial code but met with resistance to the idea and got pulled into other things. But I'd be very interested in coming back to that and making session restarts work better in a multi-server environment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by biilmann <gi...@git.apache.org>.
Github user biilmann commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-168927960
Changed the type signature to `TSSslContextCreate(void)` now.
It generally makes sense that a plugin that needs this method will want to handle the call to free the context as well. However, adding a `TSSslContextDestroy` function would probably make this more obvious and would mirror the semantics of other functions like `TSIOBufferCreate` and `TSIOBufferDestroy`...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-169014530
Looks good to me as well. Seems like a good and necessary change.
I assume that the bulk of the diff'ed lines in SSLUtils are due to just moving functions ahead of use and the functions themselves are unchanged (e.g., asn1_strdup).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: Add TSSslContextCreate method
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/402#issuecomment-170925335
I think this looks good, and I'd be willing to do the merge, but I think you still need to send around an API review notice to the dev mailing list as jpeach suggested.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---