You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by persiaAziz <gi...@git.apache.org> on 2016/08/30 20:12:18 UTC
[GitHub] trafficserver pull request #942: TS 4263: Global key block configurable via ...
GitHub user persiaAziz opened a pull request:
https://github.com/apache/trafficserver/pull/942
TS 4263: Global key block configurable via Records.config
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/persiaAziz/trafficserver TS-4263
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/942.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 #942
----
commit d51b0f06156669258dda23a8e593b480f3f69bb8
Author: Persia Aziz <pe...@yahoo-inc.com>
Date: 2016-08-26T18:15:12Z
TS-4263: Global key block configurable via Records.config
----
---
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 issue #942: TS 4263: Global key block configurable via Records...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on the issue:
https://github.com/apache/trafficserver/pull/942
If the ticket key file has a keyblock which is less than 48 bytes, then the assertion statement in ssl_callback_session_ticket(..) will fail. This is the current logic. One thing that confuses is that we create random ticket keys if the filename of key is NULL but fail on assertion if the keyblock is NULL. Can't we make a new random keyblock in such case? @shinrich @jpeach @zwoop
---
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 issue #942: TS 4263: Global key block configurable via Records...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/942
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/642/ for details.
---
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 #942: TS 4263: Global key block configurable via ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/942#discussion_r76876810
--- Diff: mgmt/RecordsConfig.cc ---
@@ -1245,7 +1245,7 @@ static const RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.ssl.server.multicert.exit_on_load_fail", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
,
- {RECT_CONFIG, "proxy.config.ssl.server.ticket_key.filename", RECD_STRING, "ssl_ticket.key", RECU_DYNAMIC, RR_NULL, RECC_NULL, NULL, RECA_NULL}
+ {RECT_CONFIG, "proxy.config.ssl.server.ticket_key.filename", RECD_STRING, "ticket.key", RECU_DYNAMIC, RR_NULL, RECC_NULL, NULL, RECA_NULL}
--- End diff --
I'm still not sure that it's worthwhile renaming it, the old name was pretty descriptive :). But I don't feel strongly.
---
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 issue #942: TS 4263: Global key block configurable via Records...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/942
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/538/ for details.
---
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 issue #942: TS 4263: Global key block configurable via Records...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the issue:
https://github.com/apache/trafficserver/pull/942
I think we should fail, if the user gives us a bad file (file to short or file not present). In that case it seems that they intended to specify a key but failed somehow.
If they don't specify any file, then they are wanting to go with a default of creating a random key on the file.
---
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 #942: TS 4263: Global key block configurable via ...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/942#discussion_r76876497
--- Diff: mgmt/RecordsConfig.cc ---
@@ -1245,7 +1245,7 @@ static const RecordElement RecordsConfig[] =
,
{RECT_CONFIG, "proxy.config.ssl.server.multicert.exit_on_load_fail", RECD_INT, "0", RECU_RESTART_TS, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}
,
- {RECT_CONFIG, "proxy.config.ssl.server.ticket_key.filename", RECD_STRING, "ssl_ticket.key", RECU_DYNAMIC, RR_NULL, RECC_NULL, NULL, RECA_NULL}
+ {RECT_CONFIG, "proxy.config.ssl.server.ticket_key.filename", RECD_STRING, "ticket.key", RECU_DYNAMIC, RR_NULL, RECC_NULL, NULL, RECA_NULL}
--- End diff --
Please update the documentation, ``doc/admin-guide/files/records.config.en.rst``.
---
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 #942: TS 4263: Global key block configurable via ...
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich closed the pull request at:
https://github.com/apache/trafficserver/pull/942
---
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 issue #942: TS 4263: Global key block configurable via Records...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/942
[approve ci]
---
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 #942: TS 4263: Global key block configurable via ...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/942#discussion_r76879380
--- Diff: iocore/net/SSLUtils.cc ---
@@ -2018,11 +2047,17 @@ SSLParseCertificateConfiguration(const SSLConfigParams *params, SSLCertLookup *l
char *tok_state = NULL;
char *line = NULL;
ats_scoped_str file_buf;
+ ats_scoped_str ticket_key_filename;
unsigned line_num = 0;
matcher_line line_info;
const matcher_tags sslCertTags = {NULL, NULL, NULL, NULL, NULL, NULL, false};
+ // load the global ticket key for later use
+ REC_ReadConfigStringAlloc(ticket_key_filename, "proxy.config.ssl.server.ticket_key.filename");
+ ats_scoped_str ticket_key_path(Layout::relative_to(params->serverCertPathOnly, ticket_key_filename));
+ global_default_keyblock = ssl_create_ticket_keyblock(ticket_key_path); // this function just returns a keyblock
--- End diff --
Question: What happens if the ticket file is empty? You create an empty key block, and then try to look up in that and fail (hopefully) gracefully?
That does seem reasonable, but I wonder if the logic should be to do the old default, and look for the dest_ip=*'s keychain at that point? At a minimum, we'd want to document in the release notes that going forward with 7.0.0, someone (like us) using the dest_ip=* as a default now must move that to the global configuration.
Alternatively, maybe we should have a warning that if someone has enabled session tickets, but not providing a key block for either all lines in ssl_multicert.config *or* at least one global one in tickets.config? That seems a little complicated to deal with though, so maybe that's a future RFE.
---
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 #942: TS 4263: Global key block configurable via ...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/942#discussion_r76885786
--- Diff: iocore/net/SSLUtils.cc ---
@@ -2018,11 +2047,17 @@ SSLParseCertificateConfiguration(const SSLConfigParams *params, SSLCertLookup *l
char *tok_state = NULL;
char *line = NULL;
ats_scoped_str file_buf;
+ ats_scoped_str ticket_key_filename;
unsigned line_num = 0;
matcher_line line_info;
const matcher_tags sslCertTags = {NULL, NULL, NULL, NULL, NULL, NULL, false};
+ // load the global ticket key for later use
+ REC_ReadConfigStringAlloc(ticket_key_filename, "proxy.config.ssl.server.ticket_key.filename");
+ ats_scoped_str ticket_key_path(Layout::relative_to(params->serverCertPathOnly, ticket_key_filename));
+ global_default_keyblock = ssl_create_ticket_keyblock(ticket_key_path); // this function just returns a keyblock
--- End diff --
hmm.. I have a checking for the file path, if the file path is null, then a random key will be generated. I guess we could do the same for invalid files too?
---
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.
---